wp_mkdir_p can error when simultaneous requests attempt to create the same directory

[grantmkin]

When two requests happen at the same time that both attempt to create a new directory with wp_mkdir_p, a race condition can occur that results in one request creating the directory and the other triggering an error.

The issue happens when both of the requests reach the file_exists check at nearly the same time:

​https://github.com/WordPress/wordpress-develop/blob/e73db80196c0044503633bda41511258a9516a8b/src/wp-includes/functions.php#L2059

The directory does not exist yet, so the check fails and both requests continue execution of the function. Then the requests are in a race to get to the code where the directory is actually created using mkdir:

​https://github.com/WordPress/wordpress-develop/blob/e73db80196c0044503633bda41511258a9516a8b/src/wp-includes/functions.php#L2082

The first request to reach that point will successfully create the directory and return true, the second will return false when mkdir fails because the directory already exists.

This causes an error upstream in the code because wp_mkdir_p is expected to return true when the directory already exists.

Found when testing the font library on new installs, see ​https://github.com/WordPress/gutenberg/issues/59023#issuecomment-2022746082

[peterwilsoncc]

This is also reproducible via the block editor when uploading media.

Reproduction steps

1. Start with a fresh install or run wp site empty
2. Delete the uploads directory
3. Open a new post
4. In your file system, select multiple image files for uploading
5. Drag these in to the editor (to create a gallery block)
6. This will cause the browser to make multiple, parallel requests to upload the images.
7. One of the uploads may fail due to the race condition in wp_mkdir_p().

The only technique I've been able to think of to resolve this issue is to use the a collision avoidance technique similar to that used by computer networks.

If calling mkdir() fails the first time, back of for usleep( wp_rand( 50, 250000 ) ) and try a second time. As the file system may not actually be writable, I think if it fails the second time it's safe to assume the issue is not a collision. The lower and upper limit would need to be figured out.

I don't love this solution so it would be nice to hear of other suggestions for resolving the race condition.

[siliconforks]

Replying to peterwilsoncc:

The only technique I've been able to think of to resolve this issue is to use the a collision avoidance technique similar to that used by computer networks.

If calling mkdir() fails the first time, back of for usleep( wp_rand( 50, 250000 ) ) and try a second time. As the file system may not actually be writable, I think if it fails the second time it's safe to assume the issue is not a collision. The lower and upper limit would need to be figured out.

I don't love this solution so it would be nice to hear of other suggestions for resolving the race condition.

I'm not sure I understand why it needs to "try a second time"? If the directory has already been created, then nothing more would need to be done, right?

Or maybe you mean it needs to check whether the directory exists a second time? That could work, although I'm not sure why the time sleeping would need to be random in that case. Actually, I'm not sure it would need to sleep at all.

[siliconforks]

Is it possibly as simple as this?

[peterwilsoncc]

Replying to siliconforks:

I'm not sure I understand why it needs to "try a second time"? If the directory has already been created, then nothing more would need to be done, right?

This is to handle a ​concurrency issue in many of the versions of PHP WordPress supports.

• Thread A needs to create /path/to/uploads/2024/05
• Thread B needs to create /path/to/uploads/fonts
• Thread A determines everything from path needs to be created
• Thread A creates path
• Thread B determines everything from to needs to be created
• Thread B creates to
• Thread A fails as to now exists
• Thread A rechecks if 05 exists, it does not.
• Thread B creates the remainder of the path to fonts

The result being that one of the requests will still fail.

I'm not sure why the time sleeping would need to be random in that case. Actually, I'm not sure it would need to sleep at all.

The back-off would be random in an attempt to avoid a second collision. If both threads wait the same amount of time, the path creation would collide a second time a few milliseconds later.

Computer networks use ​Exponential backoff to try again and again, but I think this would be overkill for WordPress's purposes as there is a small risk the request would time out.

[siliconforks]

If the problem is that the recursive version of mkdir() (with third argument true) is buggy, an alternative is to simply avoid using recursive mkdir() and implement the recursion in PHP code.

(I'm not sure if this is actually worth doing, though, given that the PHP bug is said to be fixed in version 8.1, and older versions of PHP will probably not be supported by WordPress for very much longer.)

[siliconforks]

An example of implementing wp_mkdir_p() without using PHP's (buggy) recursive mkdir(). Note: this patch is simplified for illustration purposes and contains some issues - do not attempt to actually merge this.