WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

Last modified 7 months ago

#16057 closed defect (bug) (fixed)

download_url() error checking fails to notice that the file wasnt correctly witten to disk

Reported by: dd32 Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.0
Component: HTTP API Keywords: 3.3-early has-patch needs-testing commit
Focuses: Cc:

Description

When upgrading/installing plugins, themes and WordPress download_url() is called to download the package to a temporary file.

At present, the return value of fwrite() is not checked, the result can be that the file is not written to disk correctly and subsequently, the Zip extraction fails.

This appears as if it can be caused by the user running out of disk space, as seen here: http://erisds.co.uk/wordpress/wordpress-automatic-upgrades-one-of-the-pitfalls

I'm attaching a patch which appears to fix it for me, however, As I do not have a setup where I can enforce a quota I cannot test the saving of the file without fudging the return value of fwrite() to something lower than expected.

Attachments (6)

16057.diff (650 bytes) - added by dd32 3 years ago.
16057.2.diff (2.8 KB) - added by dd32 3 years ago.
16057.htttp.diff (3.4 KB) - added by dd32 3 years ago.
16057.3.diff (3.6 KB) - added by wonderboymusic 15 months ago.
16057.4.diff (2.5 KB) - added by wonderboymusic 9 months ago.
16057.5.diff (2.4 KB) - added by dd32 7 months ago.

Download all attachments as: .zip

Change History (24)

dd323 years ago

comment:1 dd323 years ago

attachment 16057.diff added

The text "Could not write to the temporary file. There may not be enough diskspace." is simply a suggestion, however to me, seems like it's the most likely scenario of this error check being hit.

comment:2 erisds3 years ago

I've setup an install of 3.0.1 in an environment with only a small amount of disk space on the same server as I originally encountered the issue. It's a fresh install of 3.0.1 no plugins etc.

As reported in the blog post, there are two error messages I've encountered.

  1. Call to undefined function update_core() ... etc
  2. Incompatible Archive.: PCLZIP_ERR_BAD_FORMAT (-10),... etc

From further testing it appears that the following is happening:

Case 1: If there is not enough room to unpack the .tmp file archive from wp-content to wp-content/upgrade successfully and do the necessary processing to upgrade error 1 is thrown.

Case 2: If there is not enough room to even write the .tmp file to the wp-content folder, error 2 is thrown

So after receiving error 1 and then trying to auto update a few more times, eventually error 2 is thrown because WordPress can no longer even upload a .tmp file as all the leftover space has been used up by the partial .tmp files.

The applied patch fixes error 2 - now we get a nice helpful notice telling us we've probably run out of space. Error 1 still occurs and still doesn't indicate disk space being the issue.

I will investigate further to see if I can figure out where error 1 comes from, but I imagine people who know the upgrade process already will have a better chance :)

If anyone want's access to my test env that can be arranged :)

Last edited 3 years ago by erisds (previous) (diff)

comment:3 dd323 years ago

Thanks for testing it erisds.

Can I ask, Do you know which Upgrade transprot is in use? FTP or Direct? Simplest way, Is if you're using FTP, you'll be asked for the credentials upon attempting to upgrade, Direct will just jump straight into the upgrade.

I've found a similar error in the Direct class, but it shouldn't be the cause of the exact error, but i'll patch it in a moment anyway and upload a new patch for testing.

dd323 years ago

comment:4 dd323 years ago

attachment 16057.2.diff added

  • Checks to make sure temporary files are written correctly before being transferred to the FTP Server
  • Checks to see files written directly finish writing properly
  • Note: file_put_contents() seems to be unaffected, returns false on failure in all cases, (int)$bytes_written on success. Given it's PHP5-only, it'd probably be wise to switch to that if at all possible in some locations

comment:5 erisds3 years ago

Dion,

Am definitely using the Direct upgrade. I've seen the FTP version before on other setups, but my setup has SuPHP so it always works via the direct method.

The patch you submitted has changed the undefined function call error to:

Unpacking the update…
Could not copy file.: /home/erisds/public_html/wptest/wp-content/upgrade/wordpress-3.tmp/
Installation Failed

So the error message is now reporting the actual problem, but it would be nice to have a suggestion of what the problem might be again :)

Version 0, edited 3 years ago by erisds (next)

comment:6 dd323 years ago

So the error message is now reporting the actual problem, but it would be nice to have the "There may not be enough diskspace." suggestion of what the cause might be again :)

I do tend to agree, but that's the error message that should've been coming up, so all is mostly good now. The error message is a bit hard to change in this instance, as the WP_Filesystem class's are only designed to return true|false, not a specific error on why it's failed.

Now that this is tracked down, at least I can get the changes put in and work out giving some nicer error messages for the various failure cases.

dd323 years ago

comment:7 dd323 years ago

attachment 16057.htttp.diff added

  • Implements checking that the written file size matches the content-length header if set.

comment:8 ocean903 years ago

  • Keywords 3.3-early added; 3.2-early removed

wonderboymusic15 months ago

comment:9 wonderboymusic15 months ago

  • Milestone changed from Future Release to 3.6

Refreshed against trunk

comment:10 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

wonderboymusic9 months ago

comment:11 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

Increased sanity checking of file writes via HTTP transports - refreshed against trunk

comment:12 wonderboymusic8 months ago

  • Keywords needs-refresh added; has-patch needs-testing removed
  • Owner set to dd32
  • Status changed from new to assigned

My refresh no longer works - @dd32 is best-suited to know what this ticket needs

dd327 months ago

comment:13 dd327 months ago

  • Component changed from Upgrade/Install to HTTP
  • Keywords has-patch needs-testing commit added; needs-refresh removed

Attachment attachment:16057.5.diff​ added

This checks that fwrite() says that it successfully wrote the full block to the file - In the event that the disk is full, this *should* return the number of bytes written, which should be smaller than the full block size..

This is untested (other than forcing the written block size to be smaller), but logically works based on php's behaviour, and seems like the correct behaviour for WP_HTTP.

If anyone has an idea on how to fill a disk and reliably leave a few KB free for testing..

comment:14 dd327 months ago

#22881 was marked as a duplicate.

comment:15 dd327 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 25303:

WP_HTTP: When streaming to file, ensure that fwrite() suceeds and correctly writes the file to disk. Fixes #16057

comment:16 follow-up: nacin7 months ago

Can [25303] be affected by multibyte overloading of strlen()?

comment:17 in reply to: ↑ 16 nacin7 months ago

Replying to nacin:

Can [25303] be affected by multibyte overloading of strlen()?

#25259

comment:18 dd327 months ago

In 25348:

Switch WP_HTTP over to using the mbstring.func_overload helper functions. This change moves the check from within the Streaming-handling function to wrap the individual request, this fixes it for both cURL and Streams and any future changes to the transports which use strlen() on binary data. See #25259 See #16057

Note: See TracTickets for help on using tickets.