Bug #68486 [Com]: PHP SegFault zend_hash_find

Edit report at https://bugs.php.net/bug.php?id=68486&edit=1 ID: 68486 Comment by: gmoniker at gmail dot com Reported by: wattwood at tcstire dot com Summary: PHP SegFault zend_hash_find Status: Open Type: Bug Package: Apache2 related Operating System: Ubuntu 14.04.1 LTS PHP Version: 5.5.19 Block user comment: N Private report: N New Comment: This is some great work. One remark, the php_functions.c file of the apache2handler inside the virtual definition mentions that the ap_rflush is supposed to be a workaround for http://issues.apache.org/bugzilla/show_bug.cgi?id=17629. Fortunately reading its comments seems to indicate that the underlying problem was solved in Apache 2.2 around august 2010 with https://svn.apache.org/viewvc?view=revision&revision=988400. Previous Comments: ------------------------------------------------------------------------ [2015-03-16 14:11:00] ab@php.net Patrick, somehow the latest patch is broken, it won't apply to any of 5.5 through master. Also, is there any piece of code to be used for the tests? Thanks. ------------------------------------------------------------------------ [2015-03-15 10:17:28] php at bof dot de Reworked the patch another round, see updated gist: https://gist.github.com/bof/15173c7a11cb12a7b96f The pool cleanup logic is reworked, with the goal of never cleaning up the SG(server_context) of something other than the currently running + not yet complete request - regardless of when and how delayed a pool cleanup callback comes in. This I do by 1) adding a ctx->server_context pointer to the context structure, 2) registering ctx instead of &SG(server_context) with the pool cleanup callback, and finally 3) in php_server_context_cleanup() I check that *ctx->server_context == ctx, and only in that case I NULL it. I also rearranged the php_handler() config handling a bit, so that the xbithack-during-reentry case should now be correct, too; and there's some all-around cleanup. The debug output of the previous patch is gone (available on request). I tested this patch, lightly, with my testcases and our normal codebase, under both Apache 2.4.12, the openSUSE 13.1 Apache 2.4.6, and an older Apache 2.2 setup as I use it in normal production so far. No issues were found, the new logic seems to work the same everywhere. I also ran the apache 2.4.12 variant under valgrind, also without any issues flagged. THIS STILL NEEDS REVIEW from somebody who previously worked on the apache2handler code. But I think it's solid enough now, and I'll probably test it in production on monday ------------------------------------------------------------------------ [2015-03-14 10:34:07] php at bof dot de Here's a gist with a patch, against 5.6.7RC1, that appears to fix the issue for me, at least for the things I tested. https://gist.github.com/bof/15173c7a11cb12a7b96f (patch contains lots of extra debug output, none of the ap_log_error calls I added would be good for merging) I basically rip out the whole parent_req handling attempts in php_handler(), and make ctx->request_processed be the only indicator regarding reentrancy to the interpreter. In case such a reentrancy is attempted, I error out early in php_handler() before any manipulation to interpreter state is done. The PHP virtual() function (provided in php_functions.c) will now cleanly fail when used to "embed" another PHP script (the worst reentrancy), but it continues to work when embedding an URI handled by any other handler (the not_for_us case). While testing I had a final issue with the ap_rflush() call in virtual() resulting in pool destruction of the in-flight request, due to the apache 2.4 EOR thing gmoniker mentioned in the previous comment. If anybody is curious, drop me an email, I've got the backtrace saved. For now I have disabled that ap_rflush() call in virtual(), and I don't know what could break by that. Anyway, after some light testing with this gist's patch applied, my own codebase keeps working normally, and the double-echo test now works without crashes for any of these cases: - two normal PHP requests in a row - two PHP requests in a row, each using virtual to embed another PHP script (embedded script NOT run, PHP warning message in virtual() caller) - two PHP requests in a row, using virtual to embed a non-PHP thing (/robots.txt tested...) - works as it should. ------------------------------------------------------------------------ [2015-03-13 18:59:54] gmoniker at gmail dot com Bug hunting: I am trying to reopen this bug at the Apache httpd bugzilla: https://bz.apache.org/bugzilla/show_bug.cgi?id=56984 Thanks to the mention of php_server_context_cleanup() by *php at bof dot de* I decided to look at the source code of the php sapi module and the Apache/APR code which causes it to be called. The PHP sapi module uses the pool register/cleanup philosophy enabled by the APR module of Apache (@see http://www.apachetutor.org/dev/pools) and stores its SG(server_context) in the request pool, depending on it to be cleaned up at the end of the request, before the Apache worker feeds it a new request. In Apache 2.2 the code in modules/http/http_core.c is fairly straightforward: static int ap_process_http_connection(conn_rec *c) [...] while ((r = ap_read_request(c)) != NULL) { [...] if (r->status == HTTP_OK) ap_process_request(r); if (ap_extended_status) ap_increment_counts(c->sbh, r); if (c->keepalive != AP_CONN_KEEPALIVE || c->aborted) break; ap_update_child_status(c->sbh, SERVER_BUSY_KEEPALIVE, r); apr_pool_destroy(r->pool); [...] } So before handling the next request the pool is destroyed and ergo the php_server_context_cleanup() gets called because it is registered to the pools cleanup list. In Apache 2.4 things have been rewritten quite extensively and in a way that is completely breaking the primary functionality of the APR pools, in that they should provide an easy declarative cleanup where a client registers a cleanup and can count on the server to execute it at the moment the server sees the pool should be destroyed. Apache 2.4.7 code (and 2.4.12):: static int ap_process_http_sync_connection(conn_rec *c) [...] while ((r = ap_read_request(c)) != NULL) { c->keepalive = AP_CONN_UNKNOWN; /* process the request if it was read without error */ ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r); if (r->status == HTTP_OK) { if (cs) cs->state = CONN_STATE_HANDLER; ap_process_request(r); /* After the call to ap_process_request, the * request pool will have been deleted. We set * r=NULL here to ensure that any dereference * of r that might be added later in this function * will result in a segfault immediately instead * of nondeterministic failures later. */ r = NULL; } [...] } So, there clearly is no pool cleanup anymore in this place. But surely, you say, it has just been moved to a place deeper in the stack? Following the ap_process_request, ap_process_async_request chain, however crushes this hope slowly but surely. There is still an attempt to let the pool be cleaned up after the request. After the handler (for example PHP) had its go at the request, the method ap_process_request_after_handler(r) is called. (it's in modules/http/http_request.c) The after_handler routine creates an EOR bucket and puts it in the output of the request connection. This bucket registers a eor_bucket_cleanup function to the request pool pre_cleanup list that would instruct the destruction of the request pool. However this routine can only be called by apr_pool_clear of apr_pool_destroy and the problem is exactly that as long as the connection with requests or responses doesn't end, then those methods are not called for this pool or a parent pool that might exist, at least not anywhere in the prefork worker that I could see. So it seems that in Apache 2.4 at present the request pool has become a connection pool after all. Apparently the problem is well masked by PHP opcaching, which has practically become the default. When the script gets cached it apparently doesn't matter that the server_context is reused (because it doesn't have to use the Zend_cache?). And also in many cases there might be multiple requests on one pipeline but just one of them would be a php script. This could be a major problem for the PHP5 sapi module on current Apache 2.4 servers. ------------------------------------------------------------------------ [2015-03-13 17:48:33] php at bof dot de For completeness, here's a gist with the patch against PHP 5.6.7RC1 that I created the second trace output in the previous comment with: https://gist.github.com/anonymous/449a2726c876d187165b In addition to the added logging, it contains the "1 ||" change at the start of php_handler() which makes the issue go away for me in the normal, non-subrequest cases. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=68486 -- Edit this bug report at https://bugs.php.net/bug.php?id=68486&edit=1