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: fix node crashing on invalid options #13098

Closed
wants to merge 6 commits into from
Closed

Conversation

aqrln
Copy link
Contributor

@aqrln aqrln commented May 18, 2017

This PR fixes the Node process crashing when constructors of classes of the zlib module are given invalid options.

  • Throw an Error when the zlib library rejects the value of windowBits, instead of crashing with an assertion.
  • Treat windowBits and memLevel options consistently with other ones and don't crash when non-numeric values are given.

Fixes: #13082

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

The main reason behind this commit is fixing the Node process crashing
when zlib rejects the given options.

Besides that issue, which got reported and which is linked to this
commit, it turned out that Node also used to crash when a non-numeric
value was passed as the `windowBits` or the `memLevel` option. This was
fixed somewhat inadvertently; initially it was just a stylistic change
to avoid lines spanning longer than 80 characters that was written in a
manner consistent with surrounding code.

Fixes: nodejs#13082
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. zlib Issues and PRs related to the zlib subsystem. labels May 18, 2017
@aqrln aqrln mentioned this pull request May 18, 2017
@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

@lpinca
Copy link
Member

lpinca commented May 18, 2017

@aqrln did something change with zlib 1.2.11?

zlib.createDeflateRaw({ windowBits: 8 })

worked fine with Node.js 6.10.1.

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

@lpinca frankly, I don't know. But the docs state that 8 is not a valid value of windowBits for raw deflate and gzip.

@lpinca
Copy link
Member

lpinca commented May 18, 2017

I wonder if it makes sense to use this remedy

The remedy is to not use 8 with deflateInit2() with this initialization, or at least in that case use 9 with inflateInit2().

in order to avoid the breaking change.

src/node_zlib.cc Outdated
SetDictionary(ctx);

args.GetReturnValue().Set(Boolean::New(args.GetIsolate(), result));
Copy link
Member

Choose a reason for hiding this comment

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

I’m pretty sure you don’t need Boolean::New 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.

Thanks, didn't know that :)

