From c8052122a2af9b85124a8697488a0c44d66777b8 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 4 Jan 2023 11:03:55 +0200 Subject: [PATCH] Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652) TLDR: solve a problem introduced in Redis 7.0.6 (#11541) with RM_CommandFilterArgInsert being called from scripts, which can lead to memory corruption. Libc realloc can return the same pointer even if the size was changed. The code in freeLuaRedisArgv had an assumption that if the pointer didn't change, then the allocation didn't change, and the cache can still be reused. However, if rewriteClientCommandArgument or RM_CommandFilterArgInsert were used, it could be that we realloced the argv array, and the pointer didn't change, then a consecutive command being executed from Lua can use that argv cache reaching beyond its size. This was actually only possible with modules, since the decision to realloc was based on argc, rather than argv_len. --- src/module.c | 8 +++++++- src/script_lua.c | 22 +++++++++++----------- tests/unit/moduleapi/commandfilter.tcl | 20 ++++++++++++++++++++ tests/unit/scripting.tcl | 24 ++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 12 deletions(-) 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)} {