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: ab@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 Assigned To: cmb Block user comment: N Private report: N New Comment: I can see many errors like this with the new patch and the snippet @james posted earlier ==54386== Invalid read of size 1 ==54386== at 0x409ED6D: ??? ==54386== by 0xD801E77: ??? ==54386== by 0xFFEFF3D8F: ??? ==54386== Address 0xe699641 is 65 bytes inside a block of size 264 free'd ==54386== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==54386== by 0x4E14A2: free_read_only_data (pcre_jit_compile.c:2139) ==54386== by 0x502C4A: _pcre_jit_free (pcre_jit_compile.c:10532) ==54386== by 0x4D311E: pcre_free_study (pcre_study.c:1672) ==54386== by 0x503C79: php_free_pcre_cache (php_pcre.c:96) ==54386== by 0xA3DF4C: _zend_hash_del_el_ex (zend_hash.c:935) ==54386== by 0xA3E032: _zend_hash_del_el (zend_hash.c:959) ==54386== by 0xA3F4DD: zend_hash_apply_with_argument (zend_hash.c:1463) ==54386== by 0x504D30: pcre_get_compiled_regex_cache (php_pcre.c:450) ==54386== by 0x5073D7: php_pcre_replace (php_pcre.c:1028) ==54386== by 0x508322: php_replace_in_subject (php_pcre.c:1361) ==54386== by 0x508976: preg_replace_impl (php_pcre.c:1422) But in general - yep, in this case it's probably better to concentrate to fix master first, and then to backport into 5.6 when we see it stable in master. Not sure what's wrong with refcounts, probably better just to debug it. Thanks. Previous Comments: ------------------------------------------------------------------------ [2015-06-18 14:06:42] kalle@php.net Fix assignee (was lost after Rasmus' comment) ------------------------------------------------------------------------ [2015-06-18 12:50:43] cmb@php.net 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> ------------------------------------------------------------------------ [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> ------------------------------------------------------------------------ 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