FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix memory leaks in cert validation by Nibbler999 · Pull Request #12089 · nodejs/node · GitHub

/ node Public

crypto: fix memory leaks in cert validation - #12089

Closed
Nibbler999 wants to merge 1 commit into
nodejs:masterfrom
Nibbler999:fix-cert-leak
ConversationCommits (1)ChecksFiles changed
Closed

crypto: fix memory leaks in cert validation#12089
Nibbler999 wants to merge 1 commit into
nodejs:masterfrom
Nibbler999:fix-cert-leak

Conversation

Nibbler999 commented Mar 28, 2017

Copy link
Copy Markdown

The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

crypto

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Mar 28, 2017

gibfahn commented Mar 28, 2017

Copy link
Copy Markdown
Member

cc/ @nodejs/crypto

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

I will take a look at this right now.

addaleax mentioned this pull request Mar 28, 2017
Closed
addaleax added the memory Issues and PRs related to the memory management or memory footprint. label Mar 28, 2017

addaleax commented Mar 28, 2017

Copy link
Copy Markdown
Member

Maybe Fixes: #12033?

CI: https://ci.nodejs.org/job/node-test-commit/8728/

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

@addaleax Probably, yes.

seishun commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

Have we ever considered adding RAII wrappers for such functions to prevent memory leaks in the future?

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

The fix is good. I will check the memory usage to see if there is no other memory growth.

indutny approved these changes Mar 28, 2017
View reviewed changes

indutny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One minor nit, otherwise LGTM. Good catch!

Comment thread src/node_crypto.cc Outdated

indutny Mar 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we just store the result to int cmp and free before the condition test?

cjihrig approved these changes Mar 28, 2017
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like @indutny's suggestion.

crypto: fix memory leak if certificate is revoked
785b766 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033

Nibbler999 commented Mar 28, 2017

Copy link
Copy Markdown
Author

Updated as suggested.

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

Here are the graph of rss profile up to 100,000 tls.connect to two servers (verify ok and revoked with SmartCom filter). It obviously shows that this fixes the memory leaks.

@Nibbler999 Thanks for finding and fixing this.
A new CI is running on https://ci.nodejs.org/job/node-test-pull-request/7074/.
After it is green, I would like to land this and ask for a new release.
Do we need to wait for 48 hours?

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

CI results are all green.

indutny approved these changes Mar 28, 2017
View reviewed changes

indutny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

MylesBorins approved these changes Mar 28, 2017
View reviewed changes

MylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rubber Stamp LGTM

CI is green.

MylesBorins commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

I vote that we skip 48 hours and land immediately so we can do a v7.x release with this asap. Then we can backport to v4.x and v6.x and get a new release out for LTS ASAP as well

shigeki commented Mar 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Okay, @MylesBorins , @sam-github , @addaleax and I agree it and this have enough approvals. I'll make landing this. [Edited]

shigeki pushed a commit that referenced this pull request Mar 28, 2017
crypto: fix memory leak if certificate is revoked
a6f9494 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Mar 28, 2017
Closed

shigeki commented Mar 28, 2017

Copy link
Copy Markdown
Contributor

Thanks for everyone making reviewing so quickly. Landed in a6f9494.
@Nibbler999 I really appreciate your fix.

@MylesBorins I would like to ask you to prepare new releases.

shigeki closed this Mar 28, 2017
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
crypto: fix memory leak if certificate is revoked
a64aa44 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins commented Mar 29, 2017

Copy link
Copy Markdown
Contributor

This has now been release in v7.8.0.

Will release v4.8.2-rc.1 and v6.10.2-rc.1 tomorrow

philippevk mentioned this pull request Mar 29, 2017
Closed
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
crypto: fix memory leak if certificate is revoked
0490736 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
crypto: fix memory leak if certificate is revoked
5f644d2 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
crypto: fix memory leak if certificate is revoked
9d7fba4 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: #9469
Fixes: #12033
PR-URL: #12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins added a commit that referenced this pull request Mar 29, 2017
2017-04-04, Version 4.8.2 'Argon' (LTS)
006622b 
This is a special LTS to fix a memory leak that was introduced
in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
MylesBorins mentioned this pull request Mar 29, 2017
Merged
MylesBorins added a commit that referenced this pull request Mar 29, 2017
2017-04-04, Version 6.10.2 'Boron' (LTS)
be03658 
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123
MylesBorins mentioned this pull request Mar 29, 2017
Merged
coox mentioned this pull request Mar 31, 2017
Closed
MylesBorins added a commit that referenced this pull request Apr 4, 2017
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
ea2ceac 
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
MylesBorins added a commit that referenced this pull request Apr 4, 2017
2017-04-04, Version 6.10.2 'Boron' (LTS)
fbc9fde 
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    #10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) #12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    #12123
MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017
2017-04-04, Version 6.10.2 'Boron' (LTS)
28f8b17 
This is a special LTS to fix a number of regressions that were found
on the 6.10.x release line.

