Bug #107744 for Net-HTTP: Net::HTTP::NB bug with chunked responses when there is a delay around the chunk header

Tue Oct 13 03:51:31 2015 rogerluc [...] cisco.com - Ticket created

Subject:	Net::HTTP::NB bug with chunked responses when there is a delay around the chunk header
Date:	Tue, 13 Oct 2015 07:50:53 +0000
To:	"bug-Net-HTTP [...] rt.cpan.org" <bug-Net-HTTP [...] rt.cpan.org>
From:	"Roger Lucas (rogerluc)" <rogerluc [...] cisco.com>

Net::HTTP::NB v6.09 Net::HTTP::Methods v6.09 OS: RedHat 6.7 on x86_64 Dear Net::HTTP developers, I have discovered a bug in Net::HTTP::NB if the server it connects to responds with chunked data and the client cannot read the chunk length field *and* at least 1 byte of the data in a single call to read_entity_body(). The details of the bug are as follows: When Net::HTTP::NB::read_entity_body() calls Net::HTTP::Methods::read_entity_body() for the first time, the Transfer-Encoding headers are inspected. If chunked mode is to be used, the chunk length is read (line 534) and the data is read (line 571). This all works fine if the data buffer has sufficient data already within it for both reads. If there is insufficient data to complete either or both reads then a second call is made to Net::HTTP::NB::sysread() and this causes the "Multi-Read" die() on line 16 after restoring the original data buffer from "httpnb_save". When Net::HTTP::NB::read_entity_body() is retried, the call to Net::HTTP::Methods::read_entity_body() skips the header inspection because "http_first_body" was reset to zero on the first attempt. This means that it assumes that the data is not chunked and just reads all the data as the content, ignoring any chunked encoding. I believe that the solution is to preserve and restore "http_first_body" and "http_request_method" around the "eval { }" block in Net::HTTP::NB::read_entity_body() so that subsequent retry attempts to Net::HTTP::Methods::read_entity_body() correctly restore its state if an error occurs. I have attached two test utilities that trigger the bug and validate the patch. The first, test-http-server.pl, is a small web server that can chunk its response into configurable sizes and can also flush and pause its output at configurable data positions. This is used to cause specific jitter patterns in the web server response so that we can reliably exercise the various paths through the client. The second, test-http-async-2.pl, is a small client using HTTP::Async that performs a set of tests against the web server with different chunked/non-chunked encodings and with different delay patterns. The client knows what web response to expect so it uses a SHA-1 digest to validate the response. I have also attached a patch for Net::HTTP::NB which causes all tests to pass on my machine. I apologise for renaming all files with a .txt extension but our email uses Microsoft Outlook and this often blocks extensions that it doesn't think are safe. Renaming files to .txt means that they always get through. As an FYI, this bug was original spotted with Net::HTTPS::NB when working with an HTTPS connection. That part of the code in Net::HTTPS::NB was copied from Net::HTTP::NB and the investigation continued to find the root cause as described above. I'll be offering an similar patch to the Net::HTTPS::NB developers. Best regards, Roger Lucas

Message body is not shown because sender requested not to inline it.

Tue Oct 13 07:32:24 2015 oleg [...] cpan.org - Cc OLEG added

Wed Oct 14 01:10:10 2015 oleg [...] cpan.org - Correspondence added

On Tue Oct 13 03:51:31 2015, rogerluc@cisco.com wrote: Show quoted text

