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.

Sign up for GitHub

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

Jump to bottom

zlib: fix windowBits validation to allow 0 for decompression mode #19686

Closed
wants to merge 2 commits into from
Closed

zlib: fix windowBits validation to allow 0 for decompression mode #19686

wants to merge 2 commits into from

Conversation

anandsuresh
Copy link
Contributor

@anandsuresh anandsuresh commented Mar 29, 2018 •
edited
Loading

From the zlib v1.2.11 manual:

ZEXTERN int ZEXPORT inflateInit2 OF((z_streamp strm,
int windowBits));

...
windowBits can also be zero to request that inflate use the window
size in the zlib header of the compressed stream.

The current validation of windowBits in zlib.js doesn't check for this
case.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@anandsuresh
zlib: fix windowBits validation to allow 0 for decompression mode
1c0e427 
From the zlib v1.2.11 manual:

> ZEXTERN int ZEXPORT inflateInit2 OF((z_streamp strm,
>                                      int  windowBits));
>
> ...
> windowBits can also be zero to request that inflate use the window
> size in the zlib header of the compressed stream.

The current validation of windowBits in zlib.js doesn't check for this
case.
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Mar 29, 2018
addaleax
addaleax reviewed Mar 29, 2018
View reviewed changes
lib/zlib.js Outdated
mode === GUNZIP ||
mode === INFLATERAW ||
mode === UNZIP)) {
windowBits = 0
Copy link
Member

Choose a reason for hiding this comment

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

Can you run make lint? It should complain about the lack of a semicolon here ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops. I'm spoiled by standardjs! Will fix.

lib/zlib.js Outdated
if (opts.windowBits === 0 &&
(mode === INFLATE ||
mode === GUNZIP ||
mode === INFLATERAW ||
Copy link
Member

Choose a reason for hiding this comment

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

INFLATERAW means that there is no header, so I think we'd want to drop that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I picked the list from

node/src/node_zlib.cc

Lines 541 to 544 in 22f4a35

case INFLATE:
case GUNZIP:
case INFLATERAW:
case UNZIP:
, since they all end up calling inflateInit2(). Will remove it from the list.

lib/zlib.js Outdated
// windowBits is special. On the compression side, 0 is an invalid value.
// But on the decompression side, a value of 0 for windowBits tells zlib
// to extract the value from the headers of the compressed stream.
if (opts.windowBits === 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

We may actually want to also capture undefined here, so that when decompressing we always go with the header-provided value (instead of the safe maximum) for lower memory usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent idea! How about opts.windowBits == null || opts.windowBits === 0?

Copy link
Member

Choose a reason for hiding this comment

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

@anandsuresh Sounds reasonable to me!

@richardlau
Copy link
Member

Test(s)?

@lpinca
Copy link
Member

lpinca commented Mar 30, 2018

SGTM with tests added.

@anandsuresh
Copy link
Contributor Author

tests added.

richardlau
richardlau reviewed Mar 30, 2018
View reviewed changes
test/parallel/test-zlib-zero-windowBits.js Outdated
failed = true;
} catch (e) {
assert(e instanceof RangeError);
}
Copy link
Member

Choose a reason for hiding this comment

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

common.expectsError() instead of try-catch.

test/parallel/test-zlib-zero-windowBits.js Outdated
failed = true;
} catch (e) {
assert(e instanceof RangeError);
}
Copy link
Member

Choose a reason for hiding this comment

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

common.expectsError() instead of try-catch.

lpinca
lpinca reviewed Mar 30, 2018
View reviewed changes
test/parallel/test-zlib-zero-windowBits.js Outdated
let failed = false;
let deflate = null;
try {
deflate = zlib.createDeflate({windowBits: 0});
Copy link
Member

@lpinca lpinca Mar 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is already tested in test-zlib-deflate-constructors.js so it is probably redundant.

test/parallel/test-zlib-zero-windowBits.js Outdated
let failed = false;
let gzip = null;
try {
gzip = zlib.createGzip({windowBits: 0});
Copy link
Member

@lpinca lpinca Mar 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We can simply by using common.expectsError():

common.expectsError(
  () => zlib.createGzip({windowBits: 0}),
  {
    code: 'ERR_INVALID_OPT_VALUE',
    type: RangeError,
    message: '...'
  }
);

@anandsuresh anandsuresh force-pushed the anandsuresh/zlibWindowBits branch 2 times, most recently from bcb88e0 to 1ba53f2 Compare March 30, 2018 19:58
@anandsuresh
Copy link
Contributor Author

standby.... some tests are failing. Working to fix those.

@anandsuresh
Copy link
Contributor Author

all tests passing again.

@lpinca
Copy link
Member

lpinca commented Mar 31, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/13960/

@anandsuresh anandsuresh mentioned this pull request Mar 31, 2018
Closed
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Landed in 7bc5151 🎉

@BridgeAR BridgeAR closed this Apr 9, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
@anandsuresh @BridgeAR
zlib: fix windowBits validation to allow 0 for decompression mode
7bc5151 
From the zlib v1.2.11 manual:

> ZEXTERN int ZEXPORT inflateInit2 OF((z_streamp strm,
>                                      int  windowBits));
>
> ...
> windowBits can also be zero to request that inflate use the window
> size in the zlib header of the compressed stream.

The current validation of windowBits in zlib.js doesn't check for this
case.

PR-URL: nodejs#19686
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@anandsuresh @BridgeAR
zlib: fix windowBits validation to allow 0 for decompression mode
bbba428 
From the zlib v1.2.11 manual:

> ZEXTERN int ZEXPORT inflateInit2 OF((z_streamp strm,
>                                      int  windowBits));
>
> ...
> windowBits can also be zero to request that inflate use the window
> size in the zlib header of the compressed stream.

The current validation of windowBits in zlib.js doesn't check for this
case.

PR-URL: nodejs#19686
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@Adsada2205 Adsada2205 Adsada2205 approved these changes

@richardlau richardlau richardlau approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anandsuresh @richardlau @lpinca @BridgeAR @addaleax @Adsada2205 @nodejs-github-bot