Skip to content

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Apr 2, 2017

The zlib C lib has been changed to clarify that in fact window bits = 8 is not supported:

madler/zlib@049578f

We already had a guard in one of the tests to avoid the window bits = 8 case for some formats where it had been observed to fail. The change in the C lib now makes it fail in more cases (since it had never been properly supported anyway). So the simplest thing to do is just say that 9 is the minimum value.

Credit to @trofi for notifying and then tracking down the root cause of the failure with newer versions of the C lib.

This fixes issue #11.

The zlib C lib has been changed to clarify that in fact window bits = 8
is not supported:

madler/zlib@049578f

We already had a guard in one of the tests to avoid the window bits = 8
case for some formats where it had been observed to fail. The change in
the C lib now makes it fail in more cases (since it had never been
properly supported anyway). So the simplest thing to do is just say that
9 is the minimum value.

Credit to Sergei Trofimovich for notifying and then tracking down the
root cause of the failure with newer versions of the C lib.

This fixes issue #11.
@dcoutts dcoutts requested a review from hvr April 2, 2017 16:10
@dcoutts
Copy link
Contributor Author

dcoutts commented Apr 2, 2017

@trofi can I request review / testing from you too? (Seems github does not let me request you as a reviewer since you're not in the org)

@hvr
Copy link
Member

hvr commented Apr 2, 2017

@dcoutts Judging from madler/zlib#94 (comment), it sounds like size=8 is only broken for deflate, while the zlib impl may support to inflate streams created with a different impl that properly implements size=8. If that's the case, your proposed change would outlaw size=8 even for the inflate case which supposedly works properly in theory?

@trofi
Copy link

trofi commented Apr 2, 2017

The change looks good. Passes all tests with this change applied.

Thank you!

@dcoutts
Copy link
Contributor Author

dcoutts commented Apr 2, 2017

@trofi thanks!

@hvr mm. However iirc, it is ok to use a bigger windowBits for the inflateInit() call than the data stream uses. The only downside is using more memory, but for 8 and 9 we're only talking about 256 or 512 bytes, so if someone does have a data stream that was deflated with window bits 8, then it's totally fine to inflate it with window bits 9 as it only costs 256 bytes more. I assume those small window bits sizes are really for tiny embedded systems where every byte counts.

@dcoutts dcoutts merged commit f57b2e9 into master Apr 2, 2017
@hvr
Copy link
Member

hvr commented Apr 3, 2017

@dcoutts yeah, but if you forbid setting window size = 8 on the inflate side, you basically break existing code for no good reason (i.e. if it worked properly before and there's no technical issue with it, why forbid it now?)? Also this violates somewhat the robustness princple (Be conservative in what you do, be liberal in what you accept from others (often reworded as "Be conservative in what you send, be liberal in what you accept")).

COnsequently, I think it's fine to throw errors if you try to set window size = 8 on the deflate side (as that's clearly not supported properly); but setting window size = 8 on the inflate size should NOT be an error if the underlying zlib impl properly supports this.

Also, if you start forbidding window size = 8 on the inflate size, you'd require a major version bump, since you're in theory breaking clients that worked before.

@hvr hvr deleted the fix-issue-11 branch March 13, 2019 09:01
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