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

fix bug in deflate with windowBits == 8 #291

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link

This patch comes from the optipng project and is licensesd under the
zlib license. I am unsure the original author but it may be @ctruta

This patch applies to zlib-1.1.3. It fixes a problem with 256-byte
sliding windows, that exists in all the zlib versions up to 1.1.3,
and it is proposed to be included in the future zlib-1.1.4 release
The problem occurs because MIN_LOOKAHEAD in deflate.c is usually
bigger than 256, and this confuses the longest_match() function.

The validity of the patch is enabled by this spec correction. The
particular implementation of the reference library limits the
distance codes to 2^windowBits - MIN_LOOKAHEAD.
When windowBits =9, all the distance codes are no bigger than
250, so the CINFO flag in the zlib stream can safely be 0.

The patch also extends the functionality of deflateInit(), so that
other (smaller) window sizes are also accepted. This way, the
library becomes more extensible, allowing future additions of
newer strategies optimized for speed and/or low memory
requirements, such as the Z_RLE strategy. If a certain value of
windowBits is not directly supported by the implementation, it can
be adjusted to the nearest supported value: valid zlib streams are
still produced.

Refs: http://optipng.sourceforge.net/pngtech/zlib-256win-bug.html

@@ -296,11 +296,10 @@ int ZEXPORT deflateInit2_(strm, level, method, windowBits, memLevel, strategy,
}
#endif
if (memLevel < 1 || memLevel > MAX_MEM_LEVEL || method != Z_DEFLATED ||
windowBits < 8 || windowBits > 15 || level < 0 || level > 9 ||
Copy link

Choose a reason for hiding this comment

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

@MylesBorins What's the reason the check for windowBits < 8 has been removed here?

Copy link
Author

Choose a reason for hiding this comment

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

Because it is no longer an error

Copy link

Choose a reason for hiding this comment

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

I'm probably missing something but I thought that bug was just in windowBits=8. Does fixing the 256-byte window bug mean all values of windowBits from 0 to 15 are now valid?

Copy link
Author

Choose a reason for hiding this comment

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

in retrospect that is what is in the original patch mentioned in the refernce

This patch comes from the optipng project and is licensesd under the
zlib license. I am unsure the original author but it may be @ctruta

    This patch applies to zlib-1.1.3. It fixes a problem with 256-byte
    sliding windows, that exists in all the zlib versions up to 1.1.3,
    and it is proposed to be included in the future zlib-1.1.4 release
    The problem occurs because MIN_LOOKAHEAD in deflate.c is usually
    bigger than 256, and this confuses the longest_match() function.

    The validity of the patch is enabled by this spec correction. The
    particular implementation of the reference library limits the
    distance codes to 2^windowBits - MIN_LOOKAHEAD.
    When windowBits =9, all the distance codes are no bigger than
    250, so the CINFO flag in the zlib stream can safely be 0.

    The patch also extends the functionality of deflateInit(), so that
    other (smaller) window sizes are also accepted. This way, the
    library becomes more extensible, allowing future additions of
    newer strategies optimized for speed and/or low memory
    requirements, such as the Z_RLE strategy. If a certain value of
    windowBits is not directly supported by the implementation, it can
    be adjusted to the nearest supported value: valid zlib streams are
    still produced.

Refs: http://optipng.sourceforge.net/pngtech/zlib-256win-bug.html
@peter-gribanov
Copy link

peter-gribanov commented Sep 12, 2019

I think more correctly like this:

    if (memLevel < 1 || memLevel > MAX_MEM_LEVEL || method != Z_DEFLATED ||
-        windowBits < 8 || windowBits > 15 || level < 0 || level > 9 ||
+        windowBits < 9 || windowBits > 15 || level < 0 || level > 9 ||
-        strategy < 0 || strategy > Z_FIXED || (windowBits == 8 && wrap != 1)) {
+        strategy < 0 || strategy > Z_FIXED) {
        return Z_STREAM_ERROR;
    }
-    if (windowBits == 8) windowBits = 9;  /* until 256-byte window bug fixed */

s->w_bits = ((uInt)windowBits >= 8) ? (uInt)windowBits : 8;
#else
s->w_bits = ((uInt)windowBits >= 9) ? (uInt)windowBits : 9;
#endif
s->w_size = 1 << s->w_bits;

Choose a reason for hiding this comment

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

As far as i can understand, a 256 byte window is not supported at all #171. Therefore, it is more correct to write like this:

-    s->w_bits = (uInt)windowBits;
+    s->w_bits = ((uInt)windowBits >= 9) ? (uInt)windowBits : 9;

#else
uInt optimized_cinfo = (s->w_bits > 9) ? s->w_bits - 8 : 0;
uInt header = (Z_DEFLATED + (optimized_cinfo<<4)) << 8;
#endif

Choose a reason for hiding this comment

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

As far as i can understand, a 256 byte window is not supported at all #171. Therefore, it is more correct to write like this:

-        uInt header = (Z_DEFLATED + ((s->w_bits-8)<<4)) << 8;
+        uInt optimized_cinfo = (s->w_bits > 9) ? s->w_bits - 8 : 0;
+        uInt header = (Z_DEFLATED + (optimized_cinfo<<4)) << 8;

@MylesBorins MylesBorins closed this May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants