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

zlib: gracefully set windowBits from 8 to 9 #16511

Merged
merged 2 commits into from
Oct 29, 2017

Conversation

addaleax
Copy link
Member

After looking into the details of the zlib behaviour when asked for 8-/9-bit windows more closely, this patch makes sense. I’ve updated the documentation & the test commentary to account for that.

(Addling lts-watch labels for the test/doc updates.)

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
Affected core subsystem(s)

zlib

@addaleax addaleax added lts-watch-v4.x zlib Issues and PRs related to the zlib subsystem. labels Oct 26, 2017
@addaleax addaleax added this to the 9.0.0 milestone Oct 26, 2017
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 26, 2017
doc/api/zlib.md Outdated
*Note*: An upgrade of zlib from 1.2.8 to 1.2.11 changed behavior when windowBits
is set to 8 for raw deflate streams. zlib would automatically set windowBits
to 9 if was initially set to 8. Newer versions of zlib will throw an exception,
so Node restored the original behaviour of upgrading a value of 8 to 9,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Node.js

Copy link
Member Author

Choose a reason for hiding this comment

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

done

doc/api/zlib.md Outdated
*Note*: An upgrade of zlib from 1.2.8 to 1.2.11 changed behavior when windowBits
is set to 8 for raw deflate streams. zlib would automatically set windowBits
to 9 if was initially set to 8. Newer versions of zlib will throw an exception,
so Node.js restored the original behaviour of upgrading a value of 8 to 9,
Copy link
Contributor

Choose a reason for hiding this comment

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

not a big deal, but behavior (without the u) is used in the previous sentence 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

After your research did you determine that we did not want to modify inflate stream

@addaleax
Copy link
Member Author

After your research did you determine that we did not want to modify inflate stream

At least for now that seems like a sensible course of action. zlib does output valid 8-bit-window data for windowBits = 8 even with the silent upgrade, so this should be okay.

@jasnell
Copy link
Member

jasnell commented Oct 29, 2017

This needs needs a rebase and will need to land before noon pacific time today to make it in to 9.0.0

MylesBorins and others added 2 commits October 29, 2017 20:14
On 4 April 2017, Node.js versions v4.8.2 and v6.10.2 were
released. These versions bumped the vendored zlib library from
v1.2.8 to v1.2.11 in response to what it describes as low-severity
CVEs. In zlib v1.2.9, a change was made that causes an error to be
raised when a raw deflate stream is initialised with windowBits set
to 8.

In zlib v1.2.9, 8 become an invalid value for this parameter, and Node's zlib
module will crash if you call this:

```
zlib.createDeflateRaw({windowBits: 8})
```

On some versions this crashes Node and you cannot recover from it, while on some
versions it throws an exception. The permessage-deflate library up to
version v0.1.5 does make such a call with no try/catch

This commit reverts to the original behavior of zlib by gracefully changed
windowBits: 8 to windowBits: 9 for raw deflate streams.

Original-PR-URL: nodejs-private/node-private#95
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Fixes: nodejs#14847
PR-URL: nodejs#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax
Copy link
Member Author

@jasnell Thanks for the ping 😄

Landed in 241eb61, 25ef9d2

@addaleax addaleax closed this Oct 29, 2017
@addaleax addaleax deleted the zlib-windowsbits-8 branch October 29, 2017 19:24
@addaleax addaleax merged commit 25ef9d2 into nodejs:master Oct 29, 2017
@gibfahn
Copy link
Member

gibfahn commented Oct 30, 2017

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@lpinca
Copy link
Member

lpinca commented Oct 30, 2017

@gibfahn no it's already there.

@lpinca
Copy link
Member

lpinca commented Oct 30, 2017

25ef9d2 could be backported.

@addaleax
Copy link
Member Author

@lpinca Yup, that’s why I’m doing now :)

addaleax added a commit to addaleax/node that referenced this pull request Oct 30, 2017
Fixes: nodejs#14847
Original-PR-URL: nodejs#16511
Original-Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Original-Reviewed-By: James M Snell <jasnell@gmail.com>
Original-Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Original-Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Original-Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit to gibfahn/node that referenced this pull request Oct 30, 2017
Fixes: nodejs#14847
PR-URL: nodejs#16511
Backport-PR-URL: nodejs#16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn pushed a commit that referenced this pull request Oct 31, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@gibfahn gibfahn mentioned this pull request Oct 31, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
On 4 April 2017, Node.js versions v4.8.2 and v6.10.2 were
released. These versions bumped the vendored zlib library from
v1.2.8 to v1.2.11 in response to what it describes as low-severity
CVEs. In zlib v1.2.9, a change was made that causes an error to be
raised when a raw deflate stream is initialised with windowBits set
to 8.

In zlib v1.2.9, 8 become an invalid value for this parameter, and Node's zlib
module will crash if you call this:

```
zlib.createDeflateRaw({windowBits: 8})
```

On some versions this crashes Node and you cannot recover from it, while on some
versions it throws an exception. The permessage-deflate library up to
version v0.1.5 does make such a call with no try/catch

This commit reverts to the original behavior of zlib by gracefully changed
windowBits: 8 to windowBits: 9 for raw deflate streams.

Original-PR-URL: https://github.com/nodejs-private/node-private/pull/95
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Fixes: nodejs/node#14847
PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
On 4 April 2017, Node.js versions v4.8.2 and v6.10.2 were
released. These versions bumped the vendored zlib library from
v1.2.8 to v1.2.11 in response to what it describes as low-severity
CVEs. In zlib v1.2.9, a change was made that causes an error to be
raised when a raw deflate stream is initialised with windowBits set
to 8.

In zlib v1.2.9, 8 become an invalid value for this parameter, and Node's zlib
module will crash if you call this:

```
zlib.createDeflateRaw({windowBits: 8})
```

On some versions this crashes Node and you cannot recover from it, while on some
versions it throws an exception. The permessage-deflate library up to
version v0.1.5 does make such a call with no try/catch

This commit reverts to the original behavior of zlib by gracefully changed
windowBits: 8 to windowBits: 9 for raw deflate streams.

Original-PR-URL: https://github.com/nodejs-private/node-private/pull/95
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
Fixes: nodejs/node#14847
PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins
Copy link
Contributor

@addaleax do you want to backport the updated documentation to v6.x?

@addaleax
Copy link
Member Author

@MylesBorins yes, and the variant that landed in v8.x (a6b3cd8) should apply cleanly

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 16, 2017

Done!

MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Fixes: #14847
PR-URL: #16511
Backport-PR-URL: #16623
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
On 4 April 2017, Node.js versions v4.8.2 and v6.10.2 were
released. These versions bumped the vendored zlib library from
v1.2.8 to v1.2.11 in response to what it describes as low-severity
CVEs. In zlib v1.2.9, a change was made that causes an error to be
raised when a raw deflate stream is initialised with windowBits set
to 8.

In zlib v1.2.9, 8 become an invalid value for this parameter, and Node's zlib
module will crash if you call this:

```
zlib.createDeflateRaw({windowBits: 8})
```

On some versions this crashes Node and you cannot recover from it, while on some
versions it throws an exception. The permessage-deflate library up to
version v0.1.5 does make such a call with no try/catch

This commit reverts to the original behavior of zlib by gracefully changed
windowBits: 8 to windowBits: 9 for raw deflate streams.

Original-PR-URL: nodejs-private/node-private#95
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
Fixes: nodejs/node#14847
PR-URL: nodejs/node#16511
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.