From b0e18f804d016dd6664c71920d6c290c0af08909 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Sun, 22 May 2022 16:02:59 +0300 Subject: [PATCH] Scripts that declare the `no-writes` flag are implicitly `allow-oom` too. (#10699) Scripts that have the `no-writes` flag, cannot execute write commands, and since all `deny-oom` commands are write commands, we now act as if the `allow-oom` flag is implicitly set for scripts that set the `no-writes` flag. this also implicitly means that the EVAL*_RO and FCALL_RO commands can never fails with OOM error. Note about a bug that's no longer relevant: There was an issue with EVAL*_RO using shebang not being blocked correctly in OOM state: When an EVAL script declares a shebang, it was by default not allowed to run in OOM state. but this depends on a flag that is updated before the command is executed, which was not updated in case of the `_RO` variants. the result is that if the previous cached state was outdated (either true or false), the script will either unjustly fail with OOM, or unjustly allowed to run despite the OOM state. It doesn't affect scripts without a shebang since these depend on the actual commands they run, and since these are only read commands, they don't care for that cached oom state flag. it did affect scripts with shebang and no allow-oom flag, bug after the change in this PR, scripts that are run with eval_ro would implicitly have that flag so again the cached state doesn't matter. p.s. this isn't a breaking change since all it does is allow scripts to run when they should / could rather than blocking them. --- src/script.c | 19 +++++++++++++------ src/server.c | 2 ++ tests/unit/functions.tcl | 7 ++----- tests/unit/scripting.tcl | 31 +++++++++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/script.c b/src/script.c index d4325ee05..9f474f8e6 100644 --- a/src/script.c +++ b/src/script.c @@ -123,12 +123,6 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca return C_ERR; } - if (!(script_flags & SCRIPT_FLAG_ALLOW_OOM) && server.script_oom && server.maxmemory) { - addReplyError(caller, "-OOM allow-oom flag is not set on the script, " - "can not run it when used memory > 'maxmemory'"); - return C_ERR; - } - if (running_stale && !(script_flags & SCRIPT_FLAG_ALLOW_STALE)) { addReplyError(caller, "-MASTERDOWN Link with MASTER is down, " "replica-serve-stale-data is set to 'no' " @@ -177,6 +171,17 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca return C_ERR; } } + + /* Check OOM state. the no-writes flag imply allow-oom. we tested it + * after the no-write error, so no need to mention it in the error reply. */ + if (server.script_oom && server.maxmemory && + !(script_flags & (SCRIPT_FLAG_ALLOW_OOM|SCRIPT_FLAG_NO_WRITES))) + { + addReplyError(caller, "-OOM allow-oom flag is not set on the script, " + "can not run it when used memory > 'maxmemory'"); + return C_ERR; + } + } else { /* Special handling for backwards compatibility (no shebang eval[sha]) mode */ if (running_stale) { @@ -214,6 +219,8 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca run_ctx->flags |= SCRIPT_READ_ONLY; } if (!(script_flags & SCRIPT_FLAG_EVAL_COMPAT_MODE) && (script_flags & SCRIPT_FLAG_ALLOW_OOM)) { + /* Note: we don't need to test the no-writes flag here and set this run_ctx flag, + * since only write commands can are deny-oom. */ run_ctx->flags |= SCRIPT_ALLOW_OOM; } diff --git a/src/server.c b/src/server.c index 14019b671..2296ba39b 100644 --- a/src/server.c +++ b/src/server.c @@ -3750,7 +3750,9 @@ int processCommand(client *c) { * until first write within script, memory used by lua stack and * arguments might interfere. */ if (c->cmd->proc == evalCommand || + c->cmd->proc == evalRoCommand || c->cmd->proc == evalShaCommand || + c->cmd->proc == evalShaRoCommand || c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand) { diff --git a/tests/unit/functions.tcl b/tests/unit/functions.tcl index e9aeda16b..0accb4958 100644 --- a/tests/unit/functions.tcl +++ b/tests/unit/functions.tcl @@ -1064,11 +1064,8 @@ start_server {tags {"scripting"}} { r config set maxmemory 1 - catch {r fcall f1 1 k} e - assert_match {*can not run it when used memory > 'maxmemory'*} $e - - catch {r fcall_ro f1 1 k} e - assert_match {*can not run it when used memory > 'maxmemory'*} $e + assert_equal [r fcall f1 1 k] hello + assert_equal [r fcall_ro f1 1 k] hello r config set maxmemory 0 } {OK} {needs:config-maxmemory} diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index 439a1e71a..f85fc8484 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -1422,12 +1422,43 @@ start_server {tags {"scripting"}} { } 0 } + # Script with allow-oom can write despite being in OOM state assert_equal [ r eval {#!lua flags=allow-oom redis.call('set','x',1) return 1 + } 1 x + ] 1 + + # read-only scripts implies allow-oom + assert_equal [ + r eval {#!lua flags=no-writes + redis.call('get','x') + return 1 } 0 ] 1 + assert_equal [ + r eval_ro {#!lua flags=no-writes + redis.call('get','x') + return 1 + } 1 x + ] 1 + + # Script with no shebang can read in OOM state + assert_equal [ + r eval { + redis.call('get','x') + return 1 + } 1 x + ] 1 + + # Script with no shebang can read in OOM state (eval_ro variant) + assert_equal [ + r eval_ro { + redis.call('get','x') + return 1 + } 1 x + ] 1 r config set maxmemory 0 } {OK} {needs:config-maxmemory}