-
Notifications
You must be signed in to change notification settings - Fork 40
Check ptr to string before use it #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
436d5d6
to
3c95752
Compare
http/lib.c
Outdated
} | ||
} | ||
|
||
return lua_tostring(L, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should leave the Lua stack top unchanged.
(And, yep, the same problem exists in the tarantool code.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood a root of problem and just added lua_pop after function call. Now this module contains complete copy of luaT_tolstring function from "module.h". It allows to preserve compatibility with older versions.
Also test was introduced.
Thanks for your review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: tarantool/tarantool@fa0f10c
I'm not sure about this for now, so not sent yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed strange behaviour (only on centos 7). Something was wrong with Lua stack, I can't just use lua_pop after luaL_addchar/add_string but after I changed lua_pop with lua_gettop + lua_remove tests were passed.
I think we could check your patch.
However, according Lua C API reference (https://pgl.yoyo.org/luai/i/lua_tolstring):
lua_tolstring returns a fully aligned pointer to a string inside the Lua state.
This string always has a zero ('\0') after its last character (as in C), but can contain other zeros in its body.
Because Lua has garbage collection, there is no guarantee that the pointer returned by lua_tolstring will be valid after the corresponding value is removed from the stack.
You can't just get a pointer to the top of string and then change a top.
If this string is garbage collected it will be a use-after-free:
const char *res = lua_tolstring(L, -1, len); // <--- valid pointer
lua_settop(L, top);
// -- collectgarbage ---
return res; // -- invalid pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! Thanks for the notice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good to me, except a couple of details above.
I would also ask to add a test if the module would be in my responsibility.
CC @Satbek.
fae3a44
to
99a1fab
Compare
99a1fab
to
16c761a
Compare
Closes #51