Unverified Commit bed6d759 authored by Shaya Potter's avatar Shaya Potter Committed by GitHub
Browse files

Improve cmd_flags for script/functions in RM_Call (#11159)

When RM_Call was used with `M` (reject OOM), `W` (reject writes),
as well as `S` (rejecting stale or write commands in "Script mode"),
it would have only checked the command flags, but not the declared
script flag in case it's a command that runs a script.

Refactoring: extracts out similar code in server.c's processCommand
to be usable in RM_Call as well.
parent 89450675
...@@ -592,6 +592,7 @@ void moduleReleaseTempClient(client *c) { ...@@ -592,6 +592,7 @@ void moduleReleaseTempClient(client *c) {
c->bufpos = 0; c->bufpos = 0;
c->flags = CLIENT_MODULE; c->flags = CLIENT_MODULE;
c->user = NULL; /* Root user */ c->user = NULL; /* Root user */
c->cmd = c->lastcmd = c->realcmd = NULL;
moduleTempClients[moduleTempClientCount++] = c; moduleTempClients[moduleTempClientCount++] = c;
} }
   
...@@ -5781,7 +5782,6 @@ fmterr: ...@@ -5781,7 +5782,6 @@ fmterr:
* This API is documented here: https://redis.io/topics/modules-intro * This API is documented here: https://redis.io/topics/modules-intro
*/ */
RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const char *fmt, ...) { RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const char *fmt, ...) {
struct redisCommand *cmd;
client *c = NULL; client *c = NULL;
robj **argv = NULL; robj **argv = NULL;
int argc = 0, argv_len = 0, flags = 0; int argc = 0, argv_len = 0, flags = 0;
...@@ -5789,6 +5789,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5789,6 +5789,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
RedisModuleCallReply *reply = NULL; RedisModuleCallReply *reply = NULL;
int replicate = 0; /* Replicate this command? */ int replicate = 0; /* Replicate this command? */
int error_as_call_replies = 0; /* return errors as RedisModuleCallReply object */ int error_as_call_replies = 0; /* return errors as RedisModuleCallReply object */
uint64_t cmd_flags;
   
/* Handle arguments. */ /* Handle arguments. */
va_start(ap, fmt); va_start(ap, fmt);
...@@ -5831,7 +5832,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5831,7 +5832,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
/* Lookup command now, after filters had a chance to make modifications /* Lookup command now, after filters had a chance to make modifications
* if necessary. * if necessary.
*/ */
cmd = c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc); c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc);
sds err; sds err;
if (!commandCheckExistence(c, error_as_call_replies? &err : NULL)) { if (!commandCheckExistence(c, error_as_call_replies? &err : NULL)) {
errno = ENOENT; errno = ENOENT;
...@@ -5846,10 +5847,12 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5846,10 +5847,12 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
goto cleanup; goto cleanup;
} }
   
