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

crypto: simplify internal state handling #23648

Conversation

tniessen
Copy link
Member

Uninitialized DH instances are never exposed to users, so this condition should always be true.

(This also increases coverage 🎉)

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

Uninitialized instances are not exposed to users, so this condition should
always be true.
@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 Oct 13, 2018
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM

I suspect you could get rid of initialised_ altogether since at no point should instances where initialised_ == true leak to JS land.

@tniessen
Copy link
Member Author

tniessen added a commit that referenced this pull request Oct 17, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@tniessen
Copy link
Member Author

Landed in e2f58c7.

@tniessen tniessen closed this Oct 17, 2018
tniessen added a commit to tniessen/node that referenced this pull request Oct 17, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialised when exposed to users.

Refs: nodejs#23648
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
tniessen added a commit that referenced this pull request Oct 19, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@refack
Copy link
Contributor

refack commented Oct 20, 2018

This borkes Travis that uses unsupported 😣GCC 4.8

../src/node_crypto.cc:3641:29: error: no matching function for call to 'get'
    return sign->CheckThrow(std::get<Error>(ret));
                            ^~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/utility:142:5: note: candidate template ignored: invalid explicitly-specified argument for template parameter '_Int'
    get(std::pair<_Tp1, _Tp2>& __in) noexcept
    ^
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/utility:147:5: note: candidate template ignored: invalid explicitly-specified argument for template parameter '_Int'
    get(std::pair<_Tp1, _Tp2>&& __in) noexcept
    ^
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/utility:152:5: note: candidate template ignored: invalid explicitly-specified argument for template parameter '_Int'
    get(const std::pair<_Tp1, _Tp2>& __in) noexcept
    ^
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/array:268:5: note: candidate template ignored: invalid explicitly-specified argument for template parameter '_Int'
    get(array<_Tp, _Nm>& __arr) noexcept
    ^
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/array:277:5: note: candidate template ignored: invalid explicitly-specified argument for template parameter '_Int'

@refack
Copy link
Contributor

refack commented Oct 20, 2018

Suggested fix in #23778

addaleax pushed a commit that referenced this pull request Oct 20, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
As pointed out by Ben Noordhuis, this internal field can be removed
since all instances are initialized when exposed to users.

PR-URL: #23717
Refs: #23648
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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.

5 participants