zlib: gracefully set windowBits from 8 to 9 #16511

addaleax · 2017-10-26T10:38:16Z

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

lpinca · 2017-10-26T10:44:44Z

doc/api/zlib.md

+*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,


Nit: Node.js

maclover7 · 2017-10-26T13:38:14Z

doc/api/zlib.md

+*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,


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

mhdawson

LGTM

MylesBorins · 2017-10-26T15:57:35Z

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

addaleax · 2017-10-26T16:01:42Z

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 · 2017-10-29T17:44:02Z

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

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 · 2017-10-29T19:24:01Z

@jasnell Thanks for the ping 😄

Landed in 241eb61, 25ef9d2

gibfahn · 2017-10-30T20:19:45Z

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 · 2017-10-30T20:22:06Z

@gibfahn no it's already there.

lpinca · 2017-10-30T20:26:40Z

25ef9d2 could be backported.

addaleax · 2017-10-30T20:28:16Z

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

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>

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>

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>

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>

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>

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>

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 · 2017-11-16T21:34:07Z

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

addaleax · 2017-11-16T21:37:01Z

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

MylesBorins · 2017-11-16T22:21:47Z

Done!

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>

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>

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>

addaleax added lts-watch-v4.x zlib Issues and PRs related to the zlib subsystem. labels Oct 26, 2017

addaleax added this to the 9.0.0 milestone Oct 26, 2017

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 26, 2017

addaleax mentioned this pull request Oct 26, 2017

zlib crashes when given windowBits: 8 #14847

Closed

lpinca approved these changes Oct 26, 2017

View reviewed changes

maclover7 reviewed Oct 26, 2017

View reviewed changes

jasnell approved these changes Oct 26, 2017

View reviewed changes

cjihrig approved these changes Oct 26, 2017

View reviewed changes

mhdawson approved these changes Oct 26, 2017

View reviewed changes

refack approved these changes Oct 26, 2017

View reviewed changes

lpinca mentioned this pull request Oct 27, 2017

zlib: finish migrating to internal/errors #16540

Closed

4 tasks

MylesBorins and others added 2 commits October 29, 2017 20:14

addaleax force-pushed the zlib-windowsbits-8 branch from 74d69ab to 25ef9d2 Compare October 29, 2017 19:14

addaleax closed this Oct 29, 2017

addaleax deleted the zlib-windowsbits-8 branch October 29, 2017 19:24

addaleax merged commit 25ef9d2 into nodejs:master Oct 29, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn added the backport-requested-v8.x label Oct 30, 2017

addaleax removed backport-requested-v8.x labels Oct 30, 2017

addaleax added the backported-to-v8.x label Oct 30, 2017

addaleax mentioned this pull request Oct 30, 2017

[v8.x] doc: more accurate zlib windowBits information #16623

Closed

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: gracefully set windowBits from 8 to 9 #16511

zlib: gracefully set windowBits from 8 to 9 #16511

addaleax commented Oct 26, 2017

lpinca Oct 26, 2017

addaleax Oct 26, 2017

maclover7 Oct 26, 2017

addaleax Oct 26, 2017

mhdawson left a comment

MylesBorins commented Oct 26, 2017

addaleax commented Oct 26, 2017

jasnell commented Oct 29, 2017

addaleax commented Oct 29, 2017

gibfahn commented Oct 30, 2017

lpinca commented Oct 30, 2017

lpinca commented Oct 30, 2017 •

edited

Loading

addaleax commented Oct 30, 2017

MylesBorins commented Nov 16, 2017

addaleax commented Nov 16, 2017

MylesBorins commented Nov 16, 2017 •

edited

Loading

zlib: gracefully set windowBits from 8 to 9 #16511

zlib: gracefully set windowBits from 8 to 9 #16511

Conversation

addaleax commented Oct 26, 2017

Checklist

Affected core subsystem(s)

lpinca Oct 26, 2017

Choose a reason for hiding this comment

addaleax Oct 26, 2017

Choose a reason for hiding this comment

maclover7 Oct 26, 2017

Choose a reason for hiding this comment

addaleax Oct 26, 2017

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

MylesBorins commented Oct 26, 2017

addaleax commented Oct 26, 2017

jasnell commented Oct 29, 2017

addaleax commented Oct 29, 2017

gibfahn commented Oct 30, 2017

lpinca commented Oct 30, 2017

lpinca commented Oct 30, 2017 • edited Loading

addaleax commented Oct 30, 2017

MylesBorins commented Nov 16, 2017

addaleax commented Nov 16, 2017

MylesBorins commented Nov 16, 2017 • edited Loading

lpinca commented Oct 30, 2017 •

edited

Loading

MylesBorins commented Nov 16, 2017 •

edited

Loading