WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 7 weeks ago

Last modified 5 weeks ago

#52241 closed defect (bug) (fixed)

Infinite loop in clean_dirsize_cache()

Reported by: raubvogel Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: major Version: 5.6
Component: Filesystem API Keywords: has-patch early has-unit-tests
Focuses: multisite Cc:

Description

Since WP 5.6 there exsists

<?php
function clean_dirsize_cache( $path ){}

at functions.php:7725.
There we have

<?php
        while ( DIRECTORY_SEPARATOR !== $path && '.' !== $path && '..' !== $path ) {
                $path = dirname( $path );
                unset( $directory_cache[ $path ] );
        }

at functions.php:7735 which loops „forever“ if $path is something like c:\dir\subdir on Windows.

<?php
$path = dirname( $path );

returns the parent path in each step and gets „stuck“ at c:\.

Attachments (4)

s1.png (126.0 KB) - added by raubvogel 10 months ago.
Debug Screenshot 1
s2.png (121.9 KB) - added by raubvogel 10 months ago.
Debug Screenshot 1
s3.png (118.2 KB) - added by raubvogel 10 months ago.
Debug Screenshot 3
Fixed_infinite_loop_on_Windows_systems_.patch (749 bytes) - added by raubvogel 10 months ago.

Download all attachments as: .zip

Change History (53)

#1 in reply to: ↑ description @raubvogel
11 months ago

Btw. this breaks the media upload in my WordPress because media upload calls clean_dirsize_cache().

#2 @SergeyBiryukov
11 months ago

  • Component changed from General to Filesystem API
  • Focuses multisite added
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.6.1
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

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


10 months ago

#4 @audrasjb
10 months ago

  • Milestone changed from 5.6.1 to Future Release

As per today's 5.6.1 bug scrub, moving this ticket to Future release pending further informations/context on the issue/bug (and a patch/unit tests).

@raubvogel
10 months ago

Debug Screenshot 1

@raubvogel
10 months ago

Debug Screenshot 1

@raubvogel
10 months ago

Debug Screenshot 3

#5 @raubvogel
10 months ago

  • Keywords has-patch added; needs-patch removed

#6 @ocean90
10 months ago

#52292 was marked as a duplicate.

#7 @codezen8
9 months ago

Is there a schedule for this patch? I can confirm that media uploads are broken in WordPress 5.6.x for multi-site running on a Windows server. I have tested the patch above and can also confirm that with that line in place media uploads work as expected.

#8 @sjlevy
9 months ago

To comment also in support of this patch: it appears to work fine and needed to be re-applied after the last WP update.

I encountered the infinite loop issue on 3 of my windows server-based multisites a couple of months ago-- located the offending line of code, and then subsequently found this ticket. Thank you

#9 @drosmog
9 months ago

Bug continues to affect IIS in WordPress 5.7. Line changed to 7813 in functions.php.

#10 @teachlynx
9 months ago

Same as @drosmog, bug continues to affect IIS 10 in WordPress 5.7. Line changed to 7813 in functions.php.

#11 @SergeyBiryukov
9 months ago

  • Milestone changed from Future Release to 5.8

#12 @EkoJr
9 months ago

May not be the most elegant solution, but it is a small simple change, and I'm a little surprised this ticket wouldn't have a high priority & got postponed to a future subversion considering it affects IIS servers and would appear to be a blocker bug for media.

Basically, the modified code compares the $last_path with the current $path, and if the $path is the same as it previously was in the previous iteration, it will then break out of the loop.

Original Code (WP 5.7)
wp-includes/functions.php:7812 > function clean_dirsize_cache( $path )

<?php
while ( DIRECTORY_SEPARATOR !== $path && '.' !== $path && '..' !== $path ) {
        $path = dirname( $path );
        unset( $directory_cache[ $path ] );
}

Modified Code

<?php
$last_path = '';
while ( DIRECTORY_SEPARATOR !== $path && '.' !== $path && '..' !== $path && $last_path !== $path ) {
        $last_path = $path;
        $path      = dirname( $path );
        unset( $directory_cache[ $path ] );
}

