Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

simple fix for #2364 (zlib inflate/deflate/gunzip/gzip crashes when called on non-string input) #2385

Closed
wants to merge 4 commits into from

Conversation

nlacasse
Copy link

The bug symptom is that calling any of the zlib convenience methods with a non-string argument results in a crash.

The zlibBuffer function is calling removeListener without passing in the event listener itself.

This patch saves the event listeners in variables, and passes them to removeListener.

@bnoordhuis
Copy link
Member

@isaacs: You know the zlib code best. Can you review this patch when you're back?

@nlacasse
Copy link
Author

Any word on if/when this patch will be reviewed? Thanks!

@isaacs
Copy link

isaacs commented Jan 20, 2012

@nlacasse I don't get what this is doing. Can you include a test that fails without the patch, and passes with it?

Also, please prefer named function declarations like function onError() { rather than function expressions assigned to a variable like var onError = function() {.

@nlacasse
Copy link
Author

@isaacs I changed the function declarations as you asked, and I added a test.

The issue is that zlib is not calling "removeListener" correctly. So on an error (like that caused by invalid input), removeListener gets called improperly and throws an error. The expected behavior is for a useful error to be passed to the callback.

@piscisaureus
Copy link

@nlacasse Can you sign the CLA? http://nodejs.org/cla.html

@piscisaureus
Copy link

I made some minor edits and squashed everything. When the cla is signed we can land https://gist.github.com/1647651

@nlacasse
Copy link
Author

@piscisaureus CLA is signed. Thanks!

@isaacs
Copy link

isaacs commented Jan 20, 2012

Landed on 40c9348. Thanks!

@isaacs isaacs closed this Jan 20, 2012
@piscisaureus
Copy link

The related commit incorrectly references #2365. Adding backreference.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants