WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#25061 closed defect (bug) (fixed)

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

Reported by: DrProtocols Owned by:
Milestone: 3.6.1 Priority: normal
Severity: major Version: 3.6
Component: HTTP API Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

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)

Attachments (4)

stream_body_problem.txt (3.1 KB) - added by DrProtocols 7 years ago.
File shows data length discrepancies
stream_body_hack.txt (672 bytes) - added by DrProtocols 7 years ago.
Example hacked stream_body() for handling overloaded funds
25061.patch (932 bytes) - added by SergeyBiryukov 7 years ago.
25061.2.patch (930 bytes) - added by SergeyBiryukov 7 years ago.

Download all attachments as: .zip

Change History (17)

@DrProtocols
7 years ago

File shows data length discrepancies

@DrProtocols
7 years ago

Example hacked stream_body() for handling overloaded funds

#1 @nacin
7 years ago

  • Milestone changed from Awaiting Review to 3.6.1
  • Severity changed from normal to major

#2 @SergeyBiryukov
7 years ago

  • Description modified (diff)

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

Last edited 7 years ago by SergeyBiryukov (previous) (diff)

#3 @DrProtocols
7 years ago

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' ) ) { 

#4 @SergeyBiryukov
7 years ago

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

#5 @dd32
7 years ago

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

#6 @dd32
7 years ago

  • Keywords has-patch commit added

#7 follow-up: @dd32
7 years ago

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

#8 @dd32
7 years ago

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

#9 @dd32
7 years ago

  • Keywords has-patch commit removed
  • Resolution set to fixed
  • Status changed from new to closed

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!

#10 @dd32
7 years ago

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.

#11 @DrProtocols
7 years ago

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.

#12 in reply to: ↑ 7 ; follow-up: @SergeyBiryukov
7 years ago

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

#13 in reply to: ↑ 12 @dd32
7 years ago

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)

Note: See TracTickets for help on using tickets.