Race Condition when using file_save_data FILE_EXISTS_REPLACE

file_copy() should not remove the source file. We need to introduce a temporary file, possibly in the same directory (because renames across partitions/mounts are not atomic).

This needs to be fixed in HEAD and then backported.

subscribing

The chmod() issue is covered by #443286: Windows File Handling: International characters break file handling, permissions don't translate.

+  if ($replace == FILE_EXISTS_REPLACE) {

Please add a comment explaining why we have two different code paths here.

The else part covering the two other modes could be extended with a fopen($file, 'x') call to ensure that existing files are never overwritten (another race condition). But that is probably a separate issue.

You may also add a comment mentioning that the unlink() call is necessary, because rename() cannot overwrite existing files on Windows. It would be nice if we could avoid the unlink() on platforms where rename() supports overwriting, though (one approach can be seen here: http://www.smarty.net/forums/viewtopic.php?t=6956&postdays=0&postorder=a...).

+    $temp_name = $destination . '-' . mt_rand();
+    if ($temp_name && @copy($source, $temp_name)) {

$temp_name will always evalute to TRUE here.

Could we choose a name that looks more "temporary"? E.g. add ".tmp" or something.
Elsewhere in core, e.g. in lock.inc we use uniqid(mt_rand(), TRUE) for generating a unique random identifier. But why not use drupal_tempnam() ? If there is a good reason, please add a comment explaining why.

Why does the testbot not like it?

Do the tests pass on your local machine? If they do, we should fix the testbot (or whatever is causing the tests to fail), rather than implementing workarounds.

Why not assign result in copy

++    if ($temp_name && $result = @copy($source, $temp_name)) {
+      // Try the rename opperation
+      if (!$result = @rename($temp_name, $destination)) {
+        // Unlink and try again for windows since rename on windows does not
+        // replace the file if it already exists.
+        @unlink($destination);
+        $result = @rename($temp_name, $destination);
+      }
+    }
-+    else {
-+      $result = FALSE;
-+    }

Is there any benchmarks about copy and rename?

+++ includes/file.inc	2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+  // Perform the replace operation. copy() is not atomic but rename() is.

It's not clear to me what "atomic" means in this context. The further description seems to try to explain parts of it, but overall it looks like the problem space that caused this (ugly) workaround is not properly explained.

+++ includes/file.inc	2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+  // Since there could be mutiple processes writing to the same file the best
+  // option is to create a temp file in the same directory and then rename it

1) Typo in "mutiple".

2) "temp" should be "temporary". (and elsewhere)

+++ includes/file.inc	2 Jul 2010 22:32:39 -0000
@@ -677,8 +677,38 @@ function file_unmanaged_copy($source, $d
+    // Get temp filename
...
+    // Place new contents in temp file
...
+      // Try the rename opperation
...
+    // Cleanup tempname if rename/copy failed

1) Missing trailing periods.

2) Typo in "opperation".

Powered by Dreditor.

Bumping and tagging per standard policy.

#41: drupal-818818-41-D7.patch queued for re-testing.

@andypost: Just out of curiosity: copy() / rename() benchmarks in my localhost: Seems that a single copy operation is slower than a rename operation, but a lot of consecutively copy operations are faster O.o

Benchmarks copy() & rename() functions:
-------------------------------------------------------------------------
1 copy operations in 0.0011789798736572 ms.
1 rename operations in 0.00067996978759766 ms.
rename() is 173.38709677419 times faster than copy().
-------------------------------------------------------------------------
10 copy operations in 0.0082869529724121 ms.
10 rename operations in 0.0071439743041992 ms.
rename() is 115.99919903885 times faster than copy().
-------------------------------------------------------------------------
100 copy operations in 0.052769184112549 ms.
100 rename operations in 0.068475961685181 ms.
copy() is 129.76505670266 times faster than rename().
-------------------------------------------------------------------------
1000 copy operations in 0.32604908943176 ms.
1000 rename operations in 1.1666119098663 ms.
copy() is 357.80253577751 times faster than rename().

Code used for benchmarking attached.

Ported to D8, NR for testbot.

#52: drupal-818818-52-file-save-D8.patch queued for re-testing.