#42265 closed defect (bug) (fixed)
get_filesystem_method() isn't unique enough
Reported by: | bikecrazyy | Owned by: | dd32 |
---|---|---|---|
Milestone: | 5.1 | Priority: | normal |
Severity: | normal | Version: | 2.5 |
Component: | Filesystem API | Keywords: | has-patch |
Focuses: | administration, performance | Cc: |
Description
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.
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');
Attachments (2)
Change History (19)
#2
in reply to:
↑ 1
;
follow-up:
↓ 3
@
7 years ago
Replying to pross:
Why not use
microtime()
instead oftime()
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()
#3
in reply to:
↑ 2
;
follow-up:
↓ 4
@
7 years ago
Replying to bikecrazyy:
Replying to pross:
Why not use
microtime()
instead oftime()
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.
#4
in reply to:
↑ 3
;
follow-up:
↓ 5
@
7 years ago
I think what @pross is saying is that
uniqid()
usesmicrotime()
internally, so rather than appenduniqid()
totime()
, it makes more sense to remove the redundanttime()
and to instead use eithermicrotime(true)
oruniqid()
. 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.
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."
#5
in reply to:
↑ 4
;
follow-up:
↓ 6
@
7 years ago
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);
#6
in reply to:
↑ 5
@
7 years ago
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 themore_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();
@
7 years ago
Use microsecond precision to generate a more unique temporary filename identifier in get_filesystem_method() and wp_tempnam()
#8
@
7 years ago
@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.
#9
@
7 years ago
- Owner set to dd32
- Status changed from new to accepted
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.
#10
@
7 years ago
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
#11
@
7 years ago
Just and update, I haven't seen a issue yet with the since I enabled more_entropy over the last week
@
7 years ago
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()
#13
in reply to:
↑ 12
@
7 years ago
- Milestone changed from Awaiting Review to 5.0
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.
#15
@
7 years ago
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 withwp_generate_password(6)
so justuniqid()
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.
#16
@
7 years ago
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.
Why not use
microtime()
instead oftime()