cmd_flags = getCommandFlags(c);
if (flags & REDISMODULE_ARGV_SCRIPT_MODE) { if (flags & REDISMODULE_ARGV_SCRIPT_MODE) {
/* Basically on script mode we want to only allow commands that can /* Basically on script mode we want to only allow commands that can
* be executed on scripts (CMD_NOSCRIPT is not set on the command flags) */ * be executed on scripts (CMD_NOSCRIPT is not set on the command flags) */
if (cmd->flags & CMD_NOSCRIPT) { if (cmd_flags & CMD_NOSCRIPT) {
errno = ESPIPE; errno = ESPIPE;
if (error_as_call_replies) { if (error_as_call_replies) {
sds msg = sdscatfmt(sdsempty(), "command '%S' is not allowed on script mode", c->cmd->fullname); sds msg = sdscatfmt(sdsempty(), "command '%S' is not allowed on script mode", c->cmd->fullname);
...@@ -5860,7 +5863,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5860,7 +5863,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
} }
   
if (flags & REDISMODULE_ARGV_RESPECT_DENY_OOM) { if (flags & REDISMODULE_ARGV_RESPECT_DENY_OOM) {
if (cmd->flags & CMD_DENYOOM) { if (cmd_flags & CMD_DENYOOM) {
int oom_state; int oom_state;
if (ctx->flags & REDISMODULE_CTX_THREAD_SAFE) { if (ctx->flags & REDISMODULE_CTX_THREAD_SAFE) {
/* On background thread we can not count on server.pre_command_oom_state. /* On background thread we can not count on server.pre_command_oom_state.
...@@ -5882,7 +5885,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5882,7 +5885,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
} }
   
if (flags & REDISMODULE_ARGV_NO_WRITES) { if (flags & REDISMODULE_ARGV_NO_WRITES) {
if (cmd->flags & CMD_WRITE) { if (cmd_flags & CMD_WRITE) {
errno = ENOSPC; errno = ENOSPC;
if (error_as_call_replies) { if (error_as_call_replies) {
sds msg = sdscatfmt(sdsempty(), "Write command '%S' was " sds msg = sdscatfmt(sdsempty(), "Write command '%S' was "
...@@ -5895,7 +5898,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5895,7 +5898,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
   
/* Script mode tests */ /* Script mode tests */
if (flags & REDISMODULE_ARGV_SCRIPT_MODE) { if (flags & REDISMODULE_ARGV_SCRIPT_MODE) {
if (cmd->flags & CMD_WRITE) { if (cmd_flags & CMD_WRITE) {
/* on script mode, if a command is a write command, /* on script mode, if a command is a write command,
* We will not run it if we encounter disk error * We will not run it if we encounter disk error
* or we do not have enough replicas */ * or we do not have enough replicas */
...@@ -5932,7 +5935,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5932,7 +5935,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
} }
   
if (server.masterhost && server.repl_state != REPL_STATE_CONNECTED && if (server.masterhost && server.repl_state != REPL_STATE_CONNECTED &&
server.repl_serve_stale_data == 0 && !(cmd->flags & CMD_STALE)) { server.repl_serve_stale_data == 0 && !(cmd_flags & CMD_STALE)) {
errno = ESPIPE; errno = ESPIPE;
if (error_as_call_replies) { if (error_as_call_replies) {
sds msg = sdsdup(shared.masterdownerr->ptr); sds msg = sdsdup(shared.masterdownerr->ptr);
......
...@@ -3604,6 +3604,23 @@ int commandCheckArity(client *c, sds *err) { ...@@ -3604,6 +3604,23 @@ int commandCheckArity(client *c, sds *err) {
return 1; return 1;
} }
/* If we're executing a script, try to extract a set of command flags from
* it, in case it declared them. Note this is just an attempt, we don't yet
* know the script command is well formed.*/
uint64_t getCommandFlags(client *c) {
uint64_t cmd_flags = c->cmd->flags;
if (c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand) {
cmd_flags = fcallGetCommandFlags(c, cmd_flags);
} else if (c->cmd->proc == evalCommand || c->cmd->proc == evalRoCommand ||
c->cmd->proc == evalShaCommand || c->cmd->proc == evalShaRoCommand)
{
cmd_flags = evalGetCommandFlags(c, cmd_flags);
}
return cmd_flags;
}
/* If this function gets called we already read a whole /* If this function gets called we already read a whole
* command, arguments are in the client argv/argc fields. * command, arguments are in the client argv/argc fields.
* processCommand() execute the command or prepare the * processCommand() execute the command or prepare the
...@@ -3668,19 +3685,7 @@ int processCommand(client *c) { ...@@ -3668,19 +3685,7 @@ int processCommand(client *c) {
} }
} }
/* If we're executing a script, try to extract a set of command flags from uint64_t cmd_flags = getCommandFlags(c);
* it, in case it declared them. Note this is just an attempt, we don't yet
* know the script command is well formed.*/
uint64_t cmd_flags = c->cmd->flags;
if (c->cmd->proc == evalCommand || c->cmd->proc == evalShaCommand ||
c->cmd->proc == evalRoCommand || c->cmd->proc == evalShaRoCommand ||
c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand)
{
if (c->cmd->proc == fcallCommand || c->cmd->proc == fcallroCommand)
cmd_flags = fcallGetCommandFlags(c, cmd_flags);
else
cmd_flags = evalGetCommandFlags(c, cmd_flags);
}
int is_read_command = (cmd_flags & CMD_READONLY) || int is_read_command = (cmd_flags & CMD_READONLY) ||
(c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_READONLY)); (c->cmd->proc == execCommand && (c->mstate.cmd_flags & CMD_READONLY));
......
...@@ -2864,6 +2864,7 @@ int zslLexValueLteMax(sds value, zlexrangespec *spec); ...@@ -2864,6 +2864,7 @@ int zslLexValueLteMax(sds value, zlexrangespec *spec);
int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *level); int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *level);
size_t freeMemoryGetNotCountedMemory(); size_t freeMemoryGetNotCountedMemory();
int overMaxmemoryAfterAlloc(size_t moremem); int overMaxmemoryAfterAlloc(size_t moremem);
uint64_t getCommandFlags(client *c);
int processCommand(client *c); int processCommand(client *c);
int processPendingCommandAndInputBuffer(client *c); int processPendingCommandAndInputBuffer(client *c);
void setupSignalHandlers(void); void setupSignalHandlers(void);
......
...@@ -173,6 +173,29 @@ start_server {tags {"modules"}} { ...@@ -173,6 +173,29 @@ start_server {tags {"modules"}} {
r config set maxmemory 0 r config set maxmemory 0
} {OK} {needs:config-maxmemory} } {OK} {needs:config-maxmemory}
test {rm_call OOM Eval} {
r config set maxmemory 1
r config set maxmemory-policy volatile-lru
# use the M flag without allow-oom shebang flag
assert_error {OOM *} {
r test.rm_call_flags M eval {#!lua
redis.call('set','x',1)
return 1
} 1 x
}
# add the M flag with allow-oom shebang flag
assert_equal {1} [
r test.rm_call_flags M eval {#!lua flags=allow-oom
redis.call('set','x',1)
return 1
} 1 x
]
r config set maxmemory 0
} {OK} {needs:config-maxmemory}
test {rm_call write flag} { test {rm_call write flag} {
# add the W flag # add the W flag
assert_error {ERR Write command 'set' was called while write is not allowed.} { assert_error {ERR Write command 'set' was called while write is not allowed.} {
......
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