zlib: fix crash when initializing failed - #14666
Conversation
Sorry, something went wrong.
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
|
CI: https://ci.nodejs.org/job/node-test-commit/11594/ |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
+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? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
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). |
Sorry, something went wrong.
aqrln
left a comment
There was a problem hiding this comment.
Thank you!
Sorry, something went wrong.
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>
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>
|
This has backported cleanly to v6.x |
Sorry, something went wrong.
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>
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. |
Sorry, something went wrong.
|
I probably would backport both fixes, but not backporting either also seems fine to me. |
Sorry, something went wrong.
|
We are getting very close to a v4.x release. Could this please be backported to v4.x |
Sorry, something went wrong.
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 both fixes backported in #14860. |
Sorry, something went wrong.
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>
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>
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
Affected core subsystem(s)
src/zlib