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

@@ -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];
     }

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,15 +849,15 @@ 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;
     }
 
     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

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}
+    }
+}
+

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)} {