fix(server): fix memory leak on lua error #4236

adiholden · 2024-12-01T20:36:13Z

The bug:
calling lua_error does not return, instead it unwinds the Lua call stack until an error handler is found or the
script exits. This lead to memory leak on object that should release memory in destructor.
Specific example is the absl::FixedArray<string_view, 4> args(argc); which allocates on heap if argc > 4. The free was not called leading to memory leak.
The fix:
Add scoping to RedisGenericCommand and call RaiseError which calls lua_error after goto after destructors are called

Signed-off-by: adi_holden <[email protected]>

romange · 2024-12-01T22:32:23Z

Good catch, Adi! What do you mean by scoping? how this change makes sure that args are released now?

src/core/interpreter.cc

chakaz · 2024-12-02T07:07:16Z

src/core/interpreter.cc

@@ -467,7 +469,7 @@ int RedisLogCommand(lua_State* lua) {
  int argc = lua_gettop(lua);
  if (argc < 2) {
    PushError(lua, "redis.log() requires two arguments or more.");
-    return RaiseError(lua);
+    RaiseError(lua);


I think that keeping the return part here makes sense, like the lua API docs mention (this way readers can't be mislead to think that the flow will continue)
Unless you rename it like I suggested above, then I think it's ok as is

src/core/interpreter.cc

chakaz · 2024-12-02T07:10:58Z

And indeed, very nice catch!

Signed-off-by: adi_holden <[email protected]>

romange · 2024-12-02T12:26:48Z

src/core/interpreter.cc

@@ -993,7 +969,11 @@ int Interpreter::RedisGenericCommand(bool raise_error, bool async, ObjectExplore
  /* Pop all arguments from the stack, we do not need them anymore
   * and this way we guaranty we will have room on the stack for the result. */
  lua_pop(lua_, argc);
+  return std::make_optional(args);


args reference name_buffer, which is on stack 🤷🏼

romange · 2024-12-02T12:46:31Z

src/core/interpreter.cc


+std::optional<int> Interpreter::CallRedisFunction(bool* raise_error, bool async,


I think the interface here is too much. It has optional that wraps int that based on the code can return only 0 and 1, and we have raise_error that is another output argument?

CallRedisFunction should return number of arguments it put on stack. Does CallRedisFunction ever return 0? Is lua stack really on 0 when it happens ?

raise_error seems to be relevant only if CallRedisFunction returns nullopt.

Maybe introduce CallResult = variant<int, bool> so if the first argument is defined then it's number of results and the second is raise_error?

simplified the flow

romange · 2024-12-02T12:47:57Z

src/core/interpreter.cc

@@ -1017,9 +997,9 @@ int Interpreter::RedisGenericCommand(bool raise_error, bool async, ObjectExplore
    return 0;

  // Raise error for regular 'call' command if needed.
-  if (raise_error && translator->HasError()) {
+  if (*raise_error && translator->HasError()) {


what does it mean

if (!translator) return 0;

above ?

I actually dont know.. this is not code that I changed

I see we use it inside DragonflyHashCommand, were we define a custom explorer StringCollectorTranslator
but why do we return here 0 I dont know

I changed it to return 1 and it looks like everything works

Signed-off-by: adi_holden <[email protected]>

chakaz · 2024-12-03T10:20:19Z

src/core/interpreter.cc


+// Calls redis function
+// return true if error needs to be raised in case api returns error.


It's kinda backwards to return true if there's an error, and false on success, don't you think?

I flipped it though I do find it convenient to return true if we need to raise an error

chakaz · 2024-12-03T10:26:01Z

src/core/interpreter.cc

+    return 1;
+  }
+
+  // IMPORTANT! all allocations withing this funciton must be freed


Suggested change

// IMPORTANT! all allocations withing this funciton must be freed

// IMPORTANT! all allocations within this funciton must be freed

chakaz · 2024-12-03T10:30:10Z

src/core/interpreter.cc

+    std::optional<absl::FixedArray<std::string_view, 4>> args = PrepareArgs();
+    if (args.has_value()) {
+      raise_error = CallRedisFunction(raise_error, async, explorer, SliceSpan{*args});
+    }


do we need an else here to force raise_error to be true?

so the idea is that raise error is a param to function RedisGenericCommand
and if we need to raise error in case of an error it will be true when this function is called
the call to CallRedisFunction will overide this param if the invocation of the command was successful

I guess I don't have the full context here..
What's the purpose of PushError() without calling RaiseErrorAndAbort()? Where is this error later read?

when calling RaiseErrorAndAbort the script execution will abort
when pushing and error and not calling RaiseErrorAndAbort the script will not abort, so the error will be returned for the executed command and the script writer can decide how to handle this error

Signed-off-by: adi_holden <[email protected]>

romange

LGTM

fix: memleak

5616815

Signed-off-by: adi_holden <[email protected]>

adiholden requested review from romange and chakaz December 1, 2024 20:36

romange reviewed Dec 1, 2024

View reviewed changes

src/core/interpreter.cc Outdated Show resolved Hide resolved

chakaz reviewed Dec 2, 2024

View reviewed changes

fix: cr

c9c0e38

Signed-off-by: adi_holden <[email protected]>

adiholden requested review from chakaz and romange December 2, 2024 12:17

romange reviewed Dec 2, 2024

View reviewed changes

adiholden added 3 commits December 2, 2024 15:09

fix: name buffer

dfe8966

Signed-off-by: adi_holden <[email protected]>

fix dcheck

7402ec9

Signed-off-by: adi_holden <[email protected]>

simplify flow

7f43aca

Signed-off-by: adi_holden <[email protected]>

chakaz reviewed Dec 3, 2024

View reviewed changes

add pytest

ba4adbd

Signed-off-by: adi_holden <[email protected]>

romange reviewed Dec 3, 2024

View reviewed changes

romange approved these changes Dec 3, 2024

View reviewed changes

adiholden merged commit 7a23ec2 into main Dec 3, 2024
9 checks passed

adiholden deleted the fix_memory_leak branch December 3, 2024 14:47


		std::optional<int> Interpreter::CallRedisFunction(bool* raise_error, bool async,


		// Calls redis function
		// return true if error needs to be raised in case api returns error.

	// IMPORTANT! all allocations withing this funciton must be freed
	// IMPORTANT! all allocations within this funciton must be freed

fix(server): fix memory leak on lua error #4236

fix(server): fix memory leak on lua error #4236

Uh oh!

Conversation

adiholden commented Dec 1, 2024

Uh oh!

romange commented Dec 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chakaz commented Dec 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romange left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!