get_temp_dir should use the system tmp directory as first choice

[simonwheatley]

I believe WordPress should use the system tmp directory as first preference temp directory in get_temp_dir if WP_TEMP_DIR, is not defined. It just seems… odd… to use wp-content in preference to the proper, managed, temp directory.

Some historical context on introducing get_temp_dir: #5984

[simonwheatley]

Switch sys_get_temp_dir and WP_CONTENT_DIR within get_temp_dir

[scribu]

Agreed.

If WP_TEMP_DIR isn't defined, the first fallback should be sys_get_temp_dir(), not WP_CONTENT_DIR.

[nacin]

I tracked down this history in IRC with simonwheatley. The original order was introduced in #5984 and made sense at the time, but in [14016] an is_writeable() check was added to the result of sys_get_temp_dir(), at which point the order stopped making sense.

It makes sense to first check sys_get_temp_dir() as long as we are still keeping the is_writeable().

[dd32]

Does it make sense to use WP_CONTENT_DIR in preference to upload_tmp_dir here? I'd have thought that WP_CONTENT_DIR should be relegated to the absolute last preference?

[nacin]

I agree, seems like WP_CONTENT_DIR should go last.

[kurtpayne]

Patch updated with:

• Put WP_CONTENT_DIR last
• Use win_is_writable when host system is windows
• Don't save sys_get_temp_dir to static $temp var until it passes the is_writable check

[nacin]

Is win_is_writable() needed anymore? Seems like the ACL issues have been fixed since 5.0.x.

[kurtpayne]

The comments on ​this bug don't inspire confidence.

Official view on this problem is "wont fix", since we lack the time and inclination to mess with NT security API (it's an ugly beast), but would accept a patch that implements it.

[dd32]

I completely agree with switching the priorities here, however, the latest patch has a small logic error in the case of win_is_writable().

If win_is_writable() is designed to work around cases where is_writable() returns true when it can't actually write to a directory, the current logic goes as follows:
If ( ( is windows AND windows is writable ) OR is writable )
On affected windows hosts that would boil down to If ( ( 1 AND 0 ) OR 1 )

I'm not sure I understand the want to avoid writing to the $temp variable, as if a path that can't be written to is stored in it, It'll be overwritten by the next location test anyway, and failing all tests, overwritten with /tmp/. Micro-optimizations definitely isn't needed here, so that just seems to increase the complexity of the function.

I haven't tested on a Windows environment with ACL's in place yet, but attached is my take on the patch, Anyone see anything wrong with this approach?

[simonwheatley]

Replying to dd32:

Anyone see anything wrong with this approach?

The logic in the patch makes sense to me.

[kurtpayne]

Looks good to me.

[knutsp]

+1
wp-content easily becomes polluted by uploaded zip files from an upload that didn't complete (zip file not extracted and installed). They have to be deleted manually. This should not happen.

[dd32]

+1 wp-content easily becomes polluted by uploaded zip files from an upload that didn't complete (zip file not extracted and installed).

That sounds like a separate bug that should be reported if you can reliably duplicate it, the uploaded files should always be cleaned up (Only case I can think of right now would be a fatal or similar during the installation process).

Thanks for the sanity checks, I'll commit this later on

[knutsp]

Replying to dd32:

That sounds like a separate bug that should be reported if you can reliably duplicate it, the uploaded files should always be cleaned up (Only case I can think of right now would be a fatal or similar during the installation process).

I think it happens when there is a request timeout. The file, or part of the file, is uploaded, but then the process aborts. If this file is just left in a temp dir, then there is no fixable WordPress bug involved.

[dd32]

Tested OK on a Windows 7 box..

However, is_writable() works as expected for me on windows 7 with PHP 5.4.4, Denying write causes is_writable to return false, Allowing Write but not Delete causes both is_writable() and is_win_writable() to return true (although the latter makes a PHP Warning (Unable to delete the test file)). I'd be interested in finding the configurations which cause it to fail..

win_is_writable() will also need to move from wp-admin/includes/misc.php to wp-includes/functions.php, although core doesn't use get_temp_dir() outside of the admin, if it's available to plugins so it should work.

[dd32]

In [22008]:

Rearrange the order that we check for temporary directories in get_temp_dir(). This change causes us to use System temporary directories in preference to WP_CONTENT_DIR, for better windows compatibility, we use win_is_writable() as well. Props simonwheatley and kurtpayne for initial patches, See #20778

[dd32]

In [22009]:

Move win_is_writable() from wp-admin/includes to wp-includes so that it's always available for get_temp_dir(). Fixes #20778

[markoheijnen]

It seems it is broken on Mac OS since a lot of unit tests are broken now. Somehow /private/ got added.

[kirasong]

Looks like this is only happening because the test in question is realpath()ing the result.
Working with kurtpayne to fix the test.

[dd32]

Looks like this is only happening because the test in question is realpath()ing the result.

That sounds like something that someone would do to work around symlinks or something.

I'll also note this here, I am somewhat unconvinced that we *actually* need to use win_is_writable() at all in core, however, I'm unsure of the latest PHP version which is affected by the bugs which require it's use (I would not be surprised if PHP 5.2 is affected, while 5.3/5.4 are not).

Since this change means that a system directory outside of the normal web directories may be used, I thought it was the safest bet to use win_is_writable() to prevent any edge cases. A new ticket will need to circle back on it, but I'd really like to define the conditions which make it needed.

[kurtpayne]

Replying to dd32:

Looks like this is only happening because the test in question is realpath()ing the result.

The code in the #6821 patch is using realpath(). The test wasn't. On OS X, the /tmp folder is linked to /private/tmp and it was causing a mismatch on the assertion. The test was updated to call realpath() on both the $acutal an $expected before the assertion.

[ocean90]

Not sure if we want to fix it, but through the change a warning appears on updates.

Warning: is_dir() [function.is-dir]: open_basedir restriction in effect.
File(/tmp) is not within the allowed path(s): (
/var/www/web39/html/:
/var/www/web39/phptmp/:
/var/www/web39/files/:
/var/www/web39/atd/
) in /var/www/web39/html/wp-includes/functions.php on line 1394

See #12866 for background. Patch uses @is_dir.

[dd32]

Yup, we should fix warnings like that from safemode. It looks like that should be the only directory which needs the @is_dir() check.

[dd32]

In [22082]:

Silence a PHP Warning in Safe Mode from get_temp_dir(). Props ocean90. Fixes #20778

[dd32]

This could have caused ​http://wordpress.org/support/topic/unable-to-update-2?replies=1

Looking at the report, 'C:\WINDOWS\TEMP\/wordpress-latest.tmp' contains a \/ as a path delimiter, that condition seems to be causing the problem here.

[SergeyBiryukov]

Could not reproduce the "Missing archive file" error from the forum thread on Windows XP.

I could, however, reproduce the wrong path delimiter. 20778.2.patch​ fixes that.

[dd32]

I've seen windows fail with a double slash in a path , or \/ before, but I think it's limited to a certain filesystem or ACL set.

In this case, The fact that trailingslashit() and untrailingslashit() don't support windows paths seems like a good new ticket, but for now, that patch looks good to me. I don't have a windows system to test on right now, but will follow up later

[knutsp]

Created #22267

[dd32]

In 22331:

Prevent an issue on windows systems which can cause the temporary directory from get_temp_dir() from returning a path containing \/. Props to SergeyBiryukov for the initial patch. Fixes #20778

[dd32]

I really don't like that commit, but until #22267 is patched to fix untrailingslashit(), I'd prefer to avoid the potential issues which this could cause.

[Otto42]

Possibly related:
#22900