From 71e6abe423a36e9dfa9c74b7c0c9f45b5836adb7 Mon Sep 17 00:00:00 2001
From: Shaya Potter <shaya@redislabs.com>
Date: Tue, 23 May 2023 14:29:28 +0300
Subject: [PATCH] Fix memory leak when RM_Call's RUN_AS_USER fails (#12158)

previously the argv wasn't freed so would leak.  not a common case, but should be handled.

Solution: move RUN_AS_USER setup and error exit to the right place.
this way, when we do `goto cleanup` (instead of return) it'll automatically do the right thing (including autoMemoryAdd)
Removed the user argument from moduleAllocTempClient (reverted to the state before 6e993a5)

Co-authored-by: Oran Agra <oran@redislabs.com>
---
 src/module.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/src/module.c b/src/module.c
index 7277e3e72..1be1b7781 100644
--- a/src/module.c
+++ b/src/module.c
@@ -620,7 +620,7 @@ void *RM_PoolAlloc(RedisModuleCtx *ctx, size_t bytes) {
  * Helpers for modules API implementation
  * -------------------------------------------------------------------------- */
 
-client *moduleAllocTempClient(user *user) {
+client *moduleAllocTempClient(void) {
     client *c = NULL;
 
     if (moduleTempClientCount > 0) {
@@ -630,10 +630,8 @@ client *moduleAllocTempClient(user *user) {
     } else {
         c = createClient(NULL);
         c->flags |= CLIENT_MODULE;
+        c->user = NULL; /* Root user */
     }
-
-    c->user = user;
-
     return c;
 }
 
@@ -879,7 +877,7 @@ void moduleCreateContext(RedisModuleCtx *out_ctx, RedisModule *module, int ctx_f
     out_ctx->module = module;
     out_ctx->flags = ctx_flags;
     if (ctx_flags & REDISMODULE_CTX_TEMP_CLIENT)
-        out_ctx->client = moduleAllocTempClient(NULL);
+        out_ctx->client = moduleAllocTempClient();
     else if (ctx_flags & REDISMODULE_CTX_NEW_CLIENT)
         out_ctx->client = createClient(NULL);
 
@@ -6215,20 +6213,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
     error_as_call_replies = flags & REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS;
     va_end(ap);
 
-    user *user = NULL;
-    if (flags & REDISMODULE_ARGV_RUN_AS_USER) {
-        user = ctx->user ? ctx->user->user : ctx->client->user;
-        if (!user) {
-            errno = ENOTSUP;
-            if (error_as_call_replies) {
-                sds msg = sdsnew("cannot run as user, no user directly attached to context or context's client");
-                reply = callReplyCreateError(msg, ctx);
-            }
-            return reply;
-        }
-    }
-
-    c = moduleAllocTempClient(user);
+    c = moduleAllocTempClient();
 
     if (!(flags & REDISMODULE_ARGV_ALLOW_BLOCK)) {
         /* We do not want to allow block, the module do not expect it */
@@ -6248,6 +6233,20 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
     }
     if (ctx->module) ctx->module->in_call++;
 
+    user *user = NULL;
+    if (flags & REDISMODULE_ARGV_RUN_AS_USER) {
+        user = ctx->user ? ctx->user->user : ctx->client->user;
+        if (!user) {
+            errno = ENOTSUP;
+            if (error_as_call_replies) {
+                sds msg = sdsnew("cannot run as user, no user directly attached to context or context's client");
+                reply = callReplyCreateError(msg, ctx);
+            }
+            goto cleanup;
+        }
+        c->user = user;
+    }
+
     /* We handle the above format error only when the client is setup so that
      * we can free it normally. */
     if (argv == NULL) {
@@ -7680,8 +7679,8 @@ RedisModuleBlockedClient *moduleBlockClient(RedisModuleCtx *ctx, RedisModuleCmdF
     bc->disconnect_callback = NULL; /* Set by RM_SetDisconnectCallback() */
     bc->free_privdata = free_privdata;
     bc->privdata = privdata;
-    bc->reply_client = moduleAllocTempClient(NULL);
-    bc->thread_safe_ctx_client = moduleAllocTempClient(NULL);
+    bc->reply_client = moduleAllocTempClient();
+    bc->thread_safe_ctx_client = moduleAllocTempClient();
     if (bc->client)
         bc->reply_client->resp = bc->client->resp;
     bc->dbid = c->db->id;