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 use after null when calling .close #5982

Closed
wants to merge 1 commit into from
Closed

zlib: Fix use after null when calling .close #5982

wants to merge 1 commit into from

Conversation

lightsofapollo
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

  • zlib

Description of change

An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

@lightsofapollo
zlib: Fix use after null when calling .close
094e34e 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.
@Fishrock123 Fishrock123 added the zlib Issues and PRs related to the zlib subsystem. label Mar 31, 2016
@mscdex
Copy link
Contributor

mscdex commented Mar 31, 2016

LGTM

@addaleax
Copy link
Member

LGTM too, if that’s worth anything. Adding a test for this should not be too complicated, I think?

@lightsofapollo
Copy link
Contributor Author

I will give a shot adding a test tonight thanks for quick r?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

LGTM. +1 to a test.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

LGTM with a test

@headlessme headlessme mentioned this pull request Apr 4, 2016
Closed
@xPaw xPaw mentioned this pull request Apr 12, 2016
Closed
@jasnell
Copy link
Member

jasnell commented Apr 13, 2016

ping @lightsofapollo ... have you had a chance to look at the test yet?

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/2962/
I’m going to land this and add a regression test based on the code #6034 if it comes out green.

@addaleax addaleax mentioned this pull request Apr 19, 2016
Closed
3 tasks
@addaleax
Copy link
Member

addaleax commented Apr 19, 2016 •
edited
Loading

Landed in c7fef3d, and added those as reviewers whose LGTMs I read as not necessarily depending on a test in this PR. PR for the regression test: #6270

@addaleax addaleax closed this Apr 19, 2016
addaleax pushed a commit that referenced this pull request Apr 19, 2016
@lightsofapollo @addaleax
zlib: fix use after null when calling .close
c7fef3d 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@lightsofapollo
zlib: fix use after null when calling .close
bad006f 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Apr 21, 2016
Closed
Closed
@MylesBorins
Copy link
Contributor

needs to be included with f8e507e when backported

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@lightsofapollo @MylesBorins
zlib: fix use after null when calling .close
4f6f599 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@lightsofapollo @joelostrowski
zlib: fix use after null when calling .close
8dbd5c5 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: nodejs#5982
Fixes: nodejs#6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@lightsofapollo @jasnell
zlib: fix use after null when calling .close
bd944e3 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 17, 2016
@lightsofapollo
zlib: fix use after null when calling .close
0cc1e0f 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
@lightsofapollo
zlib: fix use after null when calling .close
48684af 
An internal zlib error may cause _handle to be set to null.
Close now will check if there is a _handle prior to calling .close on
it.

PR-URL: #5982
Fixes: #6034
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Merged
@fippo fippo mentioned this pull request Jul 7, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
7 participants
@lightsofapollo @mscdex @addaleax @cjihrig @jasnell @MylesBorins @Fishrock123