Bug #69864 [Ver]: Segfault in preg_replace_callback

Edit report at https://bugs.php.net/bug.php?id=69864&edit=1 ID: 69864 Updated by: cmb@php.net Reported by: james dot h dot cracknell at gmail dot com Summary: Segfault in preg_replace_callback Status: Verified Type: Bug Package: PCRE related Operating System: Windows Server 2008 R2 PHP Version: 7.0.0alpha1 Block user comment: N Private report: N New Comment: I still have not been able to get a crash; I've tested with several 7.0.0alpha1 binaries and own VC 14 x64 builds. The tests always exited cleanly, albeit with wrong results (either NULL or "ba" instead of "bb"). Anyhow, I've attached an improved patch, including initialization of the .refcount member, and additional refcounting for all paths where the PCE might have to be prevented from being garbage collected. The patch is against master; for PHP 5.6 most notably the hack to get the PCE at the beginning of pcre_clean_cache() has to be changed. This raises a somewhat related issue wrt. php_free_pcre_cache(). Line 92[1] does basically the same, but assumes that data is a zval*. However, it is supposed to be a pcre_cache_entry**[2]. That is currently not a real issue, as .value is the first member of struct _zval_struct (and zend_value is a union) but if the layout will change, Z_PTR_P(data) would return garbage. [1] <https://github.com/php/php-src/blob/8c8ad8f40ed9af2d95057a078dbaa844d072cb68/ext/pcre/php_pcre.c#L92> [2] <https://github.com/php/php-src/blob/8c8ad8f40ed9af2d95057a078dbaa844d072cb68/ext/pcre/php_pcre.c#L492> Previous Comments: ------------------------------------------------------------------------ [2015-06-18 12:49:36] cmb@php.net The following patch has been added/updated: Patch Name: pcre-refcount Revision: 1434631776 URL: https://bugs.php.net/patch-display.php?bug=69864&patch=pcre-refcount&revision=1434631776 ------------------------------------------------------------------------ [2015-06-18 09:17:02] ab@php.net Christoph, imho the idea of your patch is good, however it looks some more complex. Currently the .refcount member is not initialized, so it's most likely not zero. This results it that entries would be never cleanup. To init it it's around the line 455 (see the new_entry var). When it's initialized, it's probably about going through the usages and see where else it has to be incremented (like the same pattern could be already compiled in some match usage, etc.). Does it sound feasible? Btw I do get this crash and on Windows, same bt. Do you still use vc11? Thanks. ------------------------------------------------------------------------ [2015-06-17 22:54:15] james dot h dot cracknell at gmail dot com The patch looks as though it should do the trick. > I'd rather fix the issue, if feasible, than to tag it as WONTFIX. Well it's definitely a bug; as it stands once you are in preg_replace_callback, the PCRE functions are a timebomb; e.g: <https://github.com/WordPress/WordPress/blob/f62bf61b2c59bf5484888f22d0b7dff0f0df050a/wp-includes/shortcodes.php#L200> ------------------------------------------------------------------------ [2015-06-17 22:03:56] cmb@php.net I'm still haven't been able to reproduce a segfault, but indeed the results of the script are wrong. And yes, it is as you suspected, James. The pcre_cache_entry is deleted even though it's used later on. Interestingly, pcre_cache_entry has a member refcount which doesn't seem to be used (at least in master). This member might be used to prevent entries that will still be needed later to be deleted. See the attached patch "quick-hack" for a draft. > That sounds like more than "very extensive use" that sounds like > "insane use". I agree. However, I'd rather fix the issue, if feasible, than to tag it as WONTFIX. :) ------------------------------------------------------------------------ [2015-06-17 22:03:11] cmb@php.net The following patch has been added/updated: Patch Name: quick-hack Revision: 1434578591 URL: https://bugs.php.net/patch-display.php?bug=69864&patch=quick-hack&revision=1434578591 ------------------------------------------------------------------------ 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=69864 -- Edit this bug report at https://bugs.php.net/bug.php?id=69864&edit=1