WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 5 months ago

#39522 assigned defect (bug)

Avoidable PHP Warning thrown in _wp_handle_upload

Reported by: dglingren Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch needs-testing
Focuses: Cc:

Description

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 ) );
        }

Attachments (1)

39522.patch (1.6 KB) - added by achbed 10 months ago.
Reverse the order of existence and zero-length file tests. Also, add "is_readable" to the existence check.

Download all attachments as: .zip

Change History (16)

#1 @achbed
10 months ago

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
10 months ago

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

#2 @achbed
10 months ago

  • Keywords has-patch needs-testing added

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.

#3 @dglingren
10 months ago

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.

#4 @achbed
10 months ago

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

This ticket was mentioned in Slack in #core by achbed. View the logs.


10 months ago

#6 @dd32
10 months ago

  • Milestone changed from Awaiting Review to 4.8

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.

#7 @achbed
10 months ago

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.

#8 follow-up: @SergeyBiryukov
10 months ago

  • Version changed from trunk to 4.0

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

#9 in reply to: ↑ 8 @achbed
10 months ago

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!

#10 @klihelp
9 months ago

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

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 months ago

#13 @joemcgill
5 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by obenland. View the logs.


5 months ago

#15 @obenland
5 months ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.