copy(), move(), and move_uploaded_file() ignores return value of stream_flush()

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Seems during the rerolling we lost test coverage and possibly other things.

So this needs work.

Reread the comments in #33 and the test file change added in @22 was out of scope and removed.

So adding the tests tag as it will need it's own

Test were a good idea. Found a couple bugs, including the fact we were not validating with is_uploaded_file() before directly writing the file.

Attached a fail only (tests) patch and a combined updated fix patch.

Code standard fixes.

The last submitted patch, 44: 3057565-fflush-ignored-44-fail.patch, failed testing. View results →

I apologize I should of done this before.

This could use an issue summary update. Mainly for the proposed solution.

Tried to update but could use a once over.

As far as a code review goes.

On a drupal 10.1 Umani install
I added this code to a preprocess hook

  \Drupal::service('file_system')->copy('public://borscht-with-pork-ribs-umami.jpg', 'dummy-flush-fail://test-move.txt');

The page throws a fatal error

Drupal\Core\File\Exception\FileWriteException: The specified file 'public://borscht-with-pork-ribs-umami.jpg' could not be copied to 'dummy-flush-fail://test-move_2.txt'. in Drupal\Core\File\FileSystem->copy() (line 330 of core/lib/Drupal/Core/File/FileSystem.php).

In the logs I see 2

The specified file 'public://borscht-with-pork-ribs-umami.jpg' could not be copied to 'dummy-flush-fail://test-move_2.txt'.

and

Drupal\Core\File\Exception\FileWriteException: The specified file 'public://borscht-with-pork-ribs-umami.jpg' could not be copied to 'dummy-flush-fail://test-move_2.txt'. in Drupal\Core\File\FileSystem->copy() (line 330 of /var/www/html/web/core/lib/Drupal/Core/File/FileSystem.php).

Could you confirm that's the desired behavior?

Thanks!

Could you confirm that's the desired behavior?

Yes, the API spec for copy() and move() only permits exceptions for indicating a failure.

   * @return string
   *   The path to the new file.
   *
   * @throws \Drupal\Core\File\Exception\FileException
   *   Implementation may throw FileException or its subtype on failure.
   */

Updated IS.

Thanks!

The last submitted patch, 44: 3057565-fflush-ignored-44-fail.patch, failed testing. View results →

The last submitted patch, 44: 3057565-fflush-ignored-44-fail.patch, failed testing. View results →

I only looked at the comments in the patch.

1. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -73,16 +74,31 @@ public function __construct(StreamWrapperManagerInterface $stream_wrapper_manage
   +        // File was not uploaded through post, reject move request.
   

   Shouldn't 'post' be capitalized?
   I suggest that something like this is easier to follow,
   "Files not uploaded via a POST request are not moved."

2. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -297,17 +313,37 @@ public function tempnam($directory, $prefix) {
   +    if (is_a($wrapper, StreamWrapperInterface::class)) {
   ...
   +        // If the copy failed and realpaths exist, retry the operation using
   +        // them instead.
   ...
   +        // If the copy failed and realpaths exist, retry the operation using
   +        // them instead.
   

   realpaths should be 'real paths' here. We can make this simpler two

   "When the copy fails retry using the real paths."

3. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -742,4 +790,45 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0)
   +   * PHP convenience functions copy(), move_uploaded_file(), etc do not
   

   This paragraph is not wrapped correctly and it is not easy to follow. Maybe something along these lines,
   The PHP file write functions, such as copy() and move_uploaded_file(), do not check that the write buffer is flushed. To work around that we write the file and flush the destination. A copy succeeds when a flush of the destination is successful.

   Just a suggestion,

4. +++ b/core/lib/Drupal/Core/File/FileSystem.php
   @@ -742,4 +790,45 @@ protected function doScanDirectory($dir, $mask, array $options = [], $depth = 0)
   +    $result = FALSE;
   

   Not needed, overwritten 3 lines later.

5. +++ b/core/modules/file/tests/file_test/src/StreamWrapper/DummyFlushFailsStreamWrapper.php
   @@ -0,0 +1,44 @@
   + * Helper class for testing the stream wrapper registry.
   + *
   + * Dummy remote stream wrapper implementation (dummy-remote://).
   + *
   + * Basically just the public scheme but not returning a local file for realpath.
   

   Let's explain what this is in the summary line.

   Provides a dummy remote stream wrapper.
   
   This is the same as the public:// scheme but does not return a local file for realpath.

6. +++ b/core/modules/file/tests/src/Functional/SaveUploadTest.php
   @@ -676,6 +676,37 @@ public function testDrupalMovingUploadedFileError() {
   +   * Tests for error when flush fails.
   

   Let's keep this in the style of others in the file.
   * Tests for log entry when flush of destination fails.

Addressed the comment #51
Attached patch against Drupal 10.1.x

I’m going to needs-work this on core/modules/file/tests/file_test/src/StreamWrapper/DummyFlushFailsStreamWrapper.php comments.

I copied that all from another test file and failed to properly edit the comment to indicate that this wrappers job is to always fail on stream_flush. Oversight on my part

I also have a question: currently I have the code only attempting this for managed streamWrappers (Aka public://, private://). This is probably because I originally copied this from a module that was only suppose to work for a specific single class instead of all of core. Since this is a core level fix for we want this to also work for ‘var/www/html/etc’ ?

I do have one outstanding item I’ve been trying to find the code for php rename() to make sure it’s not susceptible to the same fault, tests appear to indicate it’s not however I would really like to see the source code to make sure we are not seeing a separate failure that just coincidently triggers rename() to error out hiding that this is not properly tested.

Tracked down the code for php rename().

At a high level it relies on the return code for the wrapper rename() function. How much we can trust this depends on the wrapper.

For local files we AFAICT end up in plain_wrapper.c which relies on essentially calling libc rename(). A libc rename() is generally safe as its a change to a file table without moving any actual data, however if the rename fails such as crossing mount points plain_wrapper.c falls back to php_copy_file() which does not validate on stream_flush().

With the current patch as written there is still an edge case where data can be lost for local disks (and unknown risk for other streamWrapper)

The downside of converting FileSystem::move() to not attempt a php rename() first would be that for 'same disk' files we would significantly increase the 'cost' to move the file since it will take a full read and write to disk instead of just a file table update.

References:
https://github.com/php/php-src/blob/bf036fa2a306b86ae37feff7703c34296b81... (rename)
https://github.com/php/php-src/blob/bf036fa2a306b86ae37feff7703c34296b81... (php_copy_file)
https://github.com/php/php-src/blob/bf036fa2a306b86ae37feff7703c34296b81... (plain file rename)

Reroll patch from #52 for Drupal 10.2.5

Reroll patch from #52 for Drupal 10.3.1