Make WordPress Core

Opened 3 months ago

Closed 6 weeks ago

#59927 closed defect (bug) (duplicate)

Missing error check in convert_smilies() can cause PHP Fatal error

Reported by: ov3rfly's profile Ov3rfly Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Formatting Keywords:
Focuses: Cc:

Description

A client website caused PHP Fatal error, tracked it down and came to this:

PHP Fatal error:  Uncaught TypeError: count(): Argument #1 ($value) must be of type Countable|array, bool given in .. wp-includes/formatting.php:3507

Which is this code:

// HTML loop taken from texturize function, could possible be consolidated.
$textarr = preg_split( '/(<.*>)/U', $text, -1, PREG_SPLIT_DELIM_CAPTURE ); // Capture the tags as well as in between.
$stop    = count( $textarr ); // Loop stuff.

The error occurs if preg_split() returns false and that is used as argument for count().

Since false is a valid return value here as described in PHP documentation, there should be a check for this case.

The problem is somehow related to size of $text where PHP interally obviously can't handle the data (any more).

Attached exported simplified WXR testcase. If you shorten the XXX part, the error might not appear any more.

Note: This kind of problem caused a PHP Warning with PHP 7.x but causes a PHP Fatal error since PHP 8.x

Attachments (1)

sandbox-hilarious-herring-izawxinstawpxyz.WordPress.2023-11-17.xml (987.1 KB) - added by Ov3rfly 3 months ago.
WXR testcase

Download all attachments as: .zip

Change History (8)

#1 @sabernhardt
3 months ago

  • Component changed from General to Formatting
  • Focuses php-compatibility added

#2 @jrf
3 months ago

  • Focuses php-compatibility removed
  • Keywords php80 added

#3 @jorbin
3 months ago

  • Keywords needs-unit-tests needs-patch added
  • Milestone changed from Awaiting Review to 6.5

Optimistically putting this in the 6.5 milestone.

In my eyes, the biggest question is what should be expected when the regex fails. Perhaps throw an error but then continue without converted smilies?

#4 @jrf
3 months ago

I have a feeling this is a duplicate of #51019...

#5 @Ov3rfly
3 months ago

@jorbin Suggested fix:

// HTML loop taken from texturize function, could possible be consolidated.
$textarr = preg_split( '/(<.*>)/U', $text, -1, PREG_SPLIT_DELIM_CAPTURE ); // Capture the tags as well as in between.
if ( $textarr === false ) {
	return $text;	// do nothing
}
$stop    = count( $textarr ); // Loop stuff.

Would also suggest an earlier milestone as 6.5, otherwise the issue gets burried (again).

@jrf Good find, seems like the same issue is described there, did search but maybe not 3 years back. Would not call it php80 related though as the missing check for false is the real problem here, not the change from Warning to Fatal since PHP 8.x.

#6 @Ov3rfly
3 months ago

@jrf On second thought the issue actually is also php80 related as it can cause a Fatal error and disruption in front end. A fix probably should be backported to all WordPress branches which claim PHP 8.x compatibility.

#7 @jorbin
6 weeks ago

  • Keywords php80 needs-unit-tests needs-patch removed
  • Milestone 6.5 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #51019.

Agree with @jrf, this is a duplicate. Going to consolidate the discussion there

Note: See TracTickets for help on using tickets.