Bug 46146

Summary: deflate_in_filter fails to inflate if CRC/length bytes are not in available stream
Product: Apache httpd-2 Reporter: Laurence 'GreenReaper' Parry <greenreaper>
Component: mod_deflateAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: RESOLVED FIXED    
Severity: normal CC: ksacry, lmeyer, somashekhar.sonnagi, t_guerin
Priority: P2 Keywords: FixedInTrunk
Version: 2.5-HEAD   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: Patch for bug 46146 where mod_deflate does not wait for all the validation data before trying to validate the input Patch is for 2.2.24
new patch with spelling fixed
Patch proposal
files to reproduce the problem described in comment #12
file to reproduce the problem described in comment #12 (gzip)
Delay filter removal after we provide (not read) the EOS

Description Laurence 'GreenReaper' Parry 2008-11-04 13:48:46 UTC
The code intended to inflate gzipped content bodies appears to have an unimplemented edge case which I believe renders it unusable for most scenarios.

The problem is in the code to verify decompressed data by comparing the CRC value and length with that provided by the client in the last eight bytes of the request. If the gzip stream has been completely decompressed but the verification bytes are still on the wire, it looks like the decompression will fail as if the CRC or length were invalid:

  /* Is the remaining 8 bytes already in the avail stream? */
  if (ctx->stream.avail_in >= 8) {
    ...
  }
  else {
    /* FIXME: We need to grab the 8 verification bytes
    * from the wire! */
  inflateEnd(&ctx->stream);
  return APR_EGENERAL;
}

I'm trying to get some traction on implementing gzipped bodies in Firefox ( https://bugzilla.mozilla.org/show_bug.cgi?id=463089 ), but I'm unlikely to get anywhere if the only server which supports it fails on valid requests that happen to be a certain length.
Comment 1 Kevin Sacry 2013-05-15 15:26:41 UTC
Created attachment 30285 [details]
Patch for bug 46146 where mod_deflate does not wait for all the validation data before trying to validate the input
Patch is for 2.2.24

This is a patch for 2.2.4
Comment 2 Kevin Sacry 2013-05-15 15:29:19 UTC
Just uploaded a patch that fixes this for 2.2.4.
Mostly borowed from inflate_out_filter
Comment 3 Kevin Sacry 2013-05-15 15:32:26 UTC
Sorry, meant to say 2.2.24, not 2.2.4
Comment 4 Mike Rumph 2013-05-21 23:07:57 UTC
The following error message in the attached patch has a slight spelling error:
"Zlib: didn't recieve valid gzip magic bytes"

'recieve' should be 'receive'.
Comment 5 Kevin Sacry 2013-05-22 19:31:32 UTC
Created attachment 30314 [details]
new patch with spelling fixed

Thanks to Mike Rumph for pointing out the spelling error
Comment 6 Christian Nols 2014-01-28 16:28:01 UTC
Created attachment 31259 [details]
Patch proposal

Patch proposal tested on Apache 2.4.4

Move the CRC/Length check only after the 8 validation bytes are received.
Use the internal validation buffer for storage.

Test scenario.
I had two specific file upload that were causing the issue.
Transfer in chunked encoding and 6 bytes where in the last chunk.
The problem was systematic and is now solved.

I cannot attach the problematic files (customer data) and a custom HTTPClient is required to do the deflate/chunking.
Comment 7 Christian Nols 2014-01-28 16:28:48 UTC
*** Bug 52492 has been marked as a duplicate of this bug. ***
Comment 8 Yann Ylavic 2014-01-28 18:36:35 UTC
There are multiples patches in Bug 55666 which address this too...
Comment 9 Yann Ylavic 2014-02-27 16:11:33 UTC
*** Bug 55666 has been marked as a duplicate of this bug. ***
Comment 10 Yann Ylavic 2014-02-28 12:16:27 UTC
Thanks for the patches, commited in r1572668 with a similar approach to Christian's one, and with related commits onto the other input/output filters.
Comment 11 Yann Ylavic 2014-02-28 12:18:29 UTC
Commits are from the pacthes staged in Bug 55666.
Comment 12 Thierry Guérin 2014-04-15 10:24:06 UTC
Hi,
thanks a lot for the fix, I'm PUTting gzipped files and the upload would not work on some files (on 2.2.x and 2.4.x). So in its current state, sending gzipped content is indeed unusable; I really apreciate that work is being done in this area.

