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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

bnoordhuis
Copy link
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
Copy link
Collaborator

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

@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
Copy link
Member

@tniessen tniessen 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 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
Copy link
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
Copy link
Member

tniessen commented Mar 5, 2020

@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
Copy link
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
Copy link
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
Copy link
Member

targos commented Apr 25, 2020

The second commit doesn't land cleanly on v12.x because it's on top of a semver-major change. Please open a backport PR or change the label to dont-land-on-v12.x

@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

@devnexen devnexen devnexen approved these changes

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