Fix use-after-free in error message construction #217
Conversation
Calling ERR_clear_error() releases the buffers that hold the path/filename, so we need to push those to Lua (which will copy them) before they are released. The problem can be verified with valgrind, or indeed just by running it on my machine which shows random memory contents prefixed to the error message string.
| } else { | ||
| return lua_pushfstring(L, "%s:%d:%s", file, line, txt); | ||
| lua_pushfstring(L, "%s:%d:%s", file, line, txt); |
There was a problem hiding this comment.
lua_pushfstring can fail (i.e. longjmp out). We need to clear the OpenSSL error before calling it.
There was a problem hiding this comment.
I'd rather take a potential longjmp (due to memory allocation failure) over consistently using released memory (the current situation).
Nevertheless, I'm happy to amend the patch. But what do you suggest? Allocating a temporary buffer to hold the string also has the potential to fail if lua_pushfstring() may fail. Then what?
I haven't dug deeply into the OpenSSL error handling API docs, but as far as I can tell, if we don't clear the error it just remains on the stack, it doesn't actually "leak".
There was a problem hiding this comment.
We already have the temporary (stack-allocated) buffer txt. ERR_error_string_n copies into that.
So the patch really only needs to swap the order of ERR_clear_error and ERR_error_string_n
Calling
ERR_clear_error()releases the buffers that hold the path/filename, so we need to push those to Lua (which will copy them) before they are released.The problem can be verified with valgrind, or indeed just by running it on my machine which shows random memory contents prefixed to the error message string.