crypto: optimize sign.update() and verify.update() by bnoordhuis · Pull Request #31767 · nodejs/node · GitHub
Skip to content

crypto: optimize sign.update() and verify.update()#31767

Closed
bnoordhuis wants to merge 3 commits into
nodejs:masterfrom
bnoordhuis:opt-sign-verify
Closed

crypto: optimize sign.update() and verify.update()#31767
bnoordhuis wants to merge 3 commits into
nodejs:masterfrom
bnoordhuis:opt-sign-verify

Conversation

@bnoordhuis

@bnoordhuis bnoordhuis commented Feb 13, 2020

Copy link
Copy Markdown
Member

The first commit is #31766.

Second commit:

Use StringBytes::InlineDecoder to decode strings inputs in C++ land
instead of decoding them to buffers in JS land before passing them on
to the C++ layer. This is what the other update() methods already did.

Third commit:

Factor out the common logic into a template function.
Removes approximately six instances of copy/pasted code.

@bnoordhuis
crypto: make update(buf, enc) ignore encoding
badfa84 
Make the cipher/decipher/hash/hmac update() methods ignore the input
encoding when the input is a buffer.

This is the documented behavior but some inputs were rejected, notably
when the specified encoding is 'hex' and the buffer has an odd length
(because a _string_ with an odd length is never a valid hex string.)

The sign/verify update() methods work okay because they use different
validation logic.

Fixes: nodejs#31751
@bnoordhuis
crypto: optimize sign.update() and verify.update()
66f6326 
Use `StringBytes::InlineDecoder` to decode strings inputs in C++ land
instead of decoding them to buffers in JS land before passing them on
to the C++ layer. This is what the other update() methods already did.
@bnoordhuis
src: DRY crypto Update() methods
f87e658 
Factor out the common logic into a template function.
Removes approximately six instances of copy/pasted code.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 13, 2020
@nodejs-github-bot

nodejs-github-bot commented Feb 13, 2020

Copy link
Copy Markdown
Collaborator

santigimeno
santigimeno reviewed Feb 13, 2020
View reviewed changes
Comment thread src/node_crypto.cc
@bnoordhuis bnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 15, 2020
tniessen
tniessen approved these changes Feb 23, 2020
View reviewed changes

@tniessen tniessen 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.

LGTM, but I am wondering if it makes sense to return a ByteSource instead of calling lambdas here? It doesn't solve the unwrapping, but the lambda obscures the control flow in my opinion. ByteSource can deal with static and dynamically allocated memory.

@bnoordhuis

bnoordhuis commented Feb 25, 2020

Copy link
Copy Markdown
Member Author

@tniessen I'm afraid I'm not following. If you want to open an alternate PR, I'll be happy to review it and abandon this one.

@tniessen

tniessen commented Mar 5, 2020

Copy link
Copy Markdown
Member

@bnoordhuis Sorry, I didn't have time to try that yet. I am okay with the change as it is, it is definitely an improvement, and if I ever end up attempting something else, we can still change it, so I would suggest to land this :-)

@tniessen tniessen added the crypto Issues and PRs related to the crypto subsystem. label Mar 5, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Mar 11, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax pushed a commit that referenced this pull request Mar 11, 2020
@bnoordhuis @addaleax
crypto: optimize sign.update() and verify.update()
3b9a403 
Use `StringBytes::InlineDecoder` to decode strings inputs in C++ land
instead of decoding them to buffers in JS land before passing them on
to the C++ layer. This is what the other update() methods already did.

PR-URL: #31767
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit that referenced this pull request Mar 11, 2020
@bnoordhuis @addaleax
src: DRY crypto Update() methods
47c2b67 
Factor out the common logic into a template function.
Removes approximately six instances of copy/pasted code.

PR-URL: #31767
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@addaleax

addaleax commented Mar 11, 2020

Copy link
Copy Markdown
Member

Landed in d09e1da...47c2b67

@addaleax addaleax closed this Mar 11, 2020
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
@bnoordhuis @MylesBorins
crypto: optimize sign.update() and verify.update()
ce686c0 
Use `StringBytes::InlineDecoder` to decode strings inputs in C++ land
instead of decoding them to buffers in JS land before passing them on
to the C++ layer. This is what the other update() methods already did.

PR-URL: #31767
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
@bnoordhuis @MylesBorins
src: DRY crypto Update() methods
e83671c 
Factor out the common logic into a template function.
Removes approximately six instances of copy/pasted code.

PR-URL: #31767
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
Merged
@targos targos added backport-blocked-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v12.x labels Apr 22, 2020
@targos

targos commented Apr 25, 2020

Copy link
Copy Markdown
Member

@joyeecheung joyeecheung mentioned this pull request Jun 8, 2020
Closed
3 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
@addaleax addaleax addaleax approved these changes
@santigimeno santigimeno santigimeno approved these changes
@cjihrig cjihrig cjihrig approved these changes
@tniessen tniessen tniessen approved these changes
+1 more reviewer
@devnexen devnexen devnexen 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. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@bnoordhuis @nodejs-github-bot @tniessen @addaleax @targos @jasnell @santigimeno @cjihrig @devnexen