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

crypto: simplify internal state handling by tniessen · Pull Request #23648 · nodejs/node · GitHub

/ node Public

crypto: simplify internal state handling - #23648

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-diffiehellman-internal-state
ConversationCommits (2)ChecksFiles changed
Closed

crypto: simplify internal state handling#23648
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-diffiehellman-internal-state

Conversation

tniessen commented Oct 13, 2018

Copy link
Copy Markdown
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

crypto: simplify internal state handling
9cb55b7 
Uninitialized instances are not exposed to users, so this condition should
always be true.

nodejs-github-bot commented Oct 13, 2018

Copy link
Copy Markdown
Collaborator

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1235/pipeline

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
bnoordhuis approved these changes Oct 14, 2018
View reviewed changes

bnoordhuis 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

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

tniessen commented Oct 14, 2018

Copy link
Copy Markdown
Member Author

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

tniessen added a commit that referenced this pull request Oct 17, 2018
crypto: simplify internal state handling
e2f58c7 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

tniessen commented Oct 17, 2018

Copy link
Copy Markdown
Member Author

Landed in e2f58c7.

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

Refs: nodejs#23648
tniessen mentioned this pull request Oct 17, 2018
Closed
2 tasks
jasnell pushed a commit that referenced this pull request Oct 17, 2018
crypto: simplify internal state handling
6975639 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
tniessen added a commit that referenced this pull request Oct 19, 2018
crypto: remove DiffieHellman.initialised_
c0d86b9 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

refack commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

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 commented Oct 20, 2018

Copy link
Copy Markdown
Contributor

Suggested fix in #23778

addaleax pushed a commit that referenced this pull request Oct 20, 2018
crypto: simplify internal state handling
7744e84 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
crypto: remove DiffieHellman.initialised_
a37f7c2 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
crypto: remove DiffieHellman.initialised_
f4d1d9c 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
crypto: simplify internal state handling
3ca41dc 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
crypto: remove DiffieHellman.initialised_
f216699 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
crypto: simplify internal state handling
ce0c125 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
crypto: remove DiffieHellman.initialised_
be76a82 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
crypto: simplify internal state handling
4b98ff2 
Uninitialized instances are not exposed to users, so this condition should
always be true.

PR-URL: #23648
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
crypto: remove DiffieHellman.initialised_
806242b 
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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere mentioned this pull request Nov 29, 2018
Merged
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
+1 more reviewer
refack refack 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL