Commit 1d2839a8 authored by Shaya Potter's avatar Shaya Potter Committed by Oran Agra
Browse files

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 6e993a5d

)
Co-authored-by: default avatarOran Agra <oran@redislabs.com>
(cherry picked from commit 71e6abe4)
parent c340fd5a
...@@ -577,7 +577,7 @@ void *RM_PoolAlloc(RedisModuleCtx *ctx, size_t bytes) { ...@@ -577,7 +577,7 @@ void *RM_PoolAlloc(RedisModuleCtx *ctx, size_t bytes) {
* Helpers for modules API implementation * Helpers for modules API implementation
* -------------------------------------------------------------------------- */ * -------------------------------------------------------------------------- */
   
client *moduleAllocTempClient(user *user) { client *moduleAllocTempClient(void) {
client *c = NULL; client *c = NULL;
   
if (moduleTempClientCount > 0) { if (moduleTempClientCount > 0) {
...@@ -587,10 +587,8 @@ client *moduleAllocTempClient(user *user) { ...@@ -587,10 +587,8 @@ client *moduleAllocTempClient(user *user) {
} else { } else {
c = createClient(NULL); c = createClient(NULL);
c->flags |= CLIENT_MODULE; c->flags |= CLIENT_MODULE;
c->user = NULL; /* Root user */
} }
c->user = user;
return c; return c;
} }
   
...@@ -774,7 +772,7 @@ void moduleCreateContext(RedisModuleCtx *out_ctx, RedisModule *module, int ctx_f ...@@ -774,7 +772,7 @@ void moduleCreateContext(RedisModuleCtx *out_ctx, RedisModule *module, int ctx_f
out_ctx->module = module; out_ctx->module = module;
out_ctx->flags = ctx_flags; out_ctx->flags = ctx_flags;
if (ctx_flags & REDISMODULE_CTX_TEMP_CLIENT) if (ctx_flags & REDISMODULE_CTX_TEMP_CLIENT)
out_ctx->client = moduleAllocTempClient(NULL); out_ctx->client = moduleAllocTempClient();
else if (ctx_flags & REDISMODULE_CTX_NEW_CLIENT) else if (ctx_flags & REDISMODULE_CTX_NEW_CLIENT)
out_ctx->client = createClient(NULL); out_ctx->client = createClient(NULL);
   
...@@ -5816,20 +5814,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5816,20 +5814,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
error_as_call_replies = flags & REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS; error_as_call_replies = flags & REDISMODULE_ARGV_CALL_REPLIES_AS_ERRORS;
va_end(ap); va_end(ap);
   
user *user = NULL; c = moduleAllocTempClient();
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);
   
/* We do not want to allow block, the module do not expect it */ /* We do not want to allow block, the module do not expect it */
c->flags |= CLIENT_DENY_BLOCKING; c->flags |= CLIENT_DENY_BLOCKING;
...@@ -5846,6 +5831,20 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5846,6 +5831,20 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
} }
if (ctx->module) ctx->module->in_call++; 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 handle the above format error only when the client is setup so that
* we can free it normally. */ * we can free it normally. */
if (argv == NULL) { if (argv == NULL) {
...@@ -7227,8 +7226,8 @@ RedisModuleBlockedClient *moduleBlockClient(RedisModuleCtx *ctx, RedisModuleCmdF ...@@ -7227,8 +7226,8 @@ RedisModuleBlockedClient *moduleBlockClient(RedisModuleCtx *ctx, RedisModuleCmdF
bc->disconnect_callback = NULL; /* Set by RM_SetDisconnectCallback() */ bc->disconnect_callback = NULL; /* Set by RM_SetDisconnectCallback() */
bc->free_privdata = free_privdata; bc->free_privdata = free_privdata;
bc->privdata = privdata; bc->privdata = privdata;
bc->reply_client = moduleAllocTempClient(NULL); bc->reply_client = moduleAllocTempClient();
bc->thread_safe_ctx_client = moduleAllocTempClient(NULL); bc->thread_safe_ctx_client = moduleAllocTempClient();
if (bc->client) if (bc->client)
bc->reply_client->resp = bc->client->resp; bc->reply_client->resp = bc->client->resp;
bc->dbid = c->db->id; bc->dbid = c->db->id;
......
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