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 Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ [2015-06-20 00:26:22] cmb@php.net Thanks for further investigation, Anatol, and the valuable hints. Indeed, the valgrind issues are not (directly) related to the patch. The following test script raises lots of these for current master (w/o the patch): <?php for ($i = 0; $i < 10000; $i++) { preg_replace('/foo'.$i.'bar/', 'baz', '???foo'.$i.'bar???'); } ?> All this depends on pcre.jit. If this (php.ini) configuration option is disabled, valgrind reports no problems. Also James' test script runs fine with the patch if pcre.jit is disabled. It seems to me that the valgrind/pcre.jit issue is not directly related to this ticket, and that it might be best to open a new ticket for it. I think it would be preferable to solve that issue before finally tackling this ticket. Anyhow, disregard my comments regarding storing PCE vs. zval in the HashTable. I had not read the code carefully enough. ------------------------------------------------------------------------ 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