It's more of a conceptual solution, and someone may have a better answer. So, I am a bit hesitant creating a commit for this (plus, it's been awhile since I touched SVN).

#13 @bartoszgrzesik
8 months ago

I have problem with this bug. @EkoJr resolve is good to me.

#14 follow-ups: @hellofromTonya
6 months ago

  • Keywords needs-testing added
  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. This one needs tests and testing. Punting to 5.9 to continue progress towards resolution.

#15 in reply to: ↑ 14 @teachlynx
6 months ago

While there is probably a lot of things to do to push out 5.8, I am a bit disappointed that for a bug like this, which has a fix already checked by many users on this page, we have yet to wait for another release because this one "needs testing". For people like me who host their WP sites on IIS 10, we need to manually fix each of our multisite installs, so this is not hell, but it is clearly a waist of time.

#16 in reply to: ↑ 14 @codezen8
6 months ago

Replying to hellofromTonya:

Today is 5.8 Beta 1. This one needs tests and testing. Punting to 5.9 to continue progress towards resolution.

This is ridiculous that core WordPress files continue to need to be modified in a multisite environment because of one line of code that multiple people have tested.

What is the process that needs to be done here? I know I was not aware that this needed further steps in order to be included in 5.8. This is a critical bug that disables key functionality in a multisite environment and there have been many releases since it was identified.

#17 @joegasper
5 months ago

#53245 was marked as a duplicate.

#18 @janthiel
5 months ago

This also happens on Unix if the $path is empty. There should be added an empty check for $path

        if ( empty( $directory_cache ) || empty( $path ) ) {
                return;
        }

https://core.trac.wordpress.org/browser/tags/5.7.1/src/wp-includes/functions.php#L7815

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


4 months ago

#20 @hellofromTonya
4 months ago

  • Keywords early added
  • Owner changed from SergeyBiryukov to hellofromTonya
  • Status changed from accepted to assigned

Marking this early to ensure elevate attention for it: finalized testing, early commit, and then broad testing throughout the 5.9 cycle.

Switching ownership to me after talking with Sergey. I'll champion gathering the testing info, doing testing, and the code review to get this ticket ready for early commit.

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


4 months ago

#22 @torralaq
4 months ago

Never mind :)

Last edited 4 months ago by torralaq (previous) (diff)

#23 @joseph.dickson
4 months ago

This issue just popped up on one of my sites within a multisite. So far the others within the network have not ran into this problem.

Message on media upload: "Post-processing of the image failed likely because the server is busy or does not have enough resources. Uploading a smaller image may help. Suggested maximum size is 2500 pixels."

https://joseph-dickson.com/wp-content/uploads/2021/08/file-upload-error-multisite.png

Is it possible that only a single site could be affected?

#24 @SergeyBiryukov
3 months ago

#53964 was marked as a duplicate.

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


3 months ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-media by ekojr. View the logs.


3 months ago

This ticket was mentioned in Slack in #core-test by fabiankaegy. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


2 months ago

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


2 months ago

#32 @jrf
2 months ago

@SergeyBiryukov was kind enough to point me to this ticket.

FYI: I ran into this exact same issue the other day trying to debug a PHP 8.1 deprecation, where as soon as I put the fix in place and run the tests in isolation, the tests go into an infinite loop.

Basically the existing Tests_Multisite_Dirsize_Cache tests when run stand-alone on a Windows system already cover the problem when run with the multisite config, i.e. -c tests/phpunit/multisite.xml.

I haven't investigated yet why the issue doesn't occur in a full test run, but there may be a hidden dependency on some other test which prevents the infinite loop (which is even worse).

We should consider adding test runs against Windows and Mac to the CI builds. I suspect that will be a lot easier if we remove the Docker lock-in from the CI, but I suppose that's for another issue.

#33 @jrf
2 months ago

Edit... Sorry - my comment above is about the recurse_dirsize() function, not the clean_dirsize_cache() function. All the same, the same principle applies to both functions by the looks of it.

#34 @hellofromTonya
8 weeks ago

