Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

content_encoding: fix inflate_stream for no bytes available #2060

Closed
wants to merge 1 commit into from
Closed

content_encoding: fix inflate_stream for no bytes available #2060

wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Nov 7, 2017

  • Don't call zlib's inflate() when avail_in stream bytes is 0.

Prior to this change libcurl's inflate_stream could call zlib's inflate
even when no bytes were available, causing inflate to return
Z_BUF_ERROR, and then inflate_stream would treat that as a hard error
and return CURLE_BAD_CONTENT_ENCODING.

According to the zlib FAQ, Z_BUF_ERROR is not fatal.

This bug would happen randomly since packet sizes are arbitrary. A test
of 10,000 transfers had 55 fail (ie 0.55%).

Ref: https://zlib.net/zlib_faq.html#faq05

Closes #2060

Bug first seen in CI:
https://travis-ci.org/curl/curl/jobs/297978443#L4332
#2056 (comment)

sprinkle:

diff --git a/lib/content_encoding.c b/lib/content_encoding.c
index 6b31685..4cdf3a1 100644
--- a/lib/content_encoding.c
+++ b/lib/content_encoding.c
@@ -129,6 +129,7 @@ inflate_stream(struct connectdata *conn, contenc_writer *writer)
   CURLcode result = CURLE_OK;   /* Curl_client_write status */
   char *decomp;                 /* Put the decompressed data here. */
 
+  fprintf(stderr, ">>>inflate_stream\n");
   /* Dynamically allocate a buffer for decompression because it's uncommonly
      large to hold on the stack */
   decomp = malloc(DSIZ);
@@ -143,7 +144,12 @@ inflate_stream(struct connectdata *conn, contenc_writer *writer)
     z->next_out = (Bytef *) decomp;
     z->avail_out = DSIZ;
 
+    fprintf(stderr, "before inflate: z->avail_in: %d, z->avail_out: %d\n",
+            z->avail_in, z->avail_out);
     status = inflate(z, Z_SYNC_FLUSH);
+    fprintf(stderr, "after inflate: status %d, z->avail_in: %d, "
+                    "z->avail_out: %d\n",
+            status, z->avail_in, z->avail_out);
     if(status == Z_OK || status == Z_STREAM_END) {
       allow_restart = 0;
       result = Curl_unencode_write(conn, writer->downstream, decomp,

success case example:

>>>inflate_stream
before inflate: z->avail_in: 214, z->avail_out: 16384
after inflate: status 0, z->avail_in: 0, z->avail_out: 16185
>>>inflate_stream
before inflate: z->avail_in: 199, z->avail_out: 16384
after inflate: status 0, z->avail_in: 0, z->avail_out: 16170
>>>inflate_stream
before inflate: z->avail_in: 1114, z->avail_out: 16384
after inflate: status 1, z->avail_in: 0, z->avail_out: 15278
>>>inflate_stream
before inflate: z->avail_in: 1106, z->avail_out: 16384
after inflate: status 1, z->avail_in: 0, z->avail_out: 11557

fail case example:

>>>inflate_stream
before inflate: z->avail_in: 14, z->avail_out: 16384
after inflate: status 0, z->avail_in: 0, z->avail_out: 16384
>>>inflate_stream
before inflate: z->avail_in: 0, z->avail_out: 16384
after inflate: status -5, z->avail_in: 0, z->avail_out: 16384

Z_BUF_ERROR (-5). inflate_stream treats it as a hard error, but according to the zlib FAQ it's not.

rather than deal with the error this change prevents it from occurring in the first place by returning when there are no bytes available

@jay
content_encoding: fix inflate_stream for no bytes available
d03b2b7 
- Don't call zlib's inflate() when avail_in stream bytes is 0.

Prior to this change libcurl's inflate_stream could call zlib's inflate
even when no bytes were available, causing inflate to return
Z_BUF_ERROR, and then inflate_stream would treat that as a hard error
and return CURLE_BAD_CONTENT_ENCODING.

According to the zlib FAQ, Z_BUF_ERROR is not fatal.

This bug would happen randomly since packet sizes are arbitrary. A test
of 10,000 transfers had 55 fail (ie 0.55%).

Ref: https://zlib.net/zlib_faq.html#faq05

Closes curl#2060
@jay jay added the HTTP label Nov 7, 2017
@jay jay requested a review from monnerat November 7, 2017 06:04
monnerat
monnerat reviewed Nov 7, 2017
View reviewed changes
Copy link
Contributor

@monnerat monnerat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jay : you're perfectly right about the problem. However it should already be fixed in a more global way by commit 19e66e5.

Checking before calling inflate() is indeed more logical and elegant, but if so, test at https://github.com/jay/curl/blob/d03b2b7575c18e88e5eda30834bd91c33c0bb8d1/lib/content_encoding.c#L171 is not needed anymore.

@jay
Copy link
Member Author

jay commented Nov 7, 2017

@jay : you're perfectly right about the problem. However it should already be fixed in a more global way by commit 19e66e5.

Right I'd just prefer to fix it closer to home. Do you object to this as a follow up?

@monnerat
Copy link
Contributor

monnerat commented Nov 8, 2017

Right I'd just prefer to fix it closer to home. Do you object to this as a follow up?

No objection. However the inflate_stream() procedure needs rework (not very elegant and a potential problem about allow_restart being reinited at each call) and I will investigate it in the next few days. In the meantime, feel free to push :-)

@jay jay closed this Nov 9, 2017
@jay jay deleted the fix_inflate branch November 9, 2017 06:46
jay added a commit that referenced this pull request Nov 9, 2017
@jay
content_encoding: fix inflate_stream for no bytes available
fa64b0f 
- Don't call zlib's inflate() when avail_in stream bytes is 0.

This is a follow up to the parent commit 19e66e5. Prior to that change
libcurl's inflate_stream could call zlib's inflate even when no bytes
were available, causing inflate to return Z_BUF_ERROR, and then
inflate_stream would treat that as a hard error and return
CURLE_BAD_CONTENT_ENCODING.

According to the zlib FAQ, Z_BUF_ERROR is not fatal.

This bug would happen randomly since packet sizes are arbitrary. A test
of 10,000 transfers had 55 fail (ie 0.55%).

Ref: https://zlib.net/zlib_faq.html#faq05

Closes #2060
@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@monnerat monnerat monnerat left review comments

@hanawa-suzuki hanawa-suzuki hanawa-suzuki approved these changes

Assignees
No one assigned
Labels
HTTP
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jay @monnerat @hanawa-suzuki