Edit report at https://bugs.php.net/bug.php?id=71659&edit=1
ID: 71659
Updated by: nikic@php.net
Reported by: nish dot aravamudan at canonical dot com
Summary: segmentation fault in pcre running twig tests
Status: Closed
Type: Bug
Package: Reproducible crash
Operating System: Ubuntu 16.04
PHP Version: 7.0.3
-Assigned To:
+Assigned To: nikic
Block user comment: N
Private report: N
New Comment:
As it did not break any tests, I've replaced the bump regex with use of
calculate_unit_length(). I've verified that this does fix the segfault in the Twig testsuite.
I think there's still one potential issue left (though very unlikely): Some of the pcre_exec
uses are in loops where MARK is set outside the loop. The loop could potentially trigger a "too
many substrings" condition and the error handler could recursively invoke pcre_exec with a
different mark value. We should probably move the code into the pcre_exec loop just to be sure.
In any case, thanks a lot to you and Zoltan Herczeg both for investigating this tricky issue!
Previous Comments:
------------------------------------------------------------------------
[2016-03-09 22:03:32] nikic@php.net
Automatic comment on behalf of nikic
Revision: http://git.php.net/?p=php-src.git;a=commit;h=5a6da79fd0bd88997b3679578c7702bc74b3f61a
Log: Fix bug #71659
------------------------------------------------------------------------
[2016-03-09 21:59:24] nish dot aravamudan at canonical dot com
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
------------------------------------------------------------------------
[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);
------------------------------------------------------------------------
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