lib/zlib.js Outdated
process.nextTick(() => {
this.emit('error', error);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this error should still be thrown synchronously, if there is one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll make it impossible to catch this error since there's on object returned to userland code yet to listen to the error event on. That's basically the reason for all this machinery, otherwise @cjihrig's one-line fix would be just fine.

Copy link
Member

Choose a reason for hiding this comment

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

You could change the ZCtx::Error(ctx, "Init error"); line to throw an error instead … that should work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... yes, it should. And there would be no need for all those changes in zlib.js then.

src/node_zlib.cc Outdated
@@ -553,6 +556,9 @@ class ZCtx : public AsyncWrap {

if (ctx->err_ != Z_OK) {
ZCtx::Error(ctx, "Init error");
if (dictionary != nullptr)
delete[] dictionary;
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I’d have to admit I prefer the solution suggested by @cjihrig in #13082 (comment) (or just move this error-checking block to the end of the Init() function, after ctx->init_done_ = true)

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

@lpinca do you suggest making windowBits 9 when 8 is passed?

@lpinca
Copy link
Member

lpinca commented May 18, 2017

@aqrln yes use 15 (default value) when 8 is passed, not sure if it is a good idea though. Maybe it is better to do this in userland code.

Throw an Error synchronously instead of fiddling with 'error' events.
@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

@addaleax throwing an error synchronously indeed makes it a lot more elegant, thanks.

@lpinca I'm not sure about that too. I'd just document this limitation and not change user's options silently, but let's see what others think about it.

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a couple comments.

doc/api/zlib.md Outdated
@@ -437,6 +437,10 @@ added: v0.5.8

Returns a new [DeflateRaw][] object with an [options][].

**Note:** zlib library rejects requests for 256-byte windows (i.e.,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe start the note with "The"

lib/zlib.js Outdated
@@ -229,9 +229,15 @@ class Zlib extends Transform {
var strategy = constants.Z_DEFAULT_STRATEGY;
if (typeof opts.strategy === 'number') strategy = opts.strategy;

this._handle.init(opts.windowBits || constants.Z_DEFAULT_WINDOWBITS,
var windowBits = constants.Z_DEFAULT_WINDOWBITS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't these both just be ternaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could, and I'd like them to be, but I just wrote these in a style consistent with the code above. How about me addressing this comment in a follow-up PR, together with some more refactoring like replacing most vars with lets and consts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

// (http://zlib.net/manual.html#Advanced)
assert.throws(() => {
zlib.createDeflateRaw({ windowBits: 8 });
}, /Init error/);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you match the full error here using ^ and $.

@lpinca
Copy link
Member

lpinca commented May 18, 2017

Does is makes sense to throw early if opts.windowBits === 8? We already have a check here.

@addaleax
Copy link
Member

@lpinca I think it would. Are you thinking of doing that instead of this PR, or in addition to? I think the changes here are still good independently

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

@lpinca idk ¯\_(ツ)_/¯ Theoretically, it may change in the future:

For the current implementation of deflate(), a windowBits value of 8 (a window size of 256 bytes) is not supported.

@lpinca
Copy link
Member

lpinca commented May 18, 2017

I would make it in addition of these changes to make the error message consistent, but then the "Init error" would not be easy to test.

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

Yeah, we might do that just for the sake of making the error message more sensible. Seems like all the other ways to break deflateInit2() are already covered by the JavaScript checks.

@lpinca
Copy link
Member

lpinca commented May 18, 2017

Yes and this explains why it didn't crash before zlib 1.2.11.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with or without the additional check for opts.windowBits === 8.

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

That's quite easy to do (aqrln@bf63a37), but I am not really sure about it. I actually see the point in doing so, but being able to test that any sort of failure can be caught seems to be more important, so I'm inclined to leave this PR as it is now unless others would prefer to pull either that commit or something similar to it here.

@aqrln
Copy link
Contributor Author

aqrln commented May 18, 2017

CI for the latest changes: https://ci.nodejs.org/job/node-test-pull-request/8159/

lib/zlib.js Outdated
if (typeof opts.windowBits === 'number') windowBits = opts.windowBits;

var memLevel = constants.Z_DEFAULT_MEMLEVEL;
if (typeof opts.memLevel === 'number') memLevel = opts.memLevel;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change alters the existing behaviour a little. If we pass NaN or Zero, they used to pick the default value earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, thanks. It uncovers a whole bunch of bugs in the validation logic above, i.e., I wanted to say that zero would be caught before this line, but after trying it out it appeared to be not. And it was also possible to pass NaN as the value of some of the other options, like level.

@lpinca
Copy link
Member

lpinca commented May 18, 2017

so I'm inclined to leave this PR as it is now unless others would prefer to pull either that commit or something similar to it here.

I agree. As you said this may change again and 8 could be restored as valid value so it's probably better to keep this as is.

jasnell pushed a commit that referenced this pull request May 28, 2017
This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

* Fix bugs in the validation logic:
  - Don't conflate 0 and undefined when checking if a field of an
    options object exists.
  - Treat NaN and Infinity values the same way as values of invalid
    types instead of allowing to actually set zlib options to NaN or
    Infinity.

PR-URL: #13098
Fixes: #13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
@Fishrock123 Fishrock123 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 5, 2017
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

PR-URL: nodejs#13098
Backport-PR-URL: nodejs#13201
Fixes: nodejs#13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

PR-URL: #13098
Backport-PR-URL: #13201
Fixes: #13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

PR-URL: #13098
Backport-PR-URL: #13201
Fixes: #13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
MylesBorins added a commit that referenced this pull request Aug 1, 2017
This LTS release comes with 221 commits. This includes 80 which are
test related, 52 which are doc related, 32 which are build / tool
related and 10 commits which are updates to dependencies.

Notable Changes:

* configure:
  - add mips64el to valid_arch (Aditya Anand)
    - #13620
* crypto:
  - Updated root certificates based on [NSS 3.30] (Ben Noordhuis)
    - #13279
    - #12402
    - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.30_release_notes
* deps:
  - upgrade OpenSSL to version 1.0.2.l (Shigeki Ohtsu)
    - #12913
* http:
  - parse errors are now reported when NODE_DEBUG=http (Sam Roberts)
    - #13206
  - Agent construction can now be envoked without `new` (cjihrig)
    - #12927
* zlib:
  - node will now throw an Error when zlib rejects the value of windowBits,
    instead of crashing (Alexey Orlenko)
    - #13098

PR-URL: #14356
MylesBorins added a commit that referenced this pull request Aug 1, 2017
This LTS release comes with 221 commits. This includes 80 which are
test related, 52 which are doc related, 32 which are build / tool
related and 10 commits which are updates to dependencies.

Notable Changes:

* configure:
  - add mips64el to valid_arch (Aditya Anand)
    - #13620
* crypto:
  - Updated root certificates based on [NSS 3.30] (Ben Noordhuis)
    - #13279
    - #12402
    - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.30_release_notes
* deps:
  - upgrade OpenSSL to version 1.0.2.l (Shigeki Ohtsu)
    - #12913
* http:
  - parse errors are now reported when NODE_DEBUG=http (Sam Roberts)
    - #13206
  - Agent construction can now be envoked without `new` (cjihrig)
    - #12927
* zlib:
  - node will now throw an Error when zlib rejects the value of windowBits,
    instead of crashing (Alexey Orlenko)
    - #13098

PR-URL: #14356
addaleax added a commit to addaleax/node that referenced this pull request Aug 7, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: nodejs#14178
Ref: nodejs#13098
addaleax added a commit that referenced this pull request Aug 9, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
PR-URL: #14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
addaleax added a commit that referenced this pull request Aug 9, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
PR-URL: #14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
PR-URL: #14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
aqrln added a commit to aqrln/node that referenced this pull request Aug 16, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

PR-URL: nodejs#13098
Fixes: nodejs#13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
aqrln pushed a commit to aqrln/node that referenced this pull request Aug 16, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: nodejs#14178
Ref: nodejs#13098
PR-URL: nodejs#14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

Backport-PR-URL: #14860
PR-URL: #13098
Fixes: #13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
Backport-PR-URL: #14860
PR-URL: #14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
This is a partial backport of semver-patch bits of
9e4660b.

This commit fixes the Node process crashing when constructors of classes
of the zlib module are given invalid options.

* Throw an Error when the zlib library rejects the value of windowBits,
  instead of crashing with an assertion.

* Treat windowBits and memLevel options consistently with other ones and
  don't crash when non-numeric values are given.

Backport-PR-URL: #14860
PR-URL: #13098
Fixes: #13082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
Backport-PR-URL: #14860
PR-URL: #14666
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zlib assertion error