Net::HTTP::NB bug with chunked responses when there is a delay around the chunk header [rt.cpan.org #107744]

[oalders]

Migrated from rt.cpan.org#107744 (status was 'open')

Requestors:

• rogerluc@cisco.com

Attachments:

• Net-HTTP-NB.patch.txt
• test-http-async-2.pl.txt
• test-http-server.pl.txt
• Net-HTTP-NB.patch

From rogerluc@cisco.com on 2015-10-13 07:51:31
:

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

From oleg@cpan.org on 2015-10-14 05:10:10
:

On Tue Oct 13 03:51:31 2015, rogerluc@cisco.com wrote:
> 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.