content_encoding: fix inflate_stream for no bytes available #2060

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@jay
Member

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

content_encoding: fix inflate_stream for no bytes available
- 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

@jay jay added the HTTP label Nov 7, 2017

@jay jay requested a review from monnerat Nov 7, 2017

@monnerat

@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

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 7, 2017

Member

@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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@monnerat

monnerat Nov 8, 2017

Collaborator

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

Collaborator

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 jay:fix_inflate branch Nov 9, 2017

jay added a commit that referenced this pull request Nov 9, 2017

content_encoding: fix inflate_stream for no bytes available
- 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.