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

[iandunn]

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

[iandunn]

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.

[iandunn]

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

[iandunn]

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.

[iandunn]

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.

[janthiel]

@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 or root path of a site it might actually resolve and return the previously stored array when an INT is expected

['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?

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

[iandunn]

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

I'll work on some unit tests.

[iandunn]

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.

[sbrajesh]

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

[iandunn]

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

[janthiel]

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

[iandunn]

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

[iandunn]

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.

[iandunn]

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.

merged in https://core.trac.wordpress.org/changeset/49744/

thanks everyone!