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.

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

src: make root_cert_vector function scoped #12788

Closed

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 2, 2017

root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

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

src

root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.
@nodejs-github-bot 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 May 2, 2017
@danbev
Copy link
Contributor Author

danbev commented May 2, 2017

@addaleax
Copy link
Member

addaleax commented May 2, 2017

LGTM but it might be good for somebody from @nodejs/crypto to confirm that this is really really safe ;)

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but let's definitely get @shigeki or @indutny to sign off

Copy link
Contributor

@shigeki shigeki left a comment

Choose a reason for hiding this comment

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

I've confirmed that it is initialized with builtin root certs only at the first time and works fine. LGTM.

@danbev
Copy link
Contributor Author

danbev commented May 4, 2017

test/windows-fanned does not look related to this:

not ok 356 sequential/test-benchmark-child-process
  ---
  duration_ms: 60.128
  severity: fail
  stack: |-
    timeout

danbev added a commit to danbev/node that referenced this pull request May 4, 2017
root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

PR-URL: nodejs#12788
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented May 4, 2017

Landed in d0c968e

@danbev danbev closed this May 4, 2017
@danbev danbev deleted the crypo-make-root_certs_vector-static branch May 4, 2017 12:41
@refack
Copy link
Contributor

refack commented May 4, 2017

test/windows-fanned does not look related to this:

Yep, related to #12821

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

PR-URL: nodejs#12788
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Is this okay for v6.x? It cherry-picks cleanly if so.

@sam-github
Copy link
Contributor

@addaleax why not land? not necessary?

#13194 would cherry-pick clean if this was landed, and this still cherry picks clean

@MylesBorins I recommend landing #12788 and then #13194, no conflicts if you do it in that order.

@addaleax
Copy link
Member

@addaleax why not land? not necessary?

Yes, in combination with a tiny chance of breakage. I assume it’s a good sign nobody complained about this change, so I don’t feel strongly. :)

sam-github pushed a commit that referenced this pull request Jul 21, 2017
root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

PR-URL: #12788
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@sam-github
Copy link
Contributor

@addaleax OK, wanted to make sure there wasn't some more fundamental problem with it.

landed on v6.x-staging, @nodejs/backporting

MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

PR-URL: #12788
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 12, 2017
root_cert_vector currently has file scope and external linkage, but is
only used in the NewRootCertsStore function. If this is not required to
be externally linked perhaps it can be changed to be static and function
scoped instead.

PR-URL: #12788
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants