Bug #31478
closed
wrong array check leads leads to segmentation fault in PHP
Added by Tim Wentzlau over 12 years ago.
Updated over 9 years ago.
Description
In classes.t3lib.parsehtml.php in line 250 there is a wrong check of the array size.
if (count($wrapArr) > 0) {
$marker = $wrapArr[0] . $marker . $wrapArr[1];
}
if count($wrapArr) returns 1 then php fails with segmentation fault when trying to access $wrapArr1, as there is no element there.
The segmentation fault must be a php bug, but can be circumvented with this fix:
if (count($wrapArr) > 1) {
$marker = $wrapArr[0] . $marker . $wrapArr[1];
}
Files
Hi Tim,
I agree this check should be vastly improved.
A few lines below $warpArr is used again, omitting this check completely.
I suggest to add a new check right after the trimExplode(), which throws an exception if $wrap is non-empty and count($wrapArr) < 2.
This safes us from checking this again and again.
I agree with the fix, but throwing an exception is not a good idea. If it's not caught anywhere it may break existing installations.
In order not to break existing installation you could do something like this
if (count($wrapArr) > 0)
$marker = $wrapArr[0] . $marker;
if (count($wrapArr)>1)
$marker.= $wrapArr[1];
Yes, that sounds good. Would you care to submit a patch?
I started to make the patch, by adding this later in the function
$pattern='';
if (count($wrapArr)>0)
$pattern.='/' . preg_quote($wrapArr[0]) . '([A-Z0-9_|\-]*)';
if (count($wrapArr)>1)
$pattern.=preg_quote($wrapArr[1]);
$pattern.= . '/is';
$content = preg_replace($pattern, '', $content);
But does it make any sense to use $pattern if $warp1 is missing. At least this should give a warning in a log, but I don't know how to post to the log in Typo3.
You raise a good point. The general assumption is that a marker will be of the form ###FOO###, which is what is generally used. What this method shows is that there could be other wrappers than ### and that they could even be asymmetric (with one even missing). The question is whether this was really meant to be so or is the result of lax testing.
As such I would propose the following: for stable versions of TYPO3 (i.e. 4.5 and 4.6) we could have a patch that just checks that $wrapArr has the necessary length and that we don't access non-existing indices in the array. For 4.7 we could have an improved patch which logs warnings and is maybe a bit stricter.
About logging we have to be careful because this class/method is used both in the FE and BE, so the logging would be different (I could imagine logging to the TS rendering in the FE, so that the warning would be visible in the admin panel, but the BE might just call devLog).
I have appended the diff, but it is to some degree untested as some part of this functions is never called in my typo3 install.
OK, appended the wrong diff, discovered it after trying to apply.
- Status changed from New to Under Review
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/22077
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/22077
Tried to reproduce and it seems explode() handles that for us.
<?php
$wraps = ['foo|bar', 'foo|', '|bar', '|', 'bar'];
foreach ($wraps as $wrap) {
var_dump(explode('|', $wrap));
}
/**
array(2) {
[0] =>
string(3) "foo"
[1] =>
string(3) "bar"
}
array(2) {
[0] =>
string(3) "foo"
[1] =>
string(0) ""
}
array(2) {
[0] =>
string(0) ""
[1] =>
string(3) "bar"
}
array(2) {
[0] =>
string(0) ""
[1] =>
string(0) ""
}
array(1) {
[0] =>
string(3) "bar"
}
- Status changed from Under Review to Rejected
$wrapArr will always have two values if a delimiter is present. Therefore no patch is needed anymore.
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by 
Updated by 
Updated by