However, the patches introduced here unfortunately result in a situation that is in fact worse with some files (I have attached one):
- before the patches
  * with version 2.2.x, I get an error 500 and a message "Could not get next bucket brigade  [500, #0]" in the error log. There is no file on the server. BUT if I retry several times (between 1 and 9 times, this varies), the file gets correctly written.
  * with version 2.4.x, I get an error 500 and a warning "AH01396: Verification data not available (bug?)", then an error "An error occurred while reading the request body (URI: /uploads/curl.xml)  [500, #0]". There is no file on the server. BUT if I retry several times (more than 20 times, again this varies), the file gets written, but is truncated (its size varies)
  * with trunk, (before this patch is applied),  I get an error 400 and a warning "AH01396: Verification data not available (bug?)", then an error "An error occurred while reading the request body (URI: /uploads/curl.xml)  [400, #0]". There is no file on the server. I haven't tried multiple times.
- after the patches, there is no message in the error log and a code 201 is returned BUT the written file is truncated. Its size is always the same.

I have tested version 2.2.27 and 2.4.9 both on Windows & Debian, but the trunk has only been tested on Debian.

Thz gzip stream is created via a Java program, but the error can be reproduced with the command 'gzip -n myFile.xml'

To reproduce the problem:
curl -H "Content-Encoding: gzip" --upload-file raw.xml http://localhost/uploads/

(with raw.xml being the result of the gzip command)

I will attach both the xml file (curl.xml) and its gzipped version (gzip.xml).
Almost any small alteration to the xml file (like adding/removing a space) solves the problem.

I have added the comment to this bug, because its fix results (in my opinion) in things being unfotunately worse, but if you think that this belongs in a separate bug, I'll create one.
Hope this helps.
Comment 13 Thierry Guérin 2014-04-15 10:26:18 UTC
Created attachment 31526 [details]
files to reproduce the problem described in comment #12
Comment 14 Thierry Guérin 2014-04-15 10:27:59 UTC
Created attachment 31527 [details]
file to reproduce the problem described in comment #12 (gzip)
Comment 15 Yann Ylavic 2014-04-15 11:09:47 UTC
Thanks for reporting, could you include your mod_deflate configuration?
Comment 16 Thierry Guérin 2014-04-15 11:35:35 UTC
Sorry, forgot to mention that I used WebDav:

DavLockDB "/usr/local/apache2/var/DavLock"

Alias /uploads "/usr/local/apache2/uploads"
<Directory "/usr/local/apache2/uploads">
    Dav On
    Order Allow,Deny
    Allow from all
    Options Indexes FollowSymLinks
    SetInputFilter DEFLATE
</Directory>

(with of course mod_deflate, mod_dav and mod_dav_fs enabled)
Comment 17 Yann Ylavic 2014-04-15 14:06:34 UTC
Created attachment 31528 [details]
Delay filter removal after we provide (not read) the EOS

Could you please try the attached patch?
I can't reproduce anymore with it applied.
Thanks again Thierry for the test case.
Comment 18 Thierry Guérin 2014-04-15 15:24:17 UTC
bug fixed, fantastic! Thanks a lot for your (lightning-fast) help.

Do you think that this will be backported to the 2.2.x branch? (I suppose it will be backported to the 2.4.x (correct me if I'm wrong))
Comment 19 Yann Ylavic 2014-04-15 15:51:57 UTC
(In reply to Thierry Guérin from comment #18)
> Do you think that this will be backported to the 2.2.x branch? (I suppose it
> will be backported to the 2.4.x (correct me if I'm wrong))

The whole bundle for this and Bug 55666 is already proposed for backport in 2.4, waiting for more votes.

Once/if accepted, I will propose a 2.2 backport too.
Comment 20 Yann Ylavic 2014-06-21 22:54:54 UTC
Backported to upcoming 2.4.10.