zlib.inflate does not support value of 0 for options.windowBits

[jeffreydwalter]

The windowBits assertion on line 400 of node_zlib.cc is asserting:
assert((windowBits >= 8 && windowBits <= 15) && "invalid windowBits");

zlib accepts a 0 windowBits in the Inflate case. The assertion doesn't consider whether the it's the inflate or deflate case. This probably happens to work because the assertion is most likely being compiled out in production builds.

So, the assertion should be: assert((windowBits == 0 || (windowBits >= 8 && windowBits <= 15)) && "invalid windowBits");

or (something like this: didn't test this.)
assert((
(ctx->mode_ == INFLATERAW || ctx->mode_ == GUNZIP || ctx->mode_ == UNZIP) && windowBits == 0) || (windowBits >= 8 && windowBits <= 15)) && "invalid windowBits");

[misterdjules]

What version of node did you use? Also, can you show us some code that reproduces this?

[jeffreydwalter]

This assertion is present in the latest version of node 0.12.0. It's also in 0.11.x, I don't know how far back it goes. I also didn't actually test and fail the assertion, but was looking at the code and noticed the assertion.

The issue is that the Init() method in node_zlib.cc (line 400 v0.12.0) isn't taking into consideration whether the zlib instance is an inflate or deflate operation. In the inflate case, 0 is a valid value for windowBits.

The zlib documentation for inflateInit2() says the following, "windowBits can also be zero to request that inflate use the window size in the zlib header of the compressed stream." -- http://www.zlib.net/manual.html

A patch to node-spdy was submitted which sets the windowBits value to 0 by default, so I suppose you could build a debug version of node and use a hello world node-spdy application to exercise the assertion. spdy-http2/node-spdy#187 - Fedor Indutny could probably help you get a quick test together.

[jeffreydwalter]

Looks like this issue is only relevant to node v0.12.0+, as the older versions of node were using zlib v1.2.3 and support for windowBits = 0 was introduced in zlib v1.2.3.5. node v0.12.0 is using zlib v1.2.8, and so the assertion needs to be updated in v0.12.0+.

I will submit a patch for node_zlib.cc and zlib.js so you can see the changes I'm talking about.

[misterdjules]

@jeffreydwalter Thank you, did you have some time to come up with a patch?

We might want to rename this issue to "zlib.inflate does not support value of 0 for options.windowBits".
The assert in itself is not "bad", it's more that the API and the implementation have not been updated with the recent changes in zlib. What do you think?

Also, to answer one of your previous questions, assert statements are not compiled out in release builds.

[jeffreydwalter]

Hey @misterdjules I started work on the patch, but got side-tracked with some family issues.
I updated the title to your suggested title.

Give me a few more days and I'll see if I can get a patch.

[misterdjules]

@jeffreydwalter No worries, there's no rush :) Thank you very much, and all the best to your family.

[jeffreydwalter]

@misterdjules thanks! Here's a gist with the diff of zlib.js and node_zlib.cc. I didn't have a chance to actually test it so please consider the changes carefully and let me know what you think.

https://gist.github.com/jeffreydwalter/f2650c262b28acbaf48b

[misterdjules]

Thank you! Since this is an API change, we would integrate that change in node v0.13.1 (at the earliest), so adding this to the corresponding milestone.

I just skimmed through the changes and they seem to look good. As you mentioned, we would need additional tests with that change before we can review and integrate it.

We're currently focused on two releases (the upcoming 0.10.37 and 0.12.1), so it will probably take a while before we can review these changes. In the meantime I suggest that you submit a pull-request and we'll pick it up as soon as possible.

How does that sound?

[jeffreydwalter]

sounds good. I will leave the test writing up to you guys if that's okay with you. I'm not comfortable that I will write proper tests for this issue.

[misterdjules]

Yes, we can take care of that if you don't want to, or I can spend some time with you to guide you on how to write tests whenever you have some time if you're interested :)

[jeffreydwalter]

Thanks, I'd be happy to learn how you guys write your tests.

Should I create a pull request for these changes against master? I noticed there was a v0.13 branch, but it looks like it's gone now.

[jeffreydwalter]

@misterdjules, I went head and made a pull-request to get these changes in master. Looks like the v0.13 branch no longer exists. If this is wrong, please let me know. Also, I haven't written any tests yet, but would be happy to spend some time with you doing so. Thanks!

[misterdjules]

@jeffreydwalter I'm not sure there ever was a v0.13 branch. As far as I know, the v0.13.0-pre version has always been built from the master branch. In other words, you did the right thing by creating your PR against the master branch 👍

Writing tests for this PR would be great! We'll need them at some point to be able to merge your changes. You can write your tests after others found in test/simple/test-zlib*, and if you have any question please come talk to us on IRC in #libuv on Freenode.

[jeffreydwalter]

@misterdjules I'm pretty close to having the test for this fix completed, but am unsure about the proper behavior of windowBits in zlib. madler/zlib#93
Do you happen to know what the valid values for windowBits are?

[misterdjules]

@jeffreydwalter I'm not familiar with zlib, but it seems that you believe the behavior for windowBits === 0 in the zlib inflate case is well defined.

Isn't that sufficient to move forward with this?

[jeffreydwalter]

@misterdjules

I went ahead and created a pull request with the latest version of my zlib changes including a test case.

#25140

Please let me know what you think. Thanks!

[misterdjules]

@jeffreydwalter Thanks!

[jasnell]

@misterdjules ... I'm thinking it would likely be best to defer this to the converged repo. What do you think?

[jeffreydwalter]

@misterdjules @jasnell looks like there WAS a bug in zlib which was fixed here madler/zlib#93. That fix isn't in the version of zlib node is built with, so we should probably defer and let me know when you're planning on upgrading zlib to that version and I'll update the code and test cases to account for the fix.

The test cases need minor tweaking and the actual code basically needs to be reverted to work like this commit. jeffreydwalter@d693dd1