Project

General

Profile

Actions
Copy link

Bug #31478

closed

wrong array check leads leads to segmentation fault in PHP

Added by Tim Wentzlau about 13 years ago. Updated almost 10 years ago.

Status:
Rejected
Priority:
Must have
Assignee:
-
Category:
-
Target version:
-
Start date:
2011-11-01
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
4.6
PHP Version:
5.3
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

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

Download all files
bug_31478.diff (1 KB) bug_31478.diff Tim Wentzlau, 2011-11-02 10:58
bug_31478.diff (1.38 KB) bug_31478.diff Tim Wentzlau, 2011-11-02 11:16
Actions
Copy link
 #1

Updated by Markus Klein about 13 years ago

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.

Actions
Copy link
 #2

Updated by Francois Suter about 13 years ago

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.

Actions
Copy link
 #3

Updated by Tim Wentzlau about 13 years ago

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];
Actions
Copy link
 #4

Updated by Francois Suter about 13 years ago

Yes, that sounds good. Would you care to submit a patch?

Actions
Copy link
 #5

Updated by Tim Wentzlau about 13 years ago

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.

Actions
Copy link
 #6

Updated by Francois Suter about 13 years ago

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).

Actions
Copy link
 #7

Updated by Tim Wentzlau about 13 years ago

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.

Actions
Copy link
 #8

Updated by Tim Wentzlau about 13 years ago

OK, appended the wrong diff, discovered it after trying to apply.

Actions
Copy link
 #9

Updated by Gerrit Code Review over 11 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/22077

Actions
Copy link
 #10

Updated by Gerrit Code Review almost 10 years ago

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

Actions
Copy link
 #11

Updated by Gerrit Code Review almost 10 years ago

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

Actions
Copy link
 #12

Updated by Steffen Müller almost 10 years ago

  • Is Regression set to No

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" 
}
Actions
Copy link
 #13

Updated by Steffen Müller almost 10 years ago

  • Status changed from Under Review to Rejected

$wrapArr will always have two values if a delimiter is present. Therefore no patch is needed anymore.

Actions
Copy link

Also available in: Atom PDF