diff --git a/src/module.c b/src/module.c index 034105d3e..cc6934dca 100644 --- a/src/module.c +++ b/src/module.c @@ -339,6 +339,7 @@ typedef struct RedisModuleDictIter { typedef struct RedisModuleCommandFilterCtx { RedisModuleString **argv; + int argv_len; int argc; } RedisModuleCommandFilterCtx; @@ -10078,6 +10079,7 @@ void moduleCallCommandFilters(client *c) { RedisModuleCommandFilterCtx filter = { .argv = c->argv, + .argv_len = c->argv_len, .argc = c->argc }; @@ -10094,6 +10096,7 @@ void moduleCallCommandFilters(client *c) { } c->argv = filter.argv; + c->argv_len = filter.argv_len; c->argc = filter.argc; } @@ -10125,7 +10128,10 @@ int RM_CommandFilterArgInsert(RedisModuleCommandFilterCtx *fctx, int pos, RedisM if (pos < 0 || pos > fctx->argc) return REDISMODULE_ERR; - fctx->argv = zrealloc(fctx->argv, (fctx->argc+1)*sizeof(RedisModuleString *)); + if (fctx->argv_len < fctx->argc+1) { + fctx->argv_len = fctx->argc+1; + fctx->argv = zrealloc(fctx->argv, fctx->argv_len*sizeof(RedisModuleString *)); + } for (i = fctx->argc; i > pos; i--) { fctx->argv[i] = fctx->argv[i-1]; } diff --git a/src/script_lua.c b/src/script_lua.c index c86cdc2d1..365a41fb5 100644 --- a/src/script_lua.c +++ b/src/script_lua.c @@ -783,7 +783,7 @@ static void luaReplyToRedisReply(client *c, client* script_client, lua_State *lu /* --------------------------------------------------------------------------- * Lua redis.* functions implementations. * ------------------------------------------------------------------------- */ -void freeLuaRedisArgv(robj **argv, int argc); +void freeLuaRedisArgv(robj **argv, int argc, int argv_len); /* Cached argv array across calls. */ static robj **lua_argv = NULL; @@ -795,7 +795,7 @@ static int lua_argv_size = 0; static robj *lua_args_cached_objects[LUA_CMD_OBJCACHE_SIZE]; static size_t lua_args_cached_objects_len[LUA_CMD_OBJCACHE_SIZE]; -static robj **luaArgsToRedisArgv(lua_State *lua, int *argc) { +static robj **luaArgsToRedisArgv(lua_State *lua, int *argc, int *argv_len) { int j; /* Require at least one argument */ *argc = lua_gettop(lua); @@ -809,6 +809,7 @@ static robj **luaArgsToRedisArgv(lua_State *lua, int *argc) { lua_argv = zrealloc(lua_argv,sizeof(robj*)* *argc); lua_argv_size = *argc; } + *argv_len = lua_argv_size; for (j = 0; j < *argc; j++) { char *obj_s; @@ -848,7 +849,7 @@ static robj **luaArgsToRedisArgv(lua_State *lua, int *argc) { * is not a string or an integer (lua_isstring() return true for * integers as well). */ if (j != *argc) { - freeLuaRedisArgv(lua_argv, j); + freeLuaRedisArgv(lua_argv, j, lua_argv_size); luaPushError(lua, "Lua redis lib command arguments must be strings or integers"); return NULL; } @@ -856,7 +857,7 @@ static robj **luaArgsToRedisArgv(lua_State *lua, int *argc) { return lua_argv; } -void freeLuaRedisArgv(robj **argv, int argc) { +void freeLuaRedisArgv(robj **argv, int argc, int argv_len) { int j; for (j = 0; j < argc; j++) { robj *o = argv[j]; @@ -878,7 +879,7 @@ void freeLuaRedisArgv(robj **argv, int argc) { decrRefCount(o); } } - if (argv != lua_argv) { + if (argv != lua_argv || argv_len != lua_argv_size) { /* The command changed argv, scrap the cache and start over. */ zfree(argv); lua_argv = NULL; @@ -894,8 +895,7 @@ static int luaRedisGenericCommand(lua_State *lua, int raise_error) { client* c = rctx->c; sds reply; - c->argv = luaArgsToRedisArgv(lua, &c->argc); - c->argv_len = lua_argv_size; + c->argv = luaArgsToRedisArgv(lua, &c->argc, &c->argv_len); if (c->argv == NULL) { return raise_error ? luaError(lua) : 1; } @@ -977,7 +977,7 @@ static int luaRedisGenericCommand(lua_State *lua, int raise_error) { cleanup: /* Clean up. Command code may have changed argv/argc so we use the * argv/argc of the client instead of the local variables. */ - freeLuaRedisArgv(c->argv, c->argc); + freeLuaRedisArgv(c->argv, c->argc, c->argv_len); c->argc = c->argv_len = 0; c->user = NULL; c->argv = NULL; @@ -1128,8 +1128,8 @@ static int luaRedisAclCheckCmdPermissionsCommand(lua_State *lua) { serverAssert(rctx); /* Only supported inside script invocation */ int raise_error = 0; - int argc; - robj **argv = luaArgsToRedisArgv(lua, &argc); + int argc, argv_len; + robj **argv = luaArgsToRedisArgv(lua, &argc, &argv_len); /* Require at least one argument */ if (argv == NULL) return luaError(lua); @@ -1148,7 +1148,7 @@ static int luaRedisAclCheckCmdPermissionsCommand(lua_State *lua) { } } - freeLuaRedisArgv(argv, argc); + freeLuaRedisArgv(argv, argc, argv_len); if (raise_error) return luaError(lua); else diff --git a/tests/unit/moduleapi/commandfilter.tcl b/tests/unit/moduleapi/commandfilter.tcl index 723dd1409..427609d3e 100644 --- a/tests/unit/moduleapi/commandfilter.tcl +++ b/tests/unit/moduleapi/commandfilter.tcl @@ -96,3 +96,23 @@ start_server {tags {"modules"}} { assert_equal {OK} [r module unload commandfilter] } } + +test {RM_CommandFilterArgInsert and script argv caching} { + # coverage for scripts calling commands that expand the argv array + # an attempt to add coverage for a possible bug in luaArgsToRedisArgv + # this test needs a fresh server so that lua_argv_size is 0. + # glibc realloc can return the same pointer even when the size changes + # still this test isn't able to trigger the issue, but we keep it anyway. + start_server {tags {"modules"}} { + r module load $testmodule log-key 0 + r del mylist + # command with 6 args + r eval {redis.call('rpush', KEYS[1], 'elem1', 'elem2', 'elem3', 'elem4')} 1 mylist + # command with 3 args that is changed to 4 + r eval {redis.call('rpush', KEYS[1], '@insertafter')} 1 mylist + # command with 6 args again + r eval {redis.call('rpush', KEYS[1], 'elem1', 'elem2', 'elem3', 'elem4')} 1 mylist + assert_equal [r lrange mylist 0 -1] {elem1 elem2 elem3 elem4 @insertafter --inserted-after-- elem1 elem2 elem3 elem4} + } +} + diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index fa617d7fe..440b9e2e1 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -585,6 +585,30 @@ start_server {tags {"scripting"}} { close_replication_stream $repl } {} {needs:repl} + test {INCRBYFLOAT: We can call scripts expanding client->argv from Lua} { + # coverage for scripts calling commands that expand the argv array + # an attempt to add coverage for a possible bug in luaArgsToRedisArgv + # this test needs a fresh server so that lua_argv_size is 0. + # glibc realloc can return the same pointer even when the size changes + # still this test isn't able to trigger the issue, but we keep it anyway. + start_server {tags {"scripting"}} { + set repl [attach_to_replication_stream] + # a command with 5 argsument + r eval {redis.call('hmget', KEYS[1], 1, 2, 3)} 1 key + # then a command with 3 that is replicated as one with 4 + r eval {redis.call('incrbyfloat', KEYS[1], 1)} 1 key + # then a command with 4 args + r eval {redis.call('set', KEYS[1], '1', 'KEEPTTL')} 1 key + + assert_replication_stream $repl { + {select *} + {set key 1 KEEPTTL} + {set key 1 KEEPTTL} + } + close_replication_stream $repl + } + } {} {needs:repl} + } ;# is_eval test {Call Redis command with many args from Lua (issue #1764)} {