Avoidable PHP Warning thrown in _wp_handle_upload

[dglingren]

When something goes wrong with an upload, /wp-admin/includes/file.php throws a PHP Warning at line 323:

PHP Warning: filesize(): stat failed for x in C:\Users\dlingren\Documents\My Data\Data Files\Web Design\VirtualHosts\mladev\wp-admin\includes\file.php on line 323

The error is handled by the logic immediately following the offending line. Changing the call to "@filesize( ..." would avoid the PHP Warning. Here's the code:

<?php
        $test_file_size = 'wp_handle_upload' === $action ? $file['size'] : filesize( $file['tmp_name'] );
        // A non-empty file will pass this test.
        if ( $test_size && ! ( $test_file_size > 0 ) ) {
                if ( is_multisite() ) {
                        $error_msg = __( 'File is empty. Please upload something more substantial.' );
                } else {
                        $error_msg = __( 'File is empty. Please upload something more substantial. This error could also be caused by uploads being disabled in your php.ini or by post_max_size being defined as smaller than upload_max_filesize in php.ini.' );
                }
                return call_user_func_array( $upload_error_handler, array( &$file, $error_msg ) );
        }

[achbed]

That's odd. We're checking for content size before we check for file existence. We should probably reverse the order of the tests.

[achbed]

Reverse the order of existence and zero-length file tests. Also, add "is_readable" to the existence check.

[achbed]

The attached patch reverses the order of the tests in order to catch issues with missing/unreadable files before checking the file length. I've also added a "is_readable" check to the existence check to catch permission issues. A second set of eyes (and someone who can run the unit tests) would be greatly appreciated.

[dglingren]

Thank you for your quick action on this ticket. I applied the patch to my test system and ran a few tests. Trying to process an invalid "tmp_name" now returns the expected "Specified file failed upload test." error and no longer throws a PHP Warning. The patch looks good to me.

[achbed]

Ok thanks. This looks like it's been an issue since at least 2014, so it's not a candidate for 4.7.2.

[dd32]

I'm not sure the is_readable() change from is_file() is needed, but I don't see the harm in it.

Marking this for 4.8 as those two blocks really do seem to be reversed.

[achbed]

The only reason I added that is to catch situations where a sideloaded file is in a folder that can be read, but is not readable itself due to a permissions issue. is_file just validates that the file exists and is a "normal" file, not that it's also readable. We could add a separate ticket for that, but I figured that this would be an applicable spot to add that check.

[SergeyBiryukov]

Technically introduced in [29209], but the related code goes all the way back to [2979] and [8048].

[achbed]

Replying to SergeyBiryukov:

Technically introduced in [29209], but the related code goes all the way back to [2979] and [8048].

Wow that's an old one. Good catch @dglingren!

[klihelp]

This php warning comes up, while can't upload images into the media library.

[SergeyBiryukov]

In 42525:

Media: In _wp_handle_upload(), check if the file was properly uploaded before checking its size.

Props achbed, dglingren.
Fixes #39522.

[johnbillion]

[42525] should be merged into 5.0

[pento]

It's okay if this one waits until 5.1.