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: optimize sign.update() and verify.update() #31767

Closed
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.

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

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

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Mar 11, 2020
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
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
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
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
@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

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. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants