philanc / luatweetnacl

Lua wrapper arount the Tweet NaCl cryptographic library
MIT License
17 stars 5 forks source link

Memory leaks when close to OOM #3

Closed daurnimator closed 7 years ago

daurnimator commented 7 years ago

Lua functions such as lua_pushlstring can long jump out on failure. Your code doesn't protect against this, and will end up leaking your mallocd buffer e.g. https://github.com/philanc/luatweetnacl/blob/master/luatweetnacl.c#L323

Consider using lua_newuserdata instead of malloc so that you get free/automatic clean up!

philanc commented 7 years ago

Thanks. I will look into it.

philanc commented 7 years ago

If I understand correctly (I am a bit naive with the C interface, especially userdata!), what you suggest is to replace this sort of sequence using malloc(): char * buf = malloc(buflen); some_C_func(buf, buflen, x, y, z); lua_pushlstring (L, buf, buflen); free(buf); return 1;

With this: char * buf = lua_newuserdata(L, buflen); some_C_func(buf, buflen, x, y, z); lua_pushlstring (L, buf, buflen); lua_remove(L, -2); // must remove the userdata from the stack // let the Lua GC get rid of the allocated userdata later return 1;

is it correct?

daurnimator commented 7 years ago

Yes, that would work. No need to call lua_remove either.

daurnimator commented 7 years ago

A different approach is to use a luaL_Buffer with luaL_buffinitsize. However that only works in 5.2+ (though you can get it via compat-5.3)

philanc commented 7 years ago

but if I don't call lua_remove, the userdata I pushed is not popped and the stack keeps growing...!? or maybe I just don't get how the stack works?

daurnimator commented 7 years ago

Lua will automatically clean up the stack after your c function returns.

http://www.lua.org/manual/5.3/manual.html#lua_CFunction

To return values to Lua, a C function just pushes them onto the stack, in direct order (the first result is pushed first), and returns the number of results. Any other value in the stack below the results will be properly discarded by Lua.

philanc commented 7 years ago

Ah, ok! I get it. Thanks for the explanation.

philanc commented 7 years ago

Ok. I did replace all the malloc/free with lua_newuserdata. Thanks for the suggestion.