Plugin/Theme/Core Updates Fail When Curl Used and String Function Overloading Configured

[DrProtocols]

When downloading an update in the form of a zip file the update consistently fails with a failure to find the end of central dir record when unpack is attempted. For example:

Downloading update from http://downloads.wordpress.org/plugin/addthis.3.5.1.zip…

Unpacking the update…

The package could not be installed. PCLZIP_ERR_BAD_FORMAT (-10) : Unable to find End of Central Dir Record signature

The download is using curl.

To reproduce add the following to PHP configuration:

mbstring.func_overload = 2;

which enables str*() function overloading.

The problem is caused by the new stream_body() function in wp-includes/class-http.php which uses the strlen() function to simply return the length of data written but when overloaded with the multi-byte function the count is almost certainly incorrect when the data is binary data such as part of a zip file download. Because for a chunk the function returns a count different from that expected by curl it terminates the transfer as "completed" at that point which appears as a successful outcome. But of course the downloaded file is incomplete so when pclzip tries to unzip it the above failure results.

Attached are two files:
stream_body_problem.txt shows a _working_ case where the "written" value is the value returned by fwrite() against the "string length" value which is the value according to strlen()
stream_body_hack.txt shows a hacked function that handles the case where function overloading is enabled (not saying this is the way to do it but just to illustrate)

[DrProtocols]

File shows data length discrepancies

[DrProtocols]

Example hacked stream_body() for handling overloaded funds

[SergeyBiryukov]

Turned stream_body_hack.txt​ into a patch: 25061.patch​.

We have a similar check in _unzip_file_pclzip(): tags/3.6/wp-admin/includes/file.php#L669​ (introduced in [17592]) and POMO_Reader: tags/3.6/wp-includes/pomo/streams.php#L17​ (introduced in [11626]).

Related: #18007 (explores other options to deal with mbstring.func_overload).

[DrProtocols]

Thanks for pushing this one through - slight correction required to first line of patch:

if ( ( ini_get( 'mbstring.func_overload' ) & 2 ) && function_exists( 'mb_internal_encoding' ) ) { 

[SergeyBiryukov]

Thanks, fixed the copy/paste error in 25061.2.patch​.

[dd32]

Is there some reason why we shouldn't just set this on environment initialisation? (ie. when we attack magic quotes)

It seems like this is something that would cause some pretty unexpected results for some plugins.

The patch looks good for 3.6.1 though

[dd32]

In 25051:

WP_HTTP: Curl: When using Stream-to-file on servers using mbstring.func_overload ensure that the file is written out correctly. Props DrProtocols. See #25061 for trunk

[dd32]

In 25052:

WP_HTTP: Curl: When using Stream-to-file on servers using mbstring.func_overload ensure that the file is written out correctly. Props DrProtocols. See #25061 for 3.6

[dd32]

Is there some reason why we shouldn't just set this on environment initialisation? (ie. when we attack magic quotes)

It seems like this is something that would cause some pretty unexpected results for some plugins.

Took a look, and if we do that, any plugin which uses mb_strlen() would result in mb_strlen() returning the same as strlen() (ie. 24char instead of 8char for a Japanese string). Ultimately, this means that we have to switch to ascii and back again whenever we're using binary data.

Calling this fixed. Thanks DrProtocols!

[dd32]

Note, We also use strlen() in stream_headers(), however, HTTP Headers have to be ASCII, so we don't need to switch back and forth there.

[DrProtocols]

Thanks guys.

Just a note on http headers - technically speaking, according to RFC2616 (​http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.2) for HTTP/1.1 (but same applies to HTTP/1.0) arbitrary 8-bit data can appear in specific types of header fields as comment text (Server, User-Agent and Via being the "standard" headers where comment is allowed). But in general, although IANA maintains a Message Headers registry for permanent and provisional headers there is nothing that stops anything being added as a header provided it obeys the header syntax - if the recipient doesn't recognize the header it should simply ignore it - so any arbitrary header that might contain 8 bit data _could_ be present.

Whether this happens in practice I can't say but it is possible. If the stream_headers() function has been around for a while and no issue has arisen there then either this hasn't arisen at all or the value actually returned by strlen() doesn't have any material impact on the operation of Curl.

[SergeyBiryukov]

Replying to dd32:

In 25051:

Is there a reason for function_exists( 'ini_get' )? We didn't check if it exists in [11626] or [17592].

[dd32]

Replying to SergeyBiryukov:

Replying to dd32:

In 25051:

Is there a reason for function_exists( 'ini_get' )? We didn't check if it exists in [11626] or [17592].

I was simply matching the style used elsewhere in the file, where we check for ini_get() first.

That being said, it seems that we don't check for it's existence in most cases in core, so we can probably skip that (And remove it from elsewhere in the file now)