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 crash when initializing failed #14666

Closed
wants to merge 1 commit into from
Closed

zlib: fix crash when initializing failed #14666

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented 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: #14178
Ref: #13098

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src/zlib

@addaleax
zlib: fix crash when initializing failed
64da7ab 
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 addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x zlib Issues and PRs related to the zlib subsystem. labels Aug 7, 2017
@addaleax addaleax requested a review from aqrln August 7, 2017 15:18
@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 Aug 7, 2017
@addaleax addaleax mentioned this pull request Aug 7, 2017
Closed
@addaleax
Copy link
Member Author

addaleax commented Aug 7, 2017

CI: https://ci.nodejs.org/job/node-test-commit/11594/
Stress master: https://ci.nodejs.org/job/node-stress-single-test/1364/
Stress this PR: https://ci.nodejs.org/job/node-stress-single-test/1365/

@addaleax addaleax mentioned this pull request Aug 7, 2017
Closed
4 tasks
@gibfahn
Copy link
Member

gibfahn commented Aug 7, 2017

Is this a regression?

Looking at the description in #13098:

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.

it sounds like zlib previously crashed under these circumstances anyway, so #13098 didn't make things worse. Is that correct?

@gibfahn
Copy link
Member

gibfahn commented Aug 7, 2017 •
edited
Loading

+1 on fast-tracking this, if only because it'll make CI less red.

Is there an easy way to add a (less intermittently failing) test for this?

@addaleax
Copy link
Member Author

addaleax commented Aug 7, 2017

@gibfahn Right, it’s only going from an assertion failure to a segfault. It’s a bit more scary to see that happening, but it won’t really hurt anyone.

Is there an easy way to add a (less intermittent) test for this?

I don’t know, the failure is only happening because there’s uninitialized memory involved. I guess the best way to make sure this doesn’t happen again is to check the return value of the zlib cleanup functions, but tbh I’m scared to do that in a patch release given how complex the zlib state machine is.

@addaleax addaleax mentioned this pull request Aug 7, 2017
Closed
2 tasks
@addaleax
Copy link
Member Author

addaleax commented Aug 7, 2017

@gibfahn I opened #14673, checking return values doesn’t seem to be enough to test this. I think more detailed CHECKs would require inspecting zlib internals.

@MylesBorins
Copy link
Contributor

@addaleax it was only partially backported... and we have not been seeing the failures on 6.x yet

Should we just revert the original change and call it a day?

@MylesBorins MylesBorins mentioned this pull request Aug 8, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Aug 8, 2017

Should we just revert the original change and call it a day?

I'd rather land this if there are no problems with it. Reverting the original means zlib will still crash, which according to #13082 was a regression from 6.10.1->6.10.2 (due to the zlib update).

aqrln
aqrln approved these changes Aug 9, 2017
View reviewed changes
Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

Thank you!

@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2017

Landed in 1e569f4

@addaleax addaleax closed this Aug 9, 2017
@addaleax addaleax deleted the zlib-failed-init branch August 9, 2017 17:59
addaleax added a commit that referenced this pull request Aug 9, 2017
@addaleax
zlib: fix crash when initializing failed
1e569f4 
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
@addaleax
zlib: fix crash when initializing failed
a813294 
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
Copy link
Contributor

This has backported cleanly to v6.x
do we need to backport both fixes to v4.x? we are getting close to enough backlog to justify a maintenance release

MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
@addaleax @MylesBorins
zlib: fix crash when initializing failed
e79c054 
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>
@gibfahn
Copy link
Member

gibfahn commented Aug 12, 2017

do we need to backport both fixes to v4.x?

If we're planning to do another v4.x already it probably makes sense, these are fixes for zlib 1.2.11 which is the version in v4.x.

cc/ @aqrln @addaleax @lpinca

@addaleax
Copy link
Member Author

I probably would backport both fixes, but not backporting either also seems fine to me.

@MylesBorins
Copy link
Contributor

We are getting very close to a v4.x release. Could this please be backported to v4.x

@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
aqrln pushed a commit to aqrln/node that referenced this pull request Aug 16, 2017
@addaleax @aqrln
zlib: fix crash when initializing failed
38316ff 
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>
@aqrln aqrln mentioned this pull request Aug 16, 2017
Closed
2 tasks
@aqrln
Copy link
Contributor

aqrln commented Aug 16, 2017 •
edited
Loading

@MylesBorins both fixes backported in #14860.

This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
@addaleax @MylesBorins
zlib: fix crash when initializing failed
5422eb8 
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
Closed
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@addaleax @MylesBorins
zlib: fix crash when initializing failed
a1f9929 
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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@cjihrig cjihrig cjihrig approved these changes

@aqrln aqrln aqrln approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.

test: parallel/test-zlib-failed-init, intermittent crash
7 participants
@addaleax @gibfahn @MylesBorins @aqrln @refack @cjihrig @nodejs-github-bot