Project

General

Profile

Actions

Bug #8182

closed

XMLRPC request fails with "Wrong size. Was 31564, should be 1501"

Added by tsagadar (Marcel Mueller) almost 11 years ago. Updated almost 10 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-darwin12.2.1]
[ruby-core:53811]

Description

Since upgrading to Ruby 2.0.0p0 we can no longer use MailChimp through the Hominid gem. Hominid relies on the Ruby XMLRPC client to access MailChimp.

Debugging the issues revealed the following problem: net/http/response.rb transparently deflates the response body, removes the "content-encoding" response header (response.rb:255), but does not adjust the "content-length" header accordingly. This makes xmlrpc/client.rb:506 raise the error, that the response body and the declared length in "content-length" does not match.

I propose a high priority for this issue for two reason:

  • The problem should occur whenever Ruby XMLRPC is used to access a service that supports content encoding with "deflate", "gzip", or "x-gzip"
  • I don't see a workaround to this problem that could be used

Files

308.patch (3.37 KB) 308.patch zzak (zzak _), 05/20/2013 07:37 AM
net.http.bug8182.patch (2.98 KB) net.http.bug8182.patch drbrain (Eric Hodel), 06/12/2013 08:36 AM
xmlrpc_client.rb.bug8182.patch (903 Bytes) xmlrpc_client.rb.bug8182.patch drbrain (Eric Hodel), 06/12/2013 08:46 AM

Updated by hsbt (Hiroshi SHIBATA) almost 11 years ago

  • Category set to core
  • Assignee set to drbrain (Eric Hodel)
  • Priority changed from Normal to 5
  • Target version set to 2.1.0

Updated by tsagadar (Marcel Mueller) almost 11 years ago

Sample code to reproduce the issue using Flickr. With 1.9.3 this gives "Invalid API key" which is fine, but 2.0.0 returns the error of the size mismatch.

require 'xmlrpc/client'

server = XMLRPC::Client.new('api.flickr.com', '/services/xmlrpc/', 80)
s = server.call('flickr.test.echo')

Updated by zzak (zzak _) almost 11 years ago

I've attached Duncan's patch from github, so we can close the pull request.

Updated by zzak (zzak _) almost 11 years ago

  • Assignee changed from drbrain (Eric Hodel) to kou (Kouhei Sutou)

Updated by kou (Kouhei Sutou) almost 11 years ago

I confirmed the problem. I consider how to resolve this problem. Please wait.
I don't like that response header is changed in Inflater. It seems that header should be changed by response itself.

Here is a workaround:

require 'xmlrpc/client'

server = XMLRPC::Client.new('api.flickr.com', '/services/xmlrpc/', 80)
server.http_header_extra = {"accept-encoding" => "identity"} # <= add this line (workaround)
s = server.call('flickr.test.echo')

Updated by drbrain (Eric Hodel) almost 11 years ago

=begin
Consider this example:

http.request req do |res|
expected = res['Content-Length'] # size of encoded content
actual = 0

res.read_body { |chunk| actual += chunk.bytesize }

raise 'too short' unless expected == actual

end

Here the user stores the Content-Length before Net::HTTP can reads the decoded body, so altering the Content-Length may not help fix this class of bug generally (but will for XMLRPC).

An alternate is to only check Content-Length when Content-Encoding is set, but Net::HTTP deletes the Content-Encoding header when it begins to decode the body. (So the user would need to retrieve the Content-Encoding before reading the body).

However this is fixed we need to document the recommended way to perform this type of integrity check.
=end

Updated by drbrain (Eric Hodel) almost 11 years ago

This patch updates the 308 patch to update the Content-Length in Response instead of Inflater and fixes the variable name ("deflated length" is the original value, but only when deflate-encoded). It also adds documentation describing when the Content-Length will be changed so the user can test properly.

We will need Naruse's OK to commit it, he rejected changing Content-Length in #6492: https://bugs.ruby-lang.org/issues/6492#note-10

Updated by drbrain (Eric Hodel) almost 11 years ago

Here is a possible patch that only checks Content-Length if the Content-Encoding was handled by Net::HTTP. Unfortunately I don't have an XMLRPC service available to test it against.

Updated by kou (Kouhei Sutou) almost 11 years ago

drbrain (Eric Hodel) wrote:

Consider this example:

http.request req do |res|
expected = res['Content-Length'] # size of encoded content
actual = 0

res.read_body { |chunk| actual += chunk.bytesize }

raise 'too short' unless expected == actual

end

