Bug #69864 [Ver->Csd]: 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 +Status: Closed 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: Automatic comment on behalf of cmb Revision: http://git.php.net/?p=php-src.git;a=commit;h=a39beaa2514514892d34bc2e9fd89f7333a6ed1f Log: Fixed bug #69864 (Segfault in preg_replace_callback) Previous Comments: ------------------------------------------------------------------------ [2015-06-23 07:11:01] ab@php.net Ok, lessons learned - when debugging something involving PCRE JIT issues, always add --smc-check=all to valgrind. JIT is a self modifying code which valgrind has to be told about explicitly. Grateful thanks Zoltán Herczeg for supporting us with investigations on this issue. Christoph, looks like we can apply the patch fixing the actual issue reported :) Thanks. ------------------------------------------------------------------------ [2015-06-22 14:24:57] cmb@php.net Indeed, it might be a libpcre issue, but I also was not able to reproduce the behavior with a pure C program. Increasing the cache size as a temporary fix might not be necessary. I found that it seems to be sufficient to only keep the pcre_extra data until shutdown; the compiled regex, the tables and strings could be released, what would save some memory. So it might be an option to store the pcre_extra data in a second cache (a fixed sized array might be sufficient) instead of calling pcre_free_study() in php_free_pcre_cache(), and to free the members of the array in the shutdown function of the extension. Another option might be to make the cache size an ini setting, so users can adjust it if necessary. ------------------------------------------------------------------------ [2015-06-22 13:45:46] ab@php.net Hi Christoph, thanks for the info. Yeah, debugged on this almost the whole Sunday, but no good result. I can confirm your insights, also I can add that i've tried several programs in pure C which didn't reproduce the crash. But what I saw also, that some pointer addresses are reused, so it could be something in PCRE which doesn't free/release properly. While this doesn't affect the normal work (usage of so many patterns at once), this needs to be fixed. I continue on this, maybe a temporary fix could be increasing the cache size - that would make the whole at least a bit more robust. Thanks. ------------------------------------------------------------------------ [2015-06-22 12:03:13] cmb@php.net My further findings regarding the invalid read (w/o the pcre-refcount patch): * all entries in the cache have been studied[1] (if that failed extra==NULL, so pcre_free_study won't be called) * it occurs only if entries are removed from the cache before shutdown[2] * if the cache is circumvented (by constructing a zval with the pce instead of calling zend_hash_update_mem[3] and freeing that zval at the end of php_pcre_replace[4]) the invalid reads may still happen * the valgrind backtrace is clear on where the respective memory block has been freed, but it doesn't show where the invalid read happens. Interestingly, frame #3 seems to be allocated on the stack. However, I'm not able to find what is causing the invalid read. :( [1] <https://github.com/php/php-src/blob/php-7.0.0alpha1/ext/pcre/php_pcre.c#L426> [2] <https://github.com/php/php-src/blob/php-7.0.0alpha1/ext/pcre/php_pcre.c#L448> [3] <https://github.com/php/php-src/blob/php-7.0.0alpha1/ext/pcre/php_pcre.c#L492> [4] <https://github.com/php/php-src/blob/php-7.0.0alpha1/ext/pcre/php_pcre.c#L1013> ------------------------------------------------------------------------ [2015-06-20 15:46:25] ab@php.net Hi Christoph, great you stay on this. To me it's currently not clear whether it's not the same thing, so I would stay in the scope of this ticket therefore. But feel free to open another one, if you're sure. What I've discovered yet - pcre_free_study() is yeah, JIT related. Furthermore - regarding to the man page, it requires a pointer produced by pcre_study(). It's not always the case currently. When relying on this information, maybe some more could come out. I'd suggest just to investigate further till we come to the clear understanding about what's going on. Thanks. ------------------------------------------------------------------------ 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