Tomorrow at 14:00 UTC, @jrf and I are hosting a live working session to discuss, test, and fix:

  • the infinite loop problem in both recurse_dirsize() and clean_dirsize_cache()
  • PHP 8.1 incompatibilities in both functions

All are invited to join the working the session to collaborate together.

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


8 weeks ago

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


8 weeks ago

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


8 weeks ago

This ticket was mentioned in PR #1741 on WordPress/wordpress-develop by jrfnl.


8 weeks ago

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

### clean_dirsize_cache(: fix infinite loop on Windows

When the PHP native dirname() function is used on a Windows disk name - i.e. C:\-, it will return the same, i.e, it will return C:\ again.
The clean_dirsize_cache() function didn't have guard clause against this, which meant that on Windows based systems and IIS servers, this function would result in WordPress getting stuck into an infinite loop.

The adjustment to the while part of the function fix this by checking if the return value of the dirname() function call is the same as the original path passed to dirname(), which effectively fixes the infinite loop.

As we were looking at this function now anyway, we also noticed a number of other improvements which were begging to be made and we have thus taken the liberty to make them.

  1. Add input validation for the $path parameter to guard against invalid variable types being passed into the function.
  2. Guard against an empty $path parameter, which would result in an infinite loop on both Windows as well as *nix based systems.

In both these cases, a "doing it wrong" will now be thrown.

  1. When a non-empty string, which isn't a path would previously be passed, the dirname() function would transform that to a . and the . key in the transient cache would be cleared out.

This was a bug as there is no relation between a non-path string and the root directory of file system.

This bug has been fixed by checking that something could actually be a path and handling received non-empty, non-path input parameters in a special way, i.e only removing the cache key for the passed string and bowing out from further processing.

Unfortunately, no tests can be added to guard against the infinite loop.

For the other fixes, we have added appropriate unit tests.

### PHP 8.1: recurse_dirsize(): fix autovivification from false to array

PHP natively allows for autovivification (auto-creation of arrays from falsey values). This feature is very useful and used in a lot of PHP projects, especially if the variable is undefined. However, there is a little oddity that allows creating an array from a false and null value.

The above quote is from the PHP 8.1 RFC and the (accepted) RFC changes the behaviour described above to deprecated auto creation of arrays from false. As it is deprecated, it _will_ still work for the time being, but as of PHP 9.0, this will become a Fatal Error, so we may as well fix it now.

The recurse_dirsize() function retrieves a transient and places it in the $directory_cache variable, but the get_transient() function in WP returns false when the transient doesn't exist, which subsequently can lead to the above mentioned deprecation notice.

By verifying that the $directory_cache variable is an array before assigning to it and initializing it to an empty array, if it's not, we prevent the deprecation notice, as well as harden the function against potentially corrupted transients where this transient would not return the expected array format, but some other variable type.

Includes adding dedicated unit tests for both the PHP 8.1 issue, as well as the hardening against corrupted transients.

Includes some girl-scouting: touching up a parameter description and some code layout.

Refs:

Trac ticket: https://core.trac.wordpress.org/ticket/52241
Trac ticket: https://core.trac.wordpress.org/ticket/53635

#39 follow-up: @hellofromTonya
8 weeks ago

  • Keywords needs-testing removed

In PR 1741 resolves the infinite loop on Windows, empty on Unix, and PHP 8.1 incompatibilities.

@raubvogel @janthiel and everyone who has reported this issue, can you please test the fix in this PR and provide feedback?

@jrf and I have reviewed and tested the fixes. Let's wait one week to give folks time to test it and provide feedback before committing.

#40 @jrf
8 weeks ago

FYI: as a side-effect of the debug effort related to this ticket, a PR has been opened for the PHP Core Manual to warn about the use of dirname() in a loop in a Windows context and the risk of getting into an infinite loop.
Ref: https://github.com/php/doc-en/pull/1005

#41 @janthiel
8 weeks ago

@hellofromTonya @jrf Thank you so much for this thoroughly fix. Looks very good codewise :-) I will give it a test run on our DEV env next week and get back to you regarding the empty path on Unix part.

Have a great weekend!

#42 in reply to: ↑ 39 @raubvogel
8 weeks ago

@hellofromTonya @jrf thank you very much. I did a SVN updated (5.9-alpha-51272-src) and applied the patch from PR 1741. Started to add a new media in the Media Library (upload) while breaking the code execution at the new clean_dirsize_cache() function, stepping and watching the variable values … works nicely! At least on WordPress Multisite, Windows 10, Apache/2.4.46, PHP 8.0.3 the issue seems to be fixed.

#43 @joegasper
7 weeks ago

Applied patched files from PR1741 on multisite, Windows, PHP 7.4 - no problems with media uploads. Thank you.

#44 @hellofromTonya
7 weeks ago

#54271 was marked as a duplicate.

#45 @hellofromTonya
7 weeks ago

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

In 51910:

FileSystem API: Fix infinite loop on Windows for clean_dirsize_cache().

When the PHP native dirname() function is used on a Windows disk name - i.e. C:\-, it will return the same, i.e, it will return C:\ again.

The clean_dirsize_cache() function didn't have guard clause against this, which meant that on Windows based systems and IIS servers, this function would result in WordPress getting stuck into an infinite loop.

The adjustment to the while part of the function fix this by checking if the return value of the dirname() function call is the same as the original path passed to dirname(), which effectively fixes the infinite loop.

A number of other improvements made:

  1. Add input validation for the $path parameter to guard against invalid variable types being passed into the function.
  1. Guard against an empty $path parameter, which would result in an infinite loop on both Windows as well as *nix based systems.

In both these cases, a PHP notice will now be thrown.

  1. When a non-empty string, which isn't a path would previously be passed, the dirname() function would transform that to a . and the . key in the transient cache would be cleared out.

This was a bug as there is no relation between a non-path string and the root directory of file system.

This bug has been fixed by checking that something could actually be a path and handling received non-empty, non-path input parameters in a special way, i.e only removing the cache key for the passed string and bowing out from further processing.

Unfortunately, no tests can be added to guard against the infinite loop.

For the other fixes, we have added appropriate unit tests.

Follow-up up [49212], [49616], [49744].

Props jrf, hellofromTonya, raubvogel, sergeybiryukov, codezen8, sjlevy, drosmog, teachlynx, ekojr, bartoszgrzesik, joegasper, janthiel, josephdickson, ocean90, audrasjb.
Fixes #52241.

#46 @hellofromTonya
7 weeks ago

In 51912:

FileSystem API: Add safeguard for invalid return from get_attached_file() in wp_delete_attachment().

The get_attached_file() function is supposed to return the path to the file, but could:

  1. Return false if the file doesn't exist.
  2. Return literally anything else, as a filter is being applied to the value on return.

As the clean_dirsize_cache() now has input validation, passing anything but a non-empty string to clean_dirsize_cache() will result in a PHP error notice.

This was exposed by the Tests_Post_GetPostStatus::wpSetUpBeforeClass() method which started generating unexpected output (the doing it wrong message) during the test run.

While this indicates that there is a flaw in the mocking being done in the test suite, debugging that is outside of the scope of the current patch.

At the same time, as based on the above point, this could potentially happen in a real-world situation as well, adding additional conditions to the if in the wp_delete_attachment() function before calling the clean_dirsize_cache() function, is warranted.

As there are no tests for the wp_delete_attachment() function at all at this time, we're not adding a test specifically for this change for now. This should however be addressed in the future, when tests will be added to cover the wp_delete_attachment() function completely.

Follow-up to [32619], [49212], [51910].

Props jrf, hellofromTonya.
See #52241.

#48 @hellofromTonya
7 weeks ago

Thank you everyone for contributing to identifying, triaging, fixing, and testing this issue!

#49 @SergeyBiryukov
5 weeks ago

In 51938:

Tests: Some test improvements for clean_dirsize_cache() tests:

  • Move the directory being tested to the data directory, for consistency with other test data.
  • Set the svn:eol-style property to native, for consistency with other files.
  • Correct the test class name in dummy.txt.

Follow-up to [51246], [51910], [51911].

See #52241, #53363.

Note: See TracTickets for help on using tickets.