Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#51913 closed defect (bug) (fixed)

Potential `unsupported operand types` dashboard fatal on PHP8 + multisite + upload space check

Reported by: iandunn's profile iandunn Owned by: iandunn's profile iandunn
Milestone: 5.6 Priority: highest omg bbq
Severity: blocker Version: 5.7
Component: Administration Keywords: php8 has-patch commit dev-reviewed
Focuses: administration, multisite Cc:

Description

After upgrading my local environment to PHP 8, and a test site to the 5.6 branch, I got a fatal error in the At A Glance dashboard widget.

PHP Fatal error:  Uncaught TypeError: Unsupported operand types: array / int in /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-includes/ms-functions.php:2635
Stack trace:
#0 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-admin/includes/dashboard.php(1549): get_space_used()
#1 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-includes/class-wp-hook.php(287): wp_dashboard_quota('')
#2 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-includes/class-wp-hook.php(311): WP_Hook->apply_filters('', Array)
#3 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-includes/plugin.php(484): WP_Hook->do_action(Array)
#4 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-admin/includes/dashboard.php(411): do_action('activity_box_en...')
#5 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-admin/includes/template.php(1389): wp_dashboard_right_now('', Array)
#6 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-admin/includes/dashboard.php(265): do_meta_boxes(Object(WP_Screen), 'column4', '')
#7 /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-admin/index.php(181): wp_dashboard()
#8 {main}
  thrown in /Users/iandunn/vhosts/localhost/wordcamp.test/public_html/mu/wp-includes/ms-functions.php on line 2635

