zlib: fix crash when initializing failed#14666
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 c++ 
nodejs-github-bot added c++ 
addaleax commented Aug 7, 2017
gibfahn commented Aug 7, 2017
Is this a regression? Looking at the description in #13098:
it sounds like zlib previously crashed under these circumstances anyway, so #13098 didn't make things worse. Is that correct? |
+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 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.
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 commented Aug 7, 2017
MylesBorins commented Aug 8, 2017
@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? |
gibfahn commented Aug 8, 2017
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). |
addaleax commented Aug 9, 2017
Landed in 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
MylesBorins commented Aug 11, 2017
This has backported cleanly to v6.x |
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
gibfahn commented Aug 12, 2017
If we're planning to do another v4.x already it probably makes sense, these are fixes for zlib |
addaleax commented Aug 12, 2017
I probably would backport both fixes, but not backporting either also seems fine to me. |
MylesBorins commented Aug 16, 2017
We are getting very close to a v4.x release. Could this please be backported to v4.x |
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
@MylesBorins both fixes backported in #14860. |
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
7 participants
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 inZCtx::Close().Fixes: #14178
Ref: #13098
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src/zlib