EZC: fix crash due to EG(exception) being transmitted between requests#3121
EZC: fix crash due to EG(exception) being transmitted between requests#3121tstarling wants to merge 1 commit intofacebook:masterfrom
Conversation
|
Amended to fix shutdown order issue. |
|
Thanks, this is up for review. FB: D1422515 |
There was a problem hiding this comment.
This is pretty sketchy. I don't see any docs on RequestEventHandler indicating a range for the priority values or what certain numbers mean. It looks like this is needed for correctness; how do we know this will be low enough?
There was a problem hiding this comment.
I couldn't find anything else that used this priority value, I don't know what it was introduced for. Assuming nothing else uses it, it just has to be larger than zero. I chose 10 because it's the smallest BASIC line number greater than zero. I introduced priority in my dev branch in tstarling@ba5cbeb to fix a crash.
The crash involved early destruction of ZendObjectStore::tl_instance.m_node.m_p, then recreation of the same later in the shutdown sequence due to a call to ZendObjectStore::tl_instance.get() from another shutdown handler, resulting in an assertion in ExecutionContext::registerRequestEventHandler() due to duplicate event handler registration. Note that ExecutionContext::onRequestShutdown() doesn't remove handlers after they are called, it only clears the container after they have all been called.
An alternative would be to not use RequestLocal for ZendObjectStore, and instead delete it explicitly later in hphp_context_exit().
There was a problem hiding this comment.
Yeah if the timing is really that sensitive I'd feel a lot better being explicit about where it gets deleted.
Although priority() doesn't seem generally useful unless there's some kind of discipline about how we assign the values to extensions using it. Maybe you could leave this as is and add a comment to its declaration in RequestEventHandler with a list of which extensions override priority and why? (just containing ZendObjectStore for now).
There was a problem hiding this comment.
I don't see why this is necessary. Shouldn't it have been cleared out by requestShutdown() in the previous request on this thread?
There was a problem hiding this comment.
Sure, assuming no other shutdown handler sets it again after it is cleared. I don't think there's much overhead -- the virtual call will be done even if we don't implement a requestInit() handler here. Maybe m_data.prev_exception should be wiped as well, to be on the safe side.
There was a problem hiding this comment.
Sounds good. Maybe add dbg-only asserts that both are nullptr before clearing them to catch this if it happens.
|
Leave the incldues as-is for now. Can you update this to add the asserts? |
|
Added the asserts and fixed the namespace indent. I haven't fixed the priority issue yet. |
|
From @markw65: From reading the summary, it seemed that requestInit was the place you were fixing the bug. But it looks like its really being fixed in requestShutdown, and requestInit is just there to verify that the bug was fixed, or to gloss over the bug if it happens in a production build... |
|
This pull request has been imported into Phabricator, and discussion and review of the diff will take place at https://reviews.facebook.net/D19953 |
There was a problem hiding this comment.
Either its an error to throw an exception after requestShutdown, or its not. Masking the error by assigning nullptr in production builds doesn't seem to be a good idea.
There was a problem hiding this comment.
Maybe it can leak, to the same extent that any circular reference can leak. References to non-smart-allocated data might not freed, if sweep() is not completely implemented for some reason. I don't think it is really an error.
|
The bug was the use of a pointer which pointed to the pool of a previous request -- this inevitably causes a crash. This is fixed by setting EG(exception) to nullptr in requestInit(), which is what Zend does to fix this same issue. The requestShutdown() hook releases the reference, thereby making it slightly more likely that memory won't be leaked. This is not necessary to fix the bug, I just added it for good measure. There's various ways to preserve a non-zero reference count beyond the end of a request, such as with a circular reference, but I thought it would be good to remove one such reference leak while I happened to be working on the same area of the code. |
|
@tstarling can we move to https://reviews.facebook.net/D19953 please |
When an exception was thrown from EZC, EG(exception) was left holding a reference to the smart-allocated exception object, even after the end of the request. At the end of the request, the object storage was freed by resetAllocator(), then when zend_wrap_func() was entered in the next request, EG(exception) would be freed again, causing a crash. Fixed by installing request event handlers. The requestInit() handler is analogous to Zend's init_executor(). Defer ZendObjectStore::requestShutdown() until after EG(exception) is deleted, to avoid an assertion from ExecutionContext::registerRequestEventHandler due to duplicate registration.
|
Removed asserts again per Phabricator discussion, move exception to a temporary variable before deleting per @markw65's request, rebased. |
6 participants
When an exception was thrown from EZC, EG(exception) was left holding a
reference to the smart-allocated exception object, even after the end of
the request. At the end of the request, the object storage was freed by
resetAllocator(), then when zend_wrap_func() was entered in the next
request, EG(exception) would be freed again, causing a crash.
Fixed by installing request event handlers. The requestInit() handler is
analogous to Zend's init_executor().