Bug 53725

Summary: Some GZipped responses result in java.io.IOException: Corrupt GZIP trailer during gunzip.
Product: Tomcat 7 Reporter: kevin.l.stern
Component: ConnectorsAssignee: Tomcat Developers Mailing List <dev>
Severity: critical    
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: Please see description for purpose of attachment.
Demonstrates the same problem post-fix.

Description kevin.l.stern 2012-08-15 18:28:36 UTC
Created attachment 29238 [details]
Please see description for purpose of attachment.

FlushableGZIPOutputStream gives corrupt output in rare circumstances.  Tomcat 7 encounters this bug when compression is turned on.

Please see the attachment:  Compile and run ErrorCase.java which uses FlushableGZIPOutputStream to GZIP the contents of data.bin (included in the attachment) and then uses GZIPInputStream to gunzip the output of FlushableGZIPOutputStream, resulting in java.io.IOException: Corrupt GZIP trailer.
Comment 1 Mark Thomas 2012-08-25 20:11:18 UTC
Thanks for the test case. It made fixing this a whole lot easier than it would otherwise have been.

The fix has been applied to trunk and 7.0.x and will be included in 7.0.30 onwards.

I haven't included the test case in the unit tests due to the size of the file required to trigger this issue. (It would significantly increase the size of the source distribution.)
Comment 2 Konstantin Kolinko 2012-08-26 14:43:50 UTC
Good catch.
I proposed backport of this fix (r1377342) to Tomcat 6.0.
Comment 3 kevin.l.stern 2012-08-27 20:52:56 UTC
Created attachment 29286 [details]
Demonstrates the same problem post-fix.

Please unzip to obtain the data.bin which causes the issue.
Comment 4 kevin.l.stern 2012-08-27 20:53:36 UTC
I have attached a new data.bin which demonstrates that the problem remains post-fix.
Comment 5 kevin.l.stern 2012-08-27 21:10:49 UTC
Note that, not surprisingly, using Java 7's GZIPOutputStream with syncFlush=true resolves the issue.
Comment 6 Mark Thomas 2012-08-27 21:12:22 UTC
That is an option for Tomcat 8 (which will require Java 7) but not for Tomcat 7 and earlier.
Comment 7 Mark Thomas 2012-08-28 21:14:14 UTC
Very different failure mode (a whole chunk of data is missing from the end). Probably a different bug. I'll start digging.
Comment 8 Mark Thomas 2012-08-28 22:41:41 UTC
Easy solution implemented for trunk (Tomcat 8).

Still trying to find a fix for Tomcat 7.
Comment 9 Konstantin Kolinko 2012-08-28 23:21:29 UTC
Created attachment 29297 [details]

Thank you for the test!

Fixed in 7.0 with r1378378. Will be in 7.0.30.

Here is patch for Tomcat 6 (r1377343 + r1378378).
Comment 10 Mark Thomas 2012-08-28 23:58:28 UTC
That test (In reply to comment #9)
> Created attachment 29297 [details]
> 2012-08-29_tc6_53725.patch
> Thank you for the test!
> Fixed in 7.0 with r1378378. Will be in 7.0.30.
> Here is patch for Tomcat 6 (r1377343 + r1378378).

While that patch fixed the test for the second test case, it re-broke it for the first.

After a lot of trial and error I have a patch that works for both test cases. I'm not at all clear as to the root cause of what is happening as the critical code is native code. The latest patch may still fail for some edge cases.
Comment 11 Jess Holle 2012-08-29 00:24:53 UTC
After some time with dealing with such issues with our own servlet filter for compression and trying various solutions tried in Tomcat, I finally gave up and went with jzlib for pre-Java-7 cases and with the built-in sync-flush functionality in Java 7 cases.  I got tired of someone always coming up with some new case where trying to allow flushing with the pre-Java-7 GZipOutputStream caused an issue.
Comment 12 Konstantin Kolinko 2012-08-29 00:59:00 UTC
I reverted the fix in r1378402
and implemented a different one in r1378403 + r1378408 
(will be in 7.0.30)

The FlushableGZIPOutputStream#deflate() method is not called by this class directly, but by its parent class, DeflaterOutputStream.
Such calls are always in a loop,

            while (!def.needsInput()) {
	    while (!def.finished()) {

so I there should not be a need to add (!def.needsInput()) condition to the deflate() method itself.
Comment 13 Mark Thomas 2012-10-04 16:09:14 UTC
Fixed in 6.0.x and will be included in 6.0.36 onwards.