Infinite loop in clean_dirsize_cache()

[raubvogel]

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:\.

[raubvogel]

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

[audrasjb]

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]

Debug Screenshot 1

[raubvogel]

Debug Screenshot 1

[raubvogel]

Debug Screenshot 3

[ocean90]

#52292 was marked as a duplicate.

[codezen8]

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.

[sjlevy]

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

[drosmog]

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

[teachlynx]

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

[EkoJr]

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

[bartoszgrzesik]

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

[hellofromTonya]

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

[teachlynx]

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.

[codezen8]

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.

[joegasper]

#53245 was marked as a duplicate.

[janthiel]

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

[hellofromTonya]

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.

[torralaq]

Never mind :)

[joseph.dickson]

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

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

[SergeyBiryukov]

#53964 was marked as a duplicate.

[jrf]

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

[jrf]

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.

[hellofromTonya]

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.

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

3. 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:

• ​https://wiki.php.net/rfc/autovivification_false
• ​https://developer.wordpress.org/reference/functions/get_transient/

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

[hellofromTonya]

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.

[jrf]

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

[janthiel]

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

[raubvogel]

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

[joegasper]

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

[hellofromTonya]

#54271 was marked as a duplicate.

[hellofromTonya]

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.

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 PHP notice will now be thrown.

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

[hellofromTonya]

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.

[prbot]

​hellofromtonya commented on ​PR #1741:

Committed with changesets https://core.trac.wordpress.org/changeset/51910, https://core.trac.wordpress.org/changeset/51911, https://core.trac.wordpress.org/changeset/51912

[hellofromTonya]

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

[SergeyBiryukov]

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.