Bug #71659 [Opn]: segmentation fault in pcre running twig tests

Edit report at https://bugs.php.net/bug.php?id=71659&edit=1 ID: 71659 User updated by: nish dot aravamudan at canonical dot com Reported by: nish dot aravamudan at canonical dot com Summary: segmentation fault in pcre running twig tests Status: Open Type: Bug Package: Reproducible crash Operating System: Ubuntu 16.04 PHP Version: 7.0.3 Block user comment: N Private report: N New Comment: I believe your understanding to be correct :) Also, your comments are alluded to by the pcre developer who helped me debug this: https://bugs.exim.org/show_bug.cgi?id=1803#c44 Previous Comments: ------------------------------------------------------------------------ [2016-03-09 21:44:24] nikic@php.net To clarify that I understand the issue correctly: What's happening here is that the regex /./us is already used (from the userland side) and we'll set the MARK flag and mark pointer in pcre_extra. This will then be cached. preg_split() then reuses that compiled regex from cache without clearing the MARK bit, so it will be using a dangling mark pointer. If that understand is correct your fix looks reasonable. However I'm not sure why we have that code there at all. That seems like a very, very terrible way to advance one UTF-8 character. In other places we use http://lxr.php.net/xref/PHP_MASTER/ext/pcre/php_pcre.c#251, which should be a good sight more efficient. I'll check if we can just replace it altogether or if this is working around some weird issue. ------------------------------------------------------------------------ [2016-03-09 18:29:14] nish dot aravamudan at canonical dot com Ok, so I believe the right change is probably to just unset the PCRE_EXTRA_MARK bit in the flags of extra_bump. I just tested my previous patch (which also unset mark itself) and can confirm that the twig testsuite passes now, but I think the smaller change (to just modify extra_bump) will be functionally equivalent. Is a GitHub PR the appropriate way to suggest the patch get included? ------------------------------------------------------------------------ [2016-03-08 23:49:55] nish dot aravamudan at canonical dot com I was probably overconfident in my ability to understand the PHP code :) But now I think the correct fix is: Index: gitwd/ext/pcre/php_pcre.c =================================================================== --- gitwd.orig/ext/pcre/php_pcre.c +++ gitwd/ext/pcre/php_pcre.c @@ -1848,6 +1848,10 @@ PHPAPI void php_pcre_split_impl(pcre_cac RETURN_FALSE; } } +#ifdef PCRE_EXTRA_MARK + extra_bump->mark = NULL; + extra_bump->flags &= ~PCRE_EXTRA_MARK; +#endif count = pcre_exec(re_bump, extra_bump, subject, subject_len, start_offset, exoptions, offsets, size_offsets); ------------------------------------------------------------------------ [2016-03-08 23:00:40] nish dot aravamudan at canonical dot com If I understand this right, that should rather be: + extra->mark = NULL; ------------------------------------------------------------------------ [2016-03-08 22:48:35] nish dot aravamudan at canonical dot com I'm going to test a new version of PHP7.0 that has a small adjustment to php_ --- php7.0-7.0.3.orig/ext/pcre/php_pcre.c +++ php7.0-7.0.3/ext/pcre/php_pcre.c @@ -1761,6 +1761,7 @@ PHPAPI void php_pcre_split_impl(pcre_cac extra->match_limit = (unsigned long)PCRE_G(backtrack_limit); extra->match_limit_recursion = (unsigned long)PCRE_G(recursion_limit); #ifdef PCRE_EXTRA_MARK + extra->mark = &mark; extra->flags &= ~PCRE_EXTRA_MARK; #endif Commit https://github.com/php/php-src/commit/376ab3b7873ca04142185d8c08dbb4c4be152474 (and presumably others based upon the current state of the code) modified the other functions to avoid ->mark corruption. I don't know why this only shows up with JIT, but perhaps the ->mark value is not clobbered except if JIT is used. ------------------------------------------------------------------------ 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=71659 -- Edit this bug report at https://bugs.php.net/bug.php?id=71659&edit=1