zng_deflateInit2 silently converts windowBits=8 to windowBits=9

[rhpvorderman]

zlib-ng can't handle windowBits=8. This is fine. What is not fine is that instead of crashing it starts deflating with windowBits=9. As a result when inflating again with windowBits=8 it crashes with an invalid window size. This crash occurs to late. If zng_deflateInit2 finds that the data cannot be deflated using the user-requested settings it should crash right away and notify the user of this problem. Verbosely crashing is better than silently subverting user expectations and then mysteriously crashing later elsewhere.

The following code causes the issue: https://github.com/zlib-ng/zlib-ng/blob/c255e58dd5e0ec3b2febb29c0905e89032419bcd/deflate.c#L227-228

    if (windowBits == 8)
        windowBits = 9;  /* until 256-byte window bug fixed */

This should be

    if (windowBits == 8) {
         strm.msg = "zlib-ng does not support windowBits=8 yet, 256-byte windows are bugged."; 
         return Z_STREAM_ERROR;
    }

[nmoinvaz]

Current zlib-ng behavior appears to be the same as madler/zlib. https://github.com/madler/zlib/blob/04f42ceca40f73e2978b50e93806c2a18c1281fc/deflate.c#L297

[nmoinvaz]

Perhaps inflate should also convert windowBits == 8 to windowBits = 9 to compensate?

[rhpvorderman]

I found this bug report on Mark Adler's zlib from 2015: https://github.com/madler/zlib/issues/94

Perhaps inflate should also convert windowBits == 8 to windowBits = 9 to compensate?

Mark Adler explicitly chose not to do this apparently. This is a known bug. Best to leave it as is, and document it, I guess?

[Dead2]

I wonder whether we still have the 256-byte window bug in our deflate code or not, a lot of the involved code has been changed. I have not looked into the cause of the bug, but I would suspect it involves 256 window vs 258 byte matches. It might be worth looking into and perhaps document it in the wiki.

[mtl1979]

I'm currently working on code that limits maximum size of the matches... It can be adapted for 256 byte windows to limit maximum match size to for example 130 bytes, but this means adding new function that only compare the last 128 bytes instead of last 256 bytes.

[nmoinvaz]

@mtl1979 were you able to get anywhere with the 256 byte window issue?

[mtl1979]

@mtl1979 were you able to get anywhere with the 256 byte window issue?

I haven't had much time to work on the patch as I've been working on other things and there has been quite big changes in the code, so I wanted to wait until the codebase is more stable.

Basically we need to reduce MAX_MATCH to 130 (128 + 2) when windowBits is 8, and compare only 130 bytes instead of 258 bytes (256 + 2).

[Dead2]

Could an alternative be to hijack check_match or something similar to just truncate any found matches that were too long?

Also, we could change zlib-ng native api to require windowBits minimum of 9 for both deflate and inflate (with a documented and silent conversion of 8 -> 9 to avoid older applications getting into trouble), I have a feeling that is a worthwhile tradeoff.

The zlib-compat codepath could still benefit from a fix of some kind of course. Personally, I don't see a problem with just using windowBits = 9 there too (The only downside is a miniscule extra amount of ram, right?), but I am open to better solutions as long as they are not too intrusive.