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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (22)
#2
@
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
@
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:
↓ 5
@
4 years ago
@dd32 Do you know if SimplePie is one of the libraries WP has adopted?
#5
in reply to:
↑ 4
@
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:
↓ 7
@
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:
↓ 8
@
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
@
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);
@
4 years ago
Testing PR1353 before and after. Results: Works as expected (no fatal error with the fix applied)
#11
@
4 years ago
- Keywords has-testing-info added
Testing Instructions
Set up the test environment:
- Change to PHP 8.0.x => In
.env
file, changeLOCAL_PHP=latest
toLOCAL_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
addset_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
#13
@
4 years ago
- Owner set to jorbin
- Resolution set to fixed
- Status changed from new to closed
In 51107:
This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.
4 years ago
#15
@
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?
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