> Net::HTTP::NB v6.09 > Net::HTTP::Methods v6.09 > OS: RedHat 6.7 on x86_64 > > Dear Net::HTTP developers, > > I have discovered a bug in Net::HTTP::NB if the server it connects to > responds with chunked data and the client cannot read the chunk length > field *and* at least 1 byte of the data in a single call to > read_entity_body(). > > The details of the bug are as follows: > > When Net::HTTP::NB::read_entity_body() calls > Net::HTTP::Methods::read_entity_body() for the first time, the > Transfer-Encoding headers are inspected. If chunked mode is to be > used, the chunk length is read (line 534) and the data is read (line > 571). This all works fine if the data buffer has sufficient data > already within it for both reads. If there is insufficient data to > complete either or both reads then a second call is made to > Net::HTTP::NB::sysread() and this causes the "Multi-Read" die() on > line 16 after restoring the original data buffer from "httpnb_save". > > When Net::HTTP::NB::read_entity_body() is retried, the call to > Net::HTTP::Methods::read_entity_body() skips the header inspection > because "http_first_body" was reset to zero on the first attempt. > This means that it assumes that the data is not chunked and just reads > all the data as the content, ignoring any chunked encoding. > > I believe that the solution is to preserve and restore > "http_first_body" and "http_request_method" around the "eval { }" > block in Net::HTTP::NB::read_entity_body() so that subsequent retry > attempts to Net::HTTP::Methods::read_entity_body() correctly restore > its state if an error occurs. > > I have attached two test utilities that trigger the bug and validate > the patch. > > The first, test-http-server.pl, is a small web server that can chunk > its response into configurable sizes and can also flush and pause its > output at configurable data positions. This is used to cause specific > jitter patterns in the web server response so that we can reliably > exercise the various paths through the client. > > The second, test-http-async-2.pl, is a small client using HTTP::Async > that performs a set of tests against the web server with different > chunked/non-chunked encodings and with different delay patterns. The > client knows what web response to expect so it uses a SHA-1 digest to > validate the response. > > I have also attached a patch for Net::HTTP::NB which causes all tests > to pass on my machine. > > I apologise for renaming all files with a .txt extension but our email > uses Microsoft Outlook and this often blocks extensions that it > doesn't think are safe. Renaming files to .txt means that they always > get through. > > As an FYI, this bug was original spotted with Net::HTTPS::NB when > working with an HTTPS connection. That part of the code in > Net::HTTPS::NB was copied from Net::HTTP::NB and the investigation > continued to find the root cause as described above. I'll be offering > an similar patch to the Net::HTTPS::NB developers. > > Best regards, > > Roger Lucas

With Roger we found that his patch doesn't cover all cases. This test server and client demonstrates problem: https://gist.github.com/olegwtf/c4dc83711c03a66ba07a So, I reworked his patch a little and now it seems it looks good for both of us.

Subject:

Net-HTTP-NB.patch

--- NB.pm.orig 2015-10-13 23:47:37.624399544 +0600 +++ NB.pm 2015-10-13 23:43:58.775329636 +0600 @@ -42,12 +42,25 @@ my $self = shift; ${*$self}{'httpnb_read_count'} = 0; ${*$self}{'httpnb_save'} = ${*$self}{'http_buf'}; + + my $chunked = ${*$self}{'http_chunked'}; + my $bytes = ${*$self}{'http_bytes'}; + my $first_body = ${*$self}{'http_first_body'}; + my @request_methods = @{${*$self}{'http_request_method'}}; + # XXX I'm not so sure this does the correct thing in case of # transfer-encoding transforms my $n = eval { $self->SUPER::read_entity_body(@_); }; if ($@) { - $_[0] = ""; - return -1; + if ($@ eq "Multi-read\n") { + # Reset some internals of Net::HTTP::Methods + ${*$self}{'http_chunked'} = $chunked; + ${*$self}{'http_bytes'} = $bytes; + ${*$self}{'http_first_body'} = $first_body; + ${*$self}{'http_request_method'} = \@request_methods; + } + $_[0] = ""; + return -1; } return $n; }

Wed Oct 14 01:10:11 2015 The RT System itself - Status changed from 'new' to 'open'

Thu May 16 15:20:42 2019 olaf [...] wundersolutions.com - Correspondence added

Ticket migrated to github as https://github.com/libwww-perl/Net-HTTP/issues/63

Thu May 16 15:20:43 2019 olaf [...] wundersolutions.com - Status changed from 'open' to 'resolved'