Add assertion that php_pcre_replace_impl() returns a valid pointer in SPL RegexIterator() #10520
Conversation
|
I can run my analysers tonight and see what they report. However, doesn't cmb's testcase (#8271 (comment)) trigger this :)? If so, why does that one not suffice? It at least triggers this on my system's PHP install. |
how did I miss that, somehow I'm blind... I'll go an amend this then. |
|
Ran analysis tools, they said nothing interesting about this function (or its callees) other than what we already know: the return value of |
Girgias
force-pushed
the
pr-8271-follow-up
branch
3 times, most recently
from
7f6a1e4 to
6ad4881
Compare
|
So I can reproduce the leak, Opcache needs to be disabled, but I can't figure out the issue. There seems to be a bug with generators and using @bwoebi do you maybe have an idea? |
| zend_string_release(replacement_str); | ||
|
|
||
| if (!result) { | ||
| RETURN_FALSE; |
There was a problem hiding this comment.
You probably have to release subject as well.
Girgias
force-pushed
the
pr-8271-follow-up
branch
from
6ad4881 to
9f7a4bc
Compare
php_pcre_replace_impl() will return NULL on failure. However, in the implementation of RegexIterator::accept() the return value of php_pcre_replace_impl() is directly used without any check, which leads to a NULL pointer dereference. Fix this by adding a NULL check, and returning false in that case. Closes phpGH-8271 Signed-off-by: George Peter Banyard <[email protected]>
Girgias
force-pushed
the
pr-8271-follow-up
branch
from
9f7a4bc to
8e258a1
Compare
| @@ -1866,6 +1866,12 @@ PHP_METHOD(RegexIterator, accept) | |||
| } | |||
|
|
|||
| result = php_pcre_replace_impl(intern->u.regex.pce, subject, ZSTR_VAL(subject), ZSTR_LEN(subject), replacement_str, -1, &count); | |||
| zend_string_release(replacement_str); | |||
| zend_string_release(subject); | |||
There was a problem hiding this comment.
Seems like this PR was forgotten, but you cannot release subject here as it would cause a double free at the end of the function. This should only be released in the !result block.
Please test this with a non-interned subject string.
|
Actually nevermind, looks like this was already fixed by b3a56bd and I forgot about it. |
The regex is precompiled during the constructions of the object at line 1424 in spl_iterators.c Moreover, the warnings emitted by pcre_get_compiled_regex_cache() get promoted to InvalidArgumentExceptions.
php_pcre_replace_impl() will only return NULL in case of an internal PCRE error, something which should not happen.
Closes GH-8271
I tried coming up with a test case to trigger this, but it seems impossible? @nielsdos could you have a look at this? I imagine static analysis is going to complain but wouldn't be able to come up with a test case for it... maybe creating a fuzzer for ext-pcre and the RegexIterator might be a good idea.