This includes:

 * a fix for memory leak in the crypto module that
   was introduced in 6.10.1
 * a fix for a regression introduced to the windows repl in 6.10.0
 * a backported fix for V8 to stop a segfault that could occur
   when using spread syntax

It also includes an upgrade to zlib 1.2.11 to fix a numberof low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
  - backport V8 fixes for spread syntax regression causing segfaults
    (Michaël Zasso) nodejs#12037
* repl:
  - Revert commit that broke REPL display on Windows (Myles Borins)
    nodejs#12123
MylesBorins added a commit to MylesBorins/node that referenced this pull request Apr 4, 2017
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
f7a3118 
This is a maintenance release to fix a memory leak that was
introduced in 4.8.1.

It also includes an upgrade to zlib 1.2.11 to fix a number of low
severity CVEs that were present in zlib 1.2.8.

http://seclists.org/oss-sec/2016/q4/602

Notable changes:

* crypto:
  - fix memory leak if certificate is revoked (Tom Atkinson)
    nodejs#12089
* deps:
  - upgrade zlib to 1.2.11 (Sam Roberts)
    nodejs#10980
italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
b82c539 
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
2017-04-04, Version 6.10.2 'Boron' (LTS)
7db63fb 
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
2017-03-28, Version 7.8.0 (Current)
e0661dd 
    Notable changes:

    * buffer:
      - do not segfault on out-of-range index (Timothy Gu)
        nodejs/node#11927
    * crypto:
      - Fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      * upgrade npm to 4.2.0 (Kat Marchán)
        nodejs/node#11389
      * fix async await desugaring in V8 (Michaël Zasso)
        nodejs/node#12004
    * readline:
      - add option to stop duplicates in history (Danny Nemer)
        nodejs/node#2982
    * src:
      - add native URL class (James M Snell)
        nodejs/node#11801

    PR-URL: nodejs/node#12104

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
2017-04-04, Version 4.8.2 'Argon' (Maintenance)
eee59a8 
    This is a maintenance release to fix a memory leak that was
    introduced in 4.8.1.

    It also includes an upgrade to zlib 1.2.11 to fix a number of low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes:

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
2017-04-04, Version 6.10.2 'Boron' (LTS)
d8517db 
    This is a special LTS to fix a number of regressions that were found
    on the 6.10.x release line.

    This includes:

     * a fix for memory leak in the crypto module that
       was introduced in 6.10.1
     * a fix for a regression introduced to the windows repl in 6.10.0
     * a backported fix for V8 to stop a segfault that could occur
       when using spread syntax

    It also includes an upgrade to zlib 1.2.11 to fix a numberof low
    severity CVEs that were present in zlib 1.2.8.

    http://seclists.org/oss-sec/2016/q4/602

    Notable changes

    * crypto:
      - fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      - upgrade zlib to 1.2.11 (Sam Roberts)
        nodejs/node#10980
      - backport V8 fixes for spread syntax regression causing segfaults
        (Michaël Zasso) nodejs/node#12037
    * repl:
      - Revert commit that broke REPL display on Windows (Myles Borins)
        nodejs/node#12123

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
mjmasn mentioned this pull request Apr 25, 2017
Closed
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
crypto: fix memory leak if certificate is revoked
9b1683e 
The additional validity checks applied to StartCom and WoSign
certificates failed to free memory before returning.

Refs: nodejs/node#9469
Fixes: nodejs/node#12033
PR-URL: nodejs/node#12089
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
cjihrig cjihrig approved these changes
+4 more reviewers
sam-github sam-github approved these changes
indutny indutny approved these changes
MylesBorins MylesBorins approved these changes
shigeki shigeki approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. memory Issues and PRs related to the memory management or memory footprint.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL