Unverified Commit b0e18f80 authored by Oran Agra's avatar Oran Agra Committed by GitHub
Browse files

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.
parent cb6933e3
...@@ -123,12 +123,6 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca ...@@ -123,12 +123,6 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
return C_ERR; 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)) { if (running_stale && !(script_flags & SCRIPT_FLAG_ALLOW_STALE)) {
addReplyError(caller, "-MASTERDOWN Link with MASTER is down, " addReplyError(caller, "-MASTERDOWN Link with MASTER is down, "
"replica-serve-stale-data is set to 'no' " "replica-serve-stale-data is set to 'no' "
...@@ -177,6 +171,17 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca ...@@ -177,6 +171,17 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
return C_ERR; 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 { } else {
/* Special handling for backwards compatibility (no shebang eval[sha]) mode */ /* Special handling for backwards compatibility (no shebang eval[sha]) mode */
if (running_stale) { if (running_stale) {
...@@ -214,6 +219,8 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca ...@@ -214,6 +219,8 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
run_ctx->flags |= SCRIPT_READ_ONLY; run_ctx->flags |= SCRIPT_READ_ONLY;
} }
if (!(script_flags & SCRIPT_FLAG_EVAL_COMPAT_MODE) && (script_flags & SCRIPT_FLAG_ALLOW_OOM)) { 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; run_ctx->flags |= SCRIPT_ALLOW_OOM;
} }
......
...@@ -3750,7 +3750,9 @@ int processCommand(client *c) { ...@@ -3750,7 +3750,9 @@ int processCommand(client *c) {
* until first write within script, memory used by lua stack and * until first write within script, memory used by lua stack and
* arguments might interfere. */ * arguments might interfere. */
if (c->cmd->proc == evalCommand || if (c->cmd->proc == evalCommand ||
c->cmd->proc == evalRoCommand ||
c->cmd->proc == evalShaCommand || c->cmd->proc == evalShaCommand ||
c->cmd->proc == evalShaRoCommand ||
c->cmd->proc == fcallCommand || c->cmd->proc == fcallCommand ||
c->cmd->proc == fcallroCommand) c->cmd->proc == fcallroCommand)
{ {
......
...@@ -1064,11 +1064,8 @@ start_server {tags {"scripting"}} { ...@@ -1064,11 +1064,8 @@ start_server {tags {"scripting"}} {
r config set maxmemory 1 r config set maxmemory 1
catch {r fcall f1 1 k} e assert_equal [r fcall f1 1 k] hello
assert_match {*can not run it when used memory > 'maxmemory'*} $e assert_equal [r fcall_ro f1 1 k] hello
catch {r fcall_ro f1 1 k} e
assert_match {*can not run it when used memory > 'maxmemory'*} $e
r config set maxmemory 0 r config set maxmemory 0
} {OK} {needs:config-maxmemory} } {OK} {needs:config-maxmemory}
......
...@@ -1422,12 +1422,43 @@ start_server {tags {"scripting"}} { ...@@ -1422,12 +1422,43 @@ start_server {tags {"scripting"}} {
} 0 } 0
} }
# Script with allow-oom can write despite being in OOM state
assert_equal [ assert_equal [
r eval {#!lua flags=allow-oom r eval {#!lua flags=allow-oom
redis.call('set','x',1) redis.call('set','x',1)
return 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 } 0
] 1 ] 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 r config set maxmemory 0
} {OK} {needs:config-maxmemory} } {OK} {needs:config-maxmemory}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment