Make WordPress Core

Opened 9 years ago

Closed 2 years ago

#36184 closed defect (bug) (duplicate)

Eternal loop under certain conditions

Reported by: mikk3lro's profile Mikk3lRo Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4.2
Component: Filesystem API Keywords: has-patch
Focuses: performance Cc:

Description

I've run into this quite a few times now, and often it puts a massive strain on the server and very rapidly fills the error log if logging is enabled.

An very tight eternal loop in wp_mkdir_p() - wp-includes/functions.php line 1556 to 1613 - can be triggered if a path in the database is wrong.

5 lines from line 1583 read:


// We need to find the permissions of the parent folder that exists and inherit that.
$target_parent = dirname( $target );
while ( '.' != $target_parent && ! is_dir( $target_parent ) ) {
    $target_parent = dirname( $target_parent );
}

I move wordpress installs around between servers and / or folders quite often, and sometimes an old (and now invalid) upload_path in the DB will cause this to loop forever. Obviously the mistake is mine for not correcting the path up front, but an eternal loop that may write to the error-log on each iteration (if open_basedir is active, possibly under other circumstances too) is not a very graceful way to fail... on a shared host (where you may not have the control I do) it could be pretty catastrophic.

I'm not sure if other conditions are needed to trigger it, but I've only ever seen it when the upload path is wrong... seeing as it won't be triggered unless the path is wrong an easy solution would be to just check if '/' has been reached and throw an error pointing towards the solution...

Attachments (3)

functions.php.patch (568 bytes) - added by wgrafael 9 years ago.
functions.php.diff (572 bytes) - added by robertstaddon 8 years ago.
functions.php.2.patch (568 bytes) - added by robertstaddon 8 years ago.

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
9 years ago

  • Component changed from General to Filesystem API

#2 @wgrafael
9 years ago

  • Keywords has-patch added

#3 @robertstaddon
8 years ago

  • Type changed from enhancement to defect (bug)

I am encountering the very same issue that @Mikk3lRo describes in this ticket. I have a WordPress install that was migrated from one server to another. Somewhere there is a misconfiguration in a very popular and well-supported plugin that is triggering an eternal loop of the wp_mkdir_p() function in wp-includes/functions.php.

Though I obviously need to figure out whatever incorrect setting is causing this, I wholeheartedly agree that an eternal loop that writes to the error-log on each iteration (if open_basedir is active, possibly under other circumstances too) is not a very graceful way to fail. It very quickly fills up the hard drive and crashes the entire server.

Here is the exact error message that appears millions of times in the error_log when triggered:

mod_fcgid: stderr: PHP Warning: is_dir(): open_basedir restriction in effect. File(/) is not within the allowed path(s): in /httpdocs/wp-includes/functions.php on line 1616

I will try the patch by @wgrafael and see if that solves it. It certainly does make a lot of sense to add an is_readable test before attempting a recursive function of this nature.

Last edited 8 years ago by robertstaddon (previous) (diff)

#4 @robertstaddon
8 years ago

#30258 was marked as a duplicate.

#5 @tmuka
8 years ago

  • Resolution set to invalid
  • Status changed from new to closed

We ran into this issue migrating a site this week, and it is amazing how fast the apache error_log grows in size with only a single person testing the site. While not a common issue, it is certainly a serious one! I can only imagine how fast this would completely kill my server if this were more than just a single user hitting the site for testing.

Thanks @robertstaddon for the patch, it takes care of the error log growth issue. I wonder why this patch hasn't been merged into core!? Perhaps it's not optimal to do a filesystem permissions check on each call? Any sort of "cheaper" sanity check could be good too, like the suggested adding

 <?php while ( '.' != $target_parent && ! is_dir( $target_parent )  && $target_parent != '/'  ) {  

(or course that's not cross platform compatible since on windows we'd need to check for '\' instead of '/')
Perhaps just adding an exit condition to prevent infinite loops when they are simply detected...

<?php
// We need to find the permissions of the parent folder that exists and inherit that.
$target_parent = dirname( $target );
     $target_parent = dirname( $target );
     $prev_target_parent = '';
        while ( '.' != $target_parent && ! is_dir( $target_parent ) ) {
            $prev_target_parent =  $target_parent;
            $target_parent = dirname( $target_parent );
            if($target_parent != $prev_target_parent) {
                error_log('infinite loop issue detected with finding parent folder');
                break;
            }
        }

Now i'm trying to track down the actual cause. The wp_options upload_path value is an empty string (which i think is correct). There must be a plugin or theme doing something funky here.

For other people searching for this issue, my error logs looked like this

[Mon Aug 21 10:07:46.705173 2017] [proxy_fcgi:error] [pid 6813] [client 192.168.1.10:36156] AH01071: Got error 'ww/vhosts/example.com/:/tmp/) in /var/www/vhosts/example.com/httpdocs/wp-includes/functions.php on line 1613\nPHP message: PHP Warning:  is_dir(): open_basedir restriction in effect. File(/) is not within the allowed path(s): (/var/www/vhosts/example.com/:/tmp/) in /var/www/vhosts/example.com/httpdocs/wp-includes/functions.php on line 1613
Last edited 8 years ago by tmuka (previous) (diff)

#6 @tmuka
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#7 @costdev
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from reopened to closed

This appears to have been fixed in [42801], which stops the while loop when dirname( $target_parent ) === $target_parent, i.e. dirname( '/' ) === '/'.

While this ticket is older, I'll close this as a duplicate of #43417 as it's been resolved.

Note: See TracTickets for help on using tickets.