Memory leaks in Lua::redisGenericCommand

[PragmaTwice]

Search before asking

• [X] I had searched in the issues and found no similar issues.

Version

redisGenericCommand use raiseError to process errors, but raiseError will perform a longjmp in the lua_error api call, so raiseError never returns.

longjmp does not compatible with RAII in C++ (C++ exceptions may use longjmp in implementation but the compiler will insert some dtor call autometically and carefully, i.e. stack unwinding), so every object in the stack of redisGenericCommand will never be destroyed, i.e. no destructor will be call, hence all resource allocated in ctor like std::vector, std::unique_ptr etc. cannot be released.

@git-hulk

Minimal reproduce step

./runtest --dont-clean on a kvrocks binary with Address Sanitizer enabled. (try to build with #599) You can only run tests in unit/scripting to save time : )

What did you expect to see?

No memory leaks

What did you see instead?

Memory leaks are reported by Address Sanitizer

Anything Else?

No response

Are you willing to submit a PR?

• [X] I'm willing to submit a PR!

[git-hulk]

Cool, Thanks for Twice great investigation.

[git-hulk]

@PragmaTwice Really good catch, I wasn't aware the destructor would be ignored by longjump. Lua supported using try..catch to workaround the C++ condition: https://github.com/KvrocksLabs/lua/blob/3e2d94b3b98d774dcd4fb448b1cb64615ae0590d/src/luaconf.h#L606

[PragmaTwice]

@PragmaTwice Really good catch, I wasn't aware the destructor would be ignored by longjump. Lua supported using try..catch to workaround the C++ condition: https://github.com/KvrocksLabs/lua/blob/3e2d94b3b98d774dcd4fb448b1cb64615ae0590d/src/luaconf.h#L606

Yeah, I found two method to solve this problem:

1. build lua with a C++ compiler like CC=g++, then lua will use these try..catch stuff to implement lua_error or other functions, refer to "4.6 – Error Handling in C" in https://www.lua.org/manual/5.3/manual.html.
2. raise C++ exception manually, like in http://lua-users.org/wiki/ErrorHandlingBetweenLuaAndCplusplus or https://stackoverflow.com/questions/4615890/how-to-handle-c-exceptions-when-calling-functions-from-lua.

Which solution do you prefer?

[tisonkun]

@PragmaTwice it seems that the minimal solution is opinion 1 proposed above. If we'd like to raise C++ exception manually, both pushError and raiseError and their call points should be investigated and changed.

[git-hulk]

Yes, it will be good to use solution 1, since we don't need to prevent using those functions(which contains longjump) every time.

[PragmaTwice]

@tisonkun @git-hulk Agree with that. I'll try the first solution.

[tisonkun]

@PragmaTwice Go ahead!

[tisonkun]

With a closer look into this issue, I'm considering whether we can use a new version of Lua or LuaJIT to workaround this issue with C++ compiler. Hacking on a fork is possibly hard to maintain.

[git-hulk]

@tisonkun Yes, many guys proposed this in Redis community, but didn't have any progress. The main issue is that will break the old Lua scripts, coz Lua 5.2+ have many breaking changes compared with 5.1. That's the reason why we still stuck in 5.1.

[tisonkun]

@git-hulk @PragmaTwice Hmmm, I just notice that Redis uses a Lua 5.1 fork also. Is it possible that we pull Lua code directly into this repo and port them into c++ code? As a kvrocks bundle Lua engine, it needs not to be able to build with C compiler. It may be an approach we working around the compiling issues at #614.

[git-hulk]

@git-hulk @PragmaTwice Hmmm, I just notice that Redis uses a Lua 5.1 fork also. Is it possible that we pull Lua code directly into this repo and port them into c++ code? As a kvrocks bundle Lua engine, it needs not to be able to build with C compiler. It may be an approach we working around the compiling issues at #614.

@tisonkun Do you mean that we can pull the Lua codes from Redis repo and only patch some files we wanted? If yes, I'm didn't have experience by this way but I think it's a good way to keep consistency with Redis repo.

[PragmaTwice]

Maybe we can discuss it in a new issue since this issue is closed? @tisonkun

[tisonkun]

@PragmaTwice Yes. I only have an unclear idea yet so I'll create an issue if I get some time to prototype. If you're interested in this topic you can create an issue and go ahead :)

Since #614 is resolved this idea isn't a required work now.

[PragmaTwice]

@PragmaTwice Yes. I only have an unclear idea yet so I'll create an issue if I get some time to prototype. If you're interested in this topic you can create an issue and go ahead :)

Since #614 is resolved this idea isn't a required work now.

Thanks. Here are some of ideas from the conversation above and my comments:

Solution 1: "pull the Lua codes from Redis repo and only patch some files we wanted" mentioned by @git-hulk

If I understand correctly, this means that the redis code is fetched and patched during the build process. I am familiar with applying patch in cmake after FetchContent, but my concern is that we may have to fetch all files in redis to local since I do not know how to just fetch a subdir of a git repo. And I think it may confuse users that we depend on redis. And I feel that maintaining patch files is more troublesome than maintaining a git repo.

Solution 2: copy lua codes from Redis repo directly to a subdir of this repo and modify it

I think having lua fork as another repo might avoid some redundant commits and make the main repo cleaner.

[tisonkun]

@PragmaTwice thanks for your feedback. After some investigation I also agree that current status is good enough for running Kvrocks project. If we meet further concrete issue, we may review this approach then.