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

[dd32]

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.

[dd32]

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.

[erisds]

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 :)

[dd32]

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.

[dd32]

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

[erisds]

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 the "There may not be enough diskspace." suggestion of what the cause might be again :)

[dd32]

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.

[dd32]

attachment 16057.htttp.diff added

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

[wonderboymusic]

Refreshed against trunk

[wonderboymusic]

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

[wonderboymusic]

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

[dd32]

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..

[dd32]

#22881 was marked as a duplicate.

[dd32]

In 25303:

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

[nacin]

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

[nacin]

Replying to nacin:

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

#25259

[dd32]

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