Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#53278 closed defect (bug) (fixed)

[PHP 8] RSS Widget has infinite loop for instances with an undefined URL.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: jorbin's profile jorbin
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: php8 has-patch has-unit-tests has-testing-info commit
Focuses: Cc:

Description

Discovered during #41683.

The while loop in WP_Widget_RSS::widget() can cause an infinite loop if the URL for the widget instance is incorrectly defined, either by having an unset URL or an empty URL.

A simple test can be seen at https://3v4l.org/PgSZg

If the URL is unset or empty, the function should simply return early before the while loop.

Moving this on the the 5.8 milestone to fix during the bug fix portion of the cycle.

Attachments (2)

53278.diff (1.1 KB) - added by dd32 4 years ago.
testing-53278.gif (3.0 MB) - added by hellofromTonya 4 years ago.
Testing PR1353 before and after. Results: Works as expected (no fatal error with the fix applied)

Change History (22)

#1 @dd32
4 years ago

For others curious, the core change is in substr() and was a documented PHP8 change https://php.watch/versions/8.0/substr-out-of-bounds

substr( '', 1 )

PHP <=7.4: false
PHP >=8.0: ''

@dd32
4 years ago

#2 @dd32
4 years ago

This while loop is also in wp_widget_rss_output().

At the most basic just prefixing a boolean $url check would probably do - 53278.diff.

Looks like SimplePie might also be affected by the change in PHP behaviour due to it's strict type check

./SimplePie/IRI.php:916: if (($port = substr($remaining, $port_start + 1)) === false)

#3 @SergeyBiryukov
4 years ago

  • Summary changed from [PHP 8] RSS Widget has infinite loop for instnaces with an undefined URL. to [PHP 8] RSS Widget has infinite loop for instances with an undefined URL.

#4 follow-up: @peterwilsoncc
4 years ago

@dd32 Do you know if SimplePie is one of the libraries WP has adopted?

#5 in reply to: ↑ 4 @dd32
4 years ago

Replying to peterwilsoncc:

Do you know if SimplePie is one of the libraries WP has adopted?

It's not, I'll have to test if it's actually affected and take my report upstream to https://github.com/simplepie/simplepie

#6 follow-up: @hellofromTonya
4 years ago

  • Keywords needs-unit-tests added

In reviewing with @jrf, yes substr() returns an empty string in PHP 8.0 whereas with < PHP 8.0, it returned false. The patch is checking for a falsey state instead of checking specifically for '' or false. In this instance checking against both values adds more code to the while which increases the cognitive complexity. Instead empty() is appropriate. It allows the next person who comes along to understand what the check is for and why.

As the tests weren't failing on this issue when they were enabled for PHP 8.0, we can only conclude that this piece of code does not have test coverage. We strongly recommend that tests be added to prevent this issue from being reintroduced.

As for SimplePie, based on the code snippet, we are 100% sure it will be affected by this. Using empty() however is not a viable solution because the port may be string '0', which would not work with empty(). For SimplePie, checks against empty string and boolean false are needed. The assignment also needs to be moved out of the conditional expression for this to work. Why? Older versions of PHP (which are supported by SimplePie) do not allow an expression to be passed into empty().

@dd32 Do you want to create the issue and PR upstream for SimplePie? Or would you prefer us to do it?

@peterwilsoncc Do you have time to write the tests for this patch?

#7 in reply to: ↑ 6 ; follow-up: @dd32
4 years ago

Replying to hellofromTonya:

dd32 Do you want to create the issue and PR upstream for SimplePie? Or would you prefer us to do it?

@hellofromTonya If you have the bandwidth for it, I'd greatly appreciate it if you could, I haven't fully had the time to actually test SimplePie yet

#8 in reply to: ↑ 7 @hellofromTonya
4 years ago

Replying to dd32:

@hellofromTonya If you have the bandwidth for it, I'd greatly appreciate it if you could, I haven't fully had the time to actually test SimplePie yet

@dd32 Yes, I have bandwidth and can take care of it.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


4 years ago

This ticket was mentioned in PR #1353 on WordPress/wordpress-develop by hellofromtonya.


4 years ago
#10

  • Keywords has-patch has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/53278

  • Adds happy and unhappy path unit tests for testing the URL argument
  • Applies 53278.diff patch. Props @dd32
  • Replaces implicit loose comparison with ! empty() (see comment 7);

@hellofromTonya
4 years ago

Testing PR1353 before and after. Results: Works as expected (no fatal error with the fix applied)

#11 @hellofromTonya
4 years ago

  • Keywords has-testing-info added

Testing Instructions

Set up the test environment:

  • Change to PHP 8.0.x => In .env file, change LOCAL_PHP=latest to LOCAL_PHP=8.0-fpm.
  • Start up the WordPress Docker environment:
    npm install
    npm run build
    npm run env:start
    npm run env:install
    
  • Set a 6 second timeout limit => In wp-config.php add set_time_limit( 6 ); before /* That's all, stop editing! Happy publishing. */
  • Log into admin area
  • Go to Plugins > Add New
  • Add and activate Classic Widgets plugin
  • Go to Appearance > Widgets
  • Add the RSS widget to the footer

Test before applying fix

  • View the frontend home/root page
  • After 6 seconds, the page should render. Scroll down to the footer section.
  • Notice the fatal error
    Fatal error: Maximum execution time of 6 seconds exceeded in /[path to the project]/wp-includes/widgets/class-wp-widget-rss.php on line 53
    

Test after applying the fix

  • Repeat above steps
  • After rendering, notice there is no fatal error

#12 @hellofromTonya
4 years ago

  • Keywords commit added

Marking PR 1353 ready for commit.

#13 @jorbin
4 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 51107:

Widgets: Prevent infinite loop in PHP8+ if the URL for the widget instance is incorrectly defined

This checks to make sure $link isn't empty before attempting to manipulate it. A simple test to demonstrate this can be seen at https://3v4l.org/PgSZg. Unit tests for both what already works and what is fixed by this change.

Props hellofromTonya, dd32, peterwilsoncc.
Fixes #53278.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 years ago

#15 @hellofromTonya
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for follow-up to add the unit tests that were not committed (and rightly so as they need improvement) with [51107].

This ticket was mentioned in PR #1359 on WordPress/wordpress-develop by hellofromtonya.


4 years ago
#16

Trac ticket: https://core.trac.wordpress.org/ticket/53278

  • Adds the unit tests from PR #1353 (which were not committed in changeset 51107 (as they needed improvement)
  • Adds the improvements suggested by @peterwilsoncc to mock the RSS response for test suite performance improvement

hellofromtonya commented on PR #1353:


4 years ago
#17

Source code merged in changeset https://core.trac.wordpress.org/changeset/51107.

The tests and improvements to the tests is moved to PR #1359.

hellofromtonya commented on PR #1359:


4 years ago
#18

All checks passed except for 7.4 on ubuntu-latest. @peterwilsoncc can you restart that GH action please?

#19 @hellofromTonya
4 years ago

@peterwilsoncc PR 1359 is ready. As it includes your previous review, leaving commit keyword. Once committed, this ticket can be closed again as this PR wraps up the work. Thanks!

#20 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51136:

Widgets: Improve unit tests for RSS Widget.

Prevent unit tests from making HTTP requests to wordpress.org. Specify URLs as https rather than http.

Follow up to [51107].

Props hellofromTonya.
Fixes #53278.

Note: See TracTickets for help on using tickets.