Here the user stores the Content-Length before Net::HTTP can reads the decoded body, so altering the Content-Length may not help fix this class of bug generally (but will for XMLRPC).

I think that we don't care about the case. In the case, the user must handle Content-Encoding by himself. The user should disable auto-decode feature.

I think that we care about the following case (this is XMLRPC::Client case):

http.request req do |res|
actual = 0

res.read_body { |chunk| actual += chunk.bytesize }

expected = res['Content-Length'] # size of encoded content
raise 'too short' unless expected == actual

end

In this case, we already deleted Content-Encoding when the user retrieves Content-Length. So the user should be able to handle the body as decoded content. In this point of view, Content-Length should be the length of decoded content. duncan's patch is this point of view.

I like duncan's approach but don't like the details as I mentioned...

Updated by kou (Kouhei Sutou) almost 11 years ago

drbrain (Eric Hodel) wrote:

This patch updates the 308 patch to update the Content-Length in Response instead of Inflater and fixes the variable name ("deflated length" is the original value, but only when deflate-encoded). It also adds documentation describing when the Content-Length will be changed so the user can test properly.

I think that we should delete Content-Length between deleting Content-Encoding and yielding the first chunk when both Content-Encoding and Content-Length are specified. I think the following HERE is a good location

  if clen
    # HERE
    @socket.read clen, dest, true   # ignore EOF
    return
  end

but we don't have information whether Content-Encoding is handled at the location... I don't like the following code because the code ignores abstraction by inflater.

  if clen
    self.delete "content-length" is @socket.is_a?(Inflater)
    @socket.read clen, dest, true   # ignore EOF
    return
  end

Umm...

Updated by kou (Kouhei Sutou) almost 11 years ago

drbrain (Eric Hodel) wrote:

Here is a possible patch that only checks Content-Length if the Content-Encoding was handled by Net::HTTP. Unfortunately I don't have an XMLRPC service available to test it against.

I'm sorry but I don't like it. It is a bit tricky.
I think that Net::HTTP should resolve this problem rather than XMLRPC has a workaround for this problem.

Updated by kou (Kouhei Sutou) almost 11 years ago

  • Assignee changed from kou (Kouhei Sutou) to naruse (Yui NARUSE)

I think that this problem should be fixed in net/http. So I change the assigned to naruse.

Updated by naruse (Yui NARUSE) almost 11 years ago

Maybe I misunderstood, why is the assertion is needed?
I feel it should be simply removed.

Moreover chunked encoded response doesn't have Content-Length header.
So it sounds nonsense.

Updated by duerst (Martin Dürst) almost 11 years ago

Checking Content-Length can help detect cases where the connection is
closed prematurely. I'm not sure how important this is in the
application where the error was discovered, but it's well known as a
problem in HTTP in general that premature closed connections are
difficult to detect.

Regards, Martin.

On 2013/06/14 19:03, naruse (Yui NARUSE) wrote:

Issue #8182 has been updated by naruse (Yui NARUSE).

Maybe I misunderstood, why is the assertion is needed?
I feel it should be simply removed.

Moreover chunked encoded response doesn't have Content-Length header.
So it sounds nonsense.

Bug #8182: XMLRPC request fails with "Wrong size. Was 31564, should be 1501"
https://bugs.ruby-lang.org/issues/8182#change-39928

Author: tsagadar (Marcel Mueller)
Status: Assigned
Priority: High
Assignee: naruse (Yui NARUSE)
Category: core
Target version: current: 2.1.0
ruby -v: ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-darwin12.2.1]
Backport:

Since upgrading to Ruby 2.0.0p0 we can no longer use MailChimp through the Hominid gem. Hominid relies on the Ruby XMLRPC client to access MailChimp.

Debugging the issues revealed the following problem: net/http/response.rb transparently deflates the response body, removes the "content-encoding" response header (response.rb:255), but does not adjust the "content-length" header accordingly. This makes xmlrpc/client.rb:506 raise the error, that the response body and the declared length in "content-length" does not match.

I propose a high priority for this issue for two reason:

  • The problem should occur whenever Ruby XMLRPC is used to access a service that supports content encoding with "deflate", "gzip", or "x-gzip"
  • I don't see a workaround to this problem that could be used

Updated by naruse (Yui NARUSE) almost 11 years ago

duerst (Martin Dürst) wrote:

Checking Content-Length can help detect cases where the connection is
closed prematurely. I'm not sure how important this is in the
application where the error was discovered, but it's well known as a
problem in HTTP in general that premature closed connections are
difficult to detect.

