Skip to content

tls: fix segfault on destroy after partial read #11898

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
Merged
merged 1 commit into from
Mar 20, 2017
Merged

tls: fix segfault on destroy after partial read #11898

merged 1 commit into from
Mar 20, 2017

Conversation

bnoordhuis
Copy link
Member

Refs: #11885

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Mar 17, 2017
@bnoordhuis bnoordhuis mentioned this pull request Mar 17, 2017
Closed
cjihrig
cjihrig approved these changes Mar 17, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Confirmed that the test segfaults without this patch.

@Fishrock123
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/6905/

@bnoordhuis
Copy link
Member Author

Curious, I started https://ci.nodejs.org/job/node-test-pull-request/6898/ but it didn't show up?

@bnoordhuis
tls: fix segfault on destroy after partial read
03a6c6e 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@bnoordhuis bnoordhuis closed this Mar 20, 2017
@bnoordhuis bnoordhuis deleted the fix11885 branch March 20, 2017 15:53
@bnoordhuis bnoordhuis merged commit 03a6c6e into nodejs:master Mar 20, 2017
@jcstanaway
Copy link

Will this get merged into the 6.x release? If so, when would this be expected to happen?

@addaleax
Copy link
Member

@ccs018 The general rule is that a patch has to live for 2 weeks in a Current (v7.x) release before it gets backported to the LTS branches.

@jcstanaway
Copy link

Thanks. Just trying to determine if we need to build a private patched version or wait for an official release.

@bnoordhuis
Copy link
Member Author

Lead time between 'fix merged' and 'available in a LTS release' is on the order of a month, assuming no regressions.

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@bnoordhuis @jungx098
tls: fix segfault on destroy after partial read
77627bd 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas
Copy link

This PR need backport to v7

@bnoordhuis
Copy link
Member Author

@italoacasas How are you trying to merge this into v7? It should apply cleanly, there have been no other changes.

@italoacasas
Copy link

italoacasas commented Mar 21, 2017 •
edited
Loading

@bnoordhuis yeah don't worry, I'm sick and I was not thinking clearly.

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 21, 2017
@bnoordhuis
tls: fix segfault on destroy after partial read
c626734 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs#11885
PR-URL: nodejs#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 21, 2017
Merged
cjihrig added a commit that referenced this pull request Mar 21, 2017
@cjihrig
2017-03-21, Version 7.7.4 (Current)
44b4c0b 
Notable changes:

* deps: Add node-inspect 1.10.6 (Jan Krems) #11869
* inspector: proper WS URLs when bound to 0.0.0.0 (Eugene Ostroukhov) #11850
* tls: fix segfault on destroy after partial read. (Ben Noordhuis) #11898

PR-URL: #11941
cjihrig added a commit that referenced this pull request Mar 21, 2017
@cjihrig
2017-03-21, Version 7.7.4 (Current)
7e6e7d3 
Notable changes:

* deps: Add node-inspect 1.10.6 (Jan Krems) #11869
* inspector: proper WS URLs when bound to 0.0.0.0 (Eugene Ostroukhov) #11850
* tls: fix segfault on destroy after partial read. (Ben Noordhuis) #11898

PR-URL: #11941
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@bnoordhuis @MylesBorins
tls: fix segfault on destroy after partial read
31dc04e 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@bnoordhuis @MylesBorins
tls: fix segfault on destroy after partial read
cd698e0 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@bnoordhuis @MylesBorins
tls: fix segfault on destroy after partial read
54f5258 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@bnoordhuis @MylesBorins
tls: fix segfault on destroy after partial read
d0868ff 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This was referenced Apr 19, 2017
Merged
Merged
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-21, Version 7.7.4 (Current)
dabd3a5 
    Notable changes:

    * deps: Add node-inspect 1.10.6 (Jan Krems) nodejs/node#11869
    * inspector: proper WS URLs when bound to 0.0.0.0 (Eugene Ostroukhov) nodejs/node#11850
    * tls: fix segfault on destroy after partial read. (Ben Noordhuis) nodejs/node#11898

    PR-URL: nodejs/node#11941

Signed-off-by: Ilkka Myller <[email protected]>
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
788f280 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 6.10.3 'Boron' (LTS)
a0bfd4f 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9802d46 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12499
MylesBorins added a commit that referenced this pull request May 2, 2017
@MylesBorins
2017-05-02, Version 6.10.3 'Boron' (LTS)
20e27dd 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) #9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) #11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) #11947
   * (Ben Noordhuis) #11898
   * (jBarz) #11776

PR-URL: #12497
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
9c2a002 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 6.10.3 'Boron' (LTS)
6c7c224 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
27a8c43 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12499

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
@imyller
2017-05-02, Version 6.10.3 'Boron' (LTS)
3d61075 
    Notable Changes:

    * module:
      - The module loading global fallback to the Node executable's
        directory now works correctly on Windows.
        (Richard Lau) nodejs/node#9283
    * src:
      - fix base64 decoding in rare edgecase
        (Nikolai Vavilov) nodejs/node#11995
    * tls:
      - fix rare segmentation faults when using TLS
       * (Trevor Norris) nodejs/node#11947
       * (Ben Noordhuis) nodejs/node#11898
       * (jBarz) nodejs/node#11776

    PR-URL: nodejs/node#12497

Signed-off-by: Ilkka Myller <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@MylesBorins
2017-05-02, Version 4.8.3 'Argon' (Maintenance)
38a7454 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12499
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
@MylesBorins
2017-05-02, Version 6.10.3 'Boron' (LTS)
f25ca7a 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs#11947
   * (Ben Noordhuis) nodejs#11898
   * (jBarz) nodejs#11776

PR-URL: nodejs#12497
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@bnoordhuis @andrew749
tls: fix segfault on destroy after partial read
72a5b0d 
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: nodejs/node#11885
PR-URL: nodejs/node#11898
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@MylesBorins @andrew749
2017-05-02, Version 6.10.3 'Boron' (LTS)
94b893c 
Notable Changes:

* module:
  - The module loading global fallback to the Node executable's
    directory now works correctly on Windows.
    (Richard Lau) nodejs/node#9283
* src:
  - fix base64 decoding in rare edgecase
    (Nikolai Vavilov) nodejs/node#11995
* tls:
  - fix rare segmentation faults when using TLS
   * (Trevor Norris) nodejs/node#11947
   * (Ben Noordhuis) nodejs/node#11898
   * (jBarz) nodejs/node#11776

PR-URL: nodejs/node#12497
@sam-github sam-github mentioned this pull request Aug 3, 2017
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bnoordhuis @Fishrock123 @jcstanaway @addaleax @italoacasas @jasnell @cjihrig @MylesBorins @nodejs-github-bot