This seems related to the caching added in r49212 (#19879), cc @helen.

get_dirsize() and recurse_dirsize() returned an array instead of an int. return $directory_cache[ $cache_path ] was expecting that value to be an int, but instead it was array( 'size' => 0 ).

I don't see how it could be created like this, but the transient value was:

a:1:{s:94:"/Users/iandunn/vhosts/localhost/wp-develop.test/public_html/build/wp-content/blogs.dir/2/files";a:1:{s:4:"size";i:0;}}

In PHP7 the arguments of get_dirsize( $upload_dir['basedir'] ) / MB_IN_BYTES would have been silently cast to match types, but in PHP8 it's a fatal error.

The error is no longer happening on my test site, and I haven't been able to reproduce it in a fresh install, so this may be invalid. There doesn't appear to be any 3rd party code related to this behavior, though. Since the 5.6 release date is only a week away, and it's had a relatively small amount of real-world testing w/ PHP 8, I thought it'd be prudent to get some more eyes on this.

get_dirsize() is only called in Multisite, but recurse_dirsize() is called by single site, so there may be other instances of a similar problem.


Trying to reproduce:

Before I upgraded and got the error, my environment looked like this:

  • PHP 7.2
  • WP 5.5 branch via SVN, probably a few weeks behind the head
  • Multisite
  • upload_space_check_disabled = 0

I installed PHP 8.0.0, then svn sw ^/branches/5.6 (head was r49734 at the time of writing this)

IIRC, I encountered the error on the main site, which affects the execution path in get_dirsize().

Change History (22)

#1 @iandunn
3 years ago

  • Owner set to iandunn
  • Priority changed from high to highest omg bbq
  • Severity changed from major to blocker
  • Status changed from new to accepted

Ah, I can reproduce this now.

It looks like 5.5 set the transient in get_dirsize, and included the size sub-array. When that logic was moved to recurse_dirsize, the size subarray was removed, probably because it seems unnecessary. That caused a conflict between the structure of new and old transients, though.

https://core.trac.wordpress.org/browser/branches/5.5/src/wp-includes/functions.php?rev=49453&marks=7518-7520#L7490

https://core.trac.wordpress.org/browser/branches/5.6/src/wp-includes/functions.php?rev=49643&marks=7708#L7705

The simplest fix might be to clear the transient during the upgrade. If that won't work, then maybe the 5.6 code should be updated to save the size item?

I'll look into those options, but let me know if anyone has any thoughts.

#2 @iandunn
3 years ago

To reproduce, I got back to a 5.5 state:

  1. svn sw ^/branches/5.5
  2. manually deleted transients in wp_{main site id}_options
  3. loaded main site dashboard
  4. checked database, saw that transient had size subarray

...then tried to upgrade again:

  1. svn sw ^/branches/5.6
  2. loaded main site dashboard, saw fatal in the widget

#3 @iandunn
3 years ago

In the public plugin repo, there are some plugins accessing the transient directly:

https://wpdirectory.net/search/01ERHZ02VEQ161CMAR4JHVZXCQ

...but most are just deleting it.

These 3 seem to have copied older versions of get_dirsize() and recurse_dirsize() into their codebase directly:

  • PT Addons for Elementor Lite - 4k installs
  • MediaPress - 3k installs
  • Trustalyze Verified WooCommerce Reviews Extension - 10 installs

It looks like they could set the transient w/ the old structure, and then that'd cause a fatal when Core tries to access it.

#4 @iandunn
3 years ago

  • Keywords needs-testing removed

CC'ing the authors of those plugins: @sbrajesh, @rebusify, @paramthemes

Your users may experience a fatal error on WP 5.6, please read the ticket above for details.

The ideal solution for you might be to include wp-includes/functions.php, rather than copy/pasting those functions into your plugin code. I've only glanced at your code, though, so that might not work in your specific case. If those files aren't running in a WP context, you could use the REST API to access the data instead.

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


3 years ago

#6 @janthiel
3 years ago

@iandunn The only line of code that I can think of which could trigger the mentioned behaviour should be this:

https://core.trac.wordpress.org/browser/branches/5.6/src/wp-includes/functions.php?rev=49643&marks=7639#L7639

This is the only direct return of an existing transient value. For the main site it might resolve and actually return the previously stored array

['size' => 12345]

Each other case should return "fresh" or no values due to the new way how the cache paths are actually stored per month.

Resetting the transient will not help, if there are plugins using own code to write it. So I would suggest adding a check in the aforementioned line to see whether the return is an actual int before returning early? That should catch backward-compat and plugin issues.

What do you think?

Version 1, edited 3 years ago by janthiel (previous) (next) (diff)

This ticket was mentioned in PR #783 on WordPress/wordpress-develop by iandunn.


3 years ago
#7

  • Keywords has-patch added

checks to make sure the transient value being returned is actually an int. it was an array in 5.5, but should be an int in 5.6. need to handle case where old 5.5 transient is present during upgrade to 5.6

see https://core.trac.wordpress.org/ticket/51913#comment:6

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

#8 @iandunn
3 years ago

Yeah, that seems to be working so far, thanks!

I'll work on some unit tests.

#9 @iandunn
3 years ago

Another condition that might be required to reproduce this is to have WP installed in a subdirectory, with a custom content directory a level above it. e.g.,

wp installed at: /home/foo/wordpress
content dir at:  /home/foo/wp-content

...and nginx config like:

server {
	root /home/foo/wordpress;

	location /wp-content/ {
		root /home/foo;
	}
}

...and a wp-config like:

define( 'ABSPATH',        '/home/foo/wordpress'  );
define( 'WP_CONTENT_DIR', '/home/foo/wp-content' );

In a setup like that, $cache_path will be equal to $directory, because ABSPATH isn't stripped out.

Making sure the return value is an int might still be worth it, but the underlying cause seems to be that the code makes assumptions about how things are setup. That could cause other problems if we don't make it more robust.

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


3 years ago

This ticket was mentioned in Slack in #core-multisite by francina. View the logs.


3 years ago

#12 @sbrajesh
3 years ago

Thank you @iandunn for notifying and suggesting the solution. Sincerely appreciate it.

#13 @iandunn
3 years ago

Status update:

Both the is_int() and the WP_CONTENT_DIR fixes seem to fix the problem on their own, but I'd recommend keeping both for safety.

I'm not done w/ the unit tests yet, but have manually tested in a couple different kinds of setups, and don't expect to discover any problems. If the WP_CONTENT_DIR switch looks good to @janthiel, then I think we're probably in good shape.

My only reservations are the short time period between RC3 and release, and the variety of setups in the wild. So, getting more eyes and testers would help a lot, especially if folks have dev/staging environments for real multisites that they could run RC2+this on.

cc @peterwilsoncc, @dd32, @flixos90, @johnjamesjacoby

#14 @janthiel
3 years ago

@iandunn Thanks for the amazing job and jumping in :-)
I agree that using WP_CONTENT_DIR seems more reliable than ABSPATH so I fully agree with that change.
I would also vote to leave in the is_int check for reliability reasons and to avoid type mismatches in the return. That should fix the possible FATAL.

The str_replace part using WP_CONTENT_DIR is meant for normalization and removal of path fragments that might change on hosting level (like moving web root or such). So WordPress should only store information relative to WordPress paths and independent of the server.

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


3 years ago

This ticket was mentioned in Slack in #core-multisite by hellofromtonya. View the logs.


3 years ago

#17 @iandunn
3 years ago

  • Keywords needs-testing added
  • Version set to trunk

I think the PR is settled and ready for testing.

Reproducing is a bit of work, since there are some specific preconditions:

  • PHP 8
  • multisite
  • custom WP_CONTENT_DIR that's not a child of WP's ABSPATH folder (e.g., like Bedrock)
  • upload_space_check_disabled option = 0

Steps:

  • install latest 5.5
  • delete the dirsize_cache transient
  • visit wp-admin/index.php. it should work fine, and the transient should be populated.
  • upgrade to 5.6-RC2. if you now visit wp-admin/index.php you should see a fatal error inside the At A Glance widget
  • if you then apply https://github.com/WordPress/wordpress-develop/pull/783.diff and refresh, the fatal should go away, and you should see the correct size calculation in that widget.

testing the PR under PHP 7 + single site is helpful too, to make sure it doesn't have any unintended side-effects

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


3 years ago

#19 @iandunn
3 years ago

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

In 49744:

Multisite: Cache absolute dirsize paths to avoid PHP 8 fatal.

r49212 greatly improved the performance of get_dirsize(), but also changed the structure of the data stored in the dirsize_cache transient. It stored relative paths instead of absolute ones, and also removed the unnecessary size array.

That difference in data structures led to a fatal error in the following environment:

  • PHP 8
  • Multisite
  • A custom WP_CONTENT_DIR which is not a child of WP's ABSPATH folder (e.g., Bedrock)
  • The upload_space_check_disabled option set to 0

After upgrading to WP 5.6, the dirsize_cache transient still had data in the old format. When wp-admin.php/index.php was visited, get_space_used() received an array instead of an int, and tried to divide it by another int. PHP 7 would silently cast the arguments to match data types, but PHP 8 throws a fatal error:

Uncaught TypeError: Unsupported operand types: array / int

recurse_dirsize() was using ABSPATH to convert the absolute paths to relative ones, but some upload locations are not located under ABSPATH. In those cases, $directory and $cache_path were identical, and that triggered the early return of the old array, instead of the expected int.

In order to avoid that, this commit restores the absolute paths, but without the size array. It also adds a type check when returning cached values. Using absolute paths without size has the result of overwriting the old data, so that it matches the new format. The type check and upgrade routine are additional safety measures.

Props peterwilsoncc, janthiel, helen, hellofromtonya, francina, pbiron.
Fixes #51913. See #19879.

#20 @iandunn
3 years ago

In 49745:

Multisite: Cache absolute dirsize paths to avoid PHP 8 fatal.

r49212 greatly improved the performance of get_dirsize(), but also changed the structure of the data stored in the dirsize_cache transient. It stored relative paths instead of absolute ones, and also removed the unnecessary size array.

That difference in data structures led to a fatal error in the following environment:

  • PHP 8
  • Multisite
  • A custom WP_CONTENT_DIR which is not a child of WP's ABSPATH folder (e.g., Bedrock)
  • The upload_space_check_disabled option set to 0

After upgrading to WP 5.6, the dirsize_cache transient still had data in the old format. When wp-admin.php/index.php was visited, get_space_used() received an array instead of an int, and tried to divide it by another int. PHP 7 would silently cast the arguments to match data types, but PHP 8 throws a fatal error:

Uncaught TypeError: Unsupported operand types: array / int

recurse_dirsize() was using ABSPATH to convert the absolute paths to relative ones, but some upload locations are not located under ABSPATH. In those cases, $directory and $cache_path were identical, and that triggered the early return of the old array, instead of the expected int.

In order to avoid that, this commit restores the absolute paths, but without the size array. It also adds a type check when returning cached values. Using absolute paths without size has the result of overwriting the old data, so that it matches the new format. The type check and upgrade routine are additional safety measures.

Props peterwilsoncc, janthiel, helen, hellofromtonya, francina, pbiron.
Reviewed by SergeyBiryukov, iandunn.
Merges [49744] to the 5.6 branch.
Fixes #51913. See #19879.

#22 @hellofromTonya
3 years ago

  • Keywords commit dev-reviewed added; needs-testing removed
Note: See TracTickets for help on using tickets.