get_filesystem_method() isn't unique enough

[bikecrazyy]

Issue:
I have 4 plugins installed that use the get_filesystem_method() and I randomly get php errors thrown and cause things not to work right sometimes.

Explanation:
When two or more calls fire off get_file_system_method() within the same second it can cause errors to be thrown by the fopen() and unlink() functions. What is causing the errors to be thrown is a collision with the file name due to the $temp_file_name not being random enough and using the time() to create a unique file name key.

Proposed fix:
Add uniqid() to the end of the string to create a random string. I understand that uniqid() function is not a secure value but should be random enough so the name collision won't happen.

php_error_log

Current Code

wp-admin\includes\file.php
<?php
        if ( ! $method ) {
                $temp_file_name = $context . 'temp-write-test-' . time();
                $temp_handle = @fopen($temp_file_name, 'w');

Proposed fix

<?php
        if ( ! $method ) {
                $temp_file_name = $context . 'temp-write-test-' . time() . uniqid();
                $temp_handle = @fopen($temp_file_name, 'w');

[pross]

Why not use microtime() instead of time()

[bikecrazyy]

Replying to pross:

Why not use microtime() instead of time()

There are many ways of fixing it for sure, I choose to just add some bytes to the end but you might be able to do microtime, I haven't tested to see if it will have a collision in the name but I did test uniqid()

[jrchamp]

Replying to bikecrazyy:

Replying to pross:

Why not use microtime() instead of time()

There are many ways of fixing it for sure, I choose to just add some bytes to the end but you might be able to do microtime, I haven't tested to see if it will have a collision in the name but I did test uniqid()

I think what @pross is saying is that uniqid() uses microtime() internally, so rather than append uniqid() to time(), it makes more sense to remove the redundant time() and to instead use either microtime(true) or uniqid(). The primary difference between the two options is whether you prefer a decimal or hex string.

[bikecrazyy]

I think what @pross is saying is that uniqid() uses microtime() internally, so rather than append uniqid() to time(), it makes more sense to remove the redundant time() and to instead use either microtime(true) or uniqid(). The primary difference between the two options is whether you prefer a decimal or hex string.

@jrchamp:
Makes sense, I changed it on my server to microtime(true) to give it a try. After a little load testing, the results were better and less errors were thrown, but there were still file access issues. See error log below.

php_error_log:
=

Tested Code:

<?php
      $temp_file_name = $context . 'temp-write-test-' . microtime(true);

When I use time() and uniqid() I don't see anything in the error log. Maybe since I used both time() and uniqid() enough cpu cycles have gone by? I'm sure calling it twice shouldn't be the answer like you explained.

NOTE:
I'm using the latest version of wordpress and PHP. I'm running php using fast-cgi via IIS on windows. This might have nothing to do with it or maybe something to do with it but when you use PHP with IIS you have to use the "Non-Thread Safe (NTS) versions of PHP."

[jrchamp]

Replying to bikecrazyy:

Makes sense, I changed it on my server to microtime(true) to give it a try. After a little load testing, the results were better and less errors were thrown, but there were still file access issues. See error log below.

The uniqueness difference between microtime(true) and uniqid() is governed by the precision configuration value. You should be able to use ini_set('precision', 16); to achieve the same behavior.

To avoid that complexity, I agree that uniqid() is easier to use and often sufficient:

Example:

<?php
    $temp_file_name = $context . 'temp-write-test-' . uniqid();

If uniqid()'s default behavior is not sufficient, you could enable the more_entropy optional parameter:

Example:

<?php
    $temp_file_name = $context . 'temp-write-test-' . uniqid('', true);

[bikecrazyy]

Replying to jrchamp:

To avoid that complexity, I agree that uniqid() is easier to use and often sufficient

If uniqid()'s default behavior is not sufficient, you could enable the more_entropy optional parameter

I load tested uniqid() without the more_entropy enabled and it seem to work fine. I didn't receive any file access issues so I think just using uniqid() should work.

Do you know how SVN works? If so could you make a patch? if not I can learn how to submit a patch for this.

Proposed fix:

<?php
      $temp_file_name = $context . 'temp-write-test-' . uniqid(); 

[jrchamp]

Use microsecond precision to generate a more unique temporary filename identifier in get_filesystem_method() and wp_tempnam()

[bikecrazyy]

@jrchamp So I think I spoke too soon, I let the load testing run over the weekend and it did throw some file access errors, I added the option for more_entropy and looks like its working I'll keep you posted to see if the load testing throws in errors but as of now nothing. I'll let this change bake for a week and let you know next Monday the results.

[dd32]

What on your system is calling get_filesystem_method() so much?

It seems very strange that it's being called so much that both a) it gets noticed, and b) that it clashes.

While we can increase the entropy of the filenames here greatly, it's not going to help with whatever bad plugin(s) you've got installed that are causing this.

[bikecrazyy]

Hi dd32,

I think it's important to note that we have all PHP errors/warnings being logged for our DEV and TEST sites. Since our TEST is a copy of PROD we decided turn on all errors for our PROD box for a few minutes and saw a ton PHP error messages that match what we saw in TEST but amplified. Which make sense, there is much more traffic hitting our PROD box versus or DEV and TEST.

I asked myself the same question you asked me, why is this method being called so much and why is it clashing. So what I did was zip up the whole plugins folder and did a text search with WinRAR to get a list of all the plugins calling the function get_filesystem_method(). I found four plugins that use the method and one of the plugins uses it five times. See screen shot below.

On the more_entropy, I'm not sure if this is the way to go right now but this is why I'm here so thanks for any help you can provide.

Screen shot of all the plugins with the get_filesystem_method() useage

[bikecrazyy]

Just and update, I haven't seen a issue yet with the since I enabled more_entropy over the last week

[jrchamp]

Use microsecond precision and some simple randomness (more_entropy) to generate a significantly more unique temporary filename identifier in get_filesystem_method() and wp_tempnam()

[bikecrazyy]

@dd32 will this make it to a release soon?

[dd32]

Replying to bikecrazyy:

@dd32 will this make it to a release soon?

I don't anticipate putting it into a point release, so 5.0 next year.

[dd32]

In 42224:

Filesystem: Use a more unique filename in wp_tempnam() and get_filesystem_method().
Using a filename which was generated from time() could cause two processes to try to use the same filename, causing unexpected behaviour.

Props jrchamp, bikecrazyy.
Fixes #42265.

[dd32]

The change in [42224] is slightly different from 42265_2.patch​

• wp_tempnam() only uses the basename of the $filename so the extra entropy of the microtime was being discarded. It's also suffixed with wp_generate_password(6) so just uniqid() is unique enough in that case.
• get_filesystem_method() previously used an extension-less file, and I'd prefer to keep that behaviour for now, so the . is replaced with - to ensure that the file has no extension. While I can't think of any issue it'd have caused, I'd rather not be generating temporary files with random numeric extensions.

[rogerlos]

As an example of a plugin combination which can trigger this in the existing codebase:

"All in One Event Calendar by Timely" hits this function (via WP_Filesystem at least once every time the plugin is called, to see if their cache directory is available. (Which seems like something which should only be done on install or periodically, but not my plugin...)

"PageBuilder by SiteOrigin" uses a lot of ajax calls when working with its tool in the editor, and can generate several calls in a very short period of time. If two calls are made within the same tick of the system clock, as it were, "All in One" will cause a PHP warning via this bug.

I haven't checked, but I can imagine the new editor for 5.0, being ajax based, may cause similar issues with plugins like AIOEC. Look forward to seeing this fix.