By default Net::HTTP#read wait to read content of the size which is specified with CONTENT_LENGTH, or chunked encoding.
Therefore such situation does not exist.

Updated by Eregon (Benoit Daloze) over 10 years ago

naruse (Yui NARUSE) wrote:

Maybe I misunderstood, why is the assertion is needed?
I feel it should be simply removed.

Moreover chunked encoded response doesn't have Content-Length header.
So it sounds nonsense.

I agree, it does not seem XMLRPC's job to check Content-Length.

Could we please solve this issue promptly?
Such a bad bug should not be left opened for long,
it encourages users to use workarounds like above which is really not a proper solution.

Updated by naruse (Yui NARUSE) over 10 years ago

  • Category changed from core to lib
  • Assignee changed from naruse (Yui NARUSE) to kou (Kouhei Sutou)
  • Priority changed from 5 to Normal
  • Backport set to 1.9.3: DONTNEED, 2.0.0: REQUIRED

The check should be removed as following:

diff --git a/lib/xmlrpc/client.rb b/lib/xmlrpc/client.rb
index ced3d01..6aa7c1f 100644
--- a/lib/xmlrpc/client.rb
+++ b/lib/xmlrpc/client.rb
@@ -509,8 +509,6 @@ module XMLRPC # :nodoc:
       expected = resp["Content-Length"] || "<unknown>"
       if data.nil? or data.bytesize == 0
         raise "Wrong size. Was #{data.bytesize}, should be #{expected}"
-      elsif expected != "<unknown>" and expected.to_i != data.bytesize and resp["Transfer-Encoding"
-        raise "Wrong size. Was #{data.bytesize}, should be #{expected}"
       end

       parse_set_cookies(resp.get_fields("Set-Cookie"))

Updated by bbuchalter (Brian Buchalter) over 10 years ago

Just a reminder, this fix is not not yet in 2.1.0 rc1.
https://github.com/ruby/ruby/blob/v2_1_0_preview1/lib/xmlrpc/client.rb?source=c#L509

Would love to see it in there!

Updated by joevandyk (Joe Van Dyk) over 10 years ago

I'm runing into this same problem on Ruby 2.0 p247.

RuntimeError: Wrong size. Was 157, should be 134
/usr/local/stow/ruby-2.0.0-p247/lib/ruby/2.0.0/xmlrpc/client.rb:506:in do_rpc' /usr/local/stow/ruby-2.0.0-p247/lib/ruby/2.0.0/xmlrpc/client.rb:281:in call2'
/usr/local/stow/ruby-2.0.0-p247/lib/ruby/2.0.0/xmlrpc/client.rb:262:in call' /mnt/data/tanga/current/bundler/ruby/2.0.0/gems/hominid-3.0.4/lib/hominid/api.rb:32:in method_missing'
/mnt/data/tanga/current/programs/mailchimp/mailchimp.rb:28:in `perform'

What's the best way for me to fix this problem now?

Updated by mat (Mathieu Arnold) about 10 years ago

Today I upgraded a ruby app that uses xmlrpc/client from 1.9.3-p374 to 2.0.0-p353, still not fixed in it…

Updated by hsbt (Hiroshi SHIBATA) about 10 years ago

  • Target version changed from 2.1.0 to 2.2.0

Updated by naruse (Yui NARUSE) almost 10 years ago

As I wrote, the check is useless. It should be removed.

Updated by naruse (Yui NARUSE) almost 10 years ago

  • Backport changed from 1.9.3: DONTNEED, 2.0.0: REQUIRED to 1.9.3: DONTNEED, 2.0.0: REQUIRED, 2.1: REQUIRED

Updated by naruse (Yui NARUSE) almost 10 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Applied in changeset r45529.


  • lib/xmlrpc/client.rb (do_rpc): don't check body length.
    If HTTP content-encoding is used, the length may be different.
    [Bug #8182] [ruby-core:53811]

Updated by usa (Usaku NAKAMURA) almost 10 years ago

  • Backport changed from 1.9.3: DONTNEED, 2.0.0: REQUIRED, 2.1: REQUIRED to 1.9.3: DONTNEED, 2.0.0: DONE, 2.1: REQUIRED

Backported into ruby_2_0_0 at r46153.

Updated by nagachika (Tomoyuki Chikanaga) almost 10 years ago

  • Backport changed from 1.9.3: DONTNEED, 2.0.0: DONE, 2.1: REQUIRED to 1.9.3: DONTNEED, 2.0.0: DONE, 2.1: DONE

r45529 was backported into ruby_2_1 branch at r46186.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0