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: refactor array buffer view validation #29683

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

This is just a refactoring to reduce code and computational overhead.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This is just a refactoring to reduce code and computational overhead.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 23, 2019
@@ -164,7 +150,8 @@ Cipher.prototype.update = function update(data, inputEncoding, outputEncoding) {
outputEncoding = outputEncoding || encoding;

if (typeof data !== 'string' && !isArrayBufferView(data)) {
throw invalidArrayBufferView('data', data);
throw new ERR_INVALID_ARG_TYPE(
'data', ['string', 'Buffer', 'TypedArray', 'DataView'], data);
Copy link
Member Author

Choose a reason for hiding this comment

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

@nodejs/crypto all other cases that validate the input like that convert strings to buffers before passing the data on. Is it intentional, that this is not done here?

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR Should we pull the author ready label until we get an answer for this question? Or are you comfortable landing as-is?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am comfortable to land this as-is. It's mainly about consistency and the behavior is not changed with this PR.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 25, 2019

CI passed, if that encourages anyone to take a closer look.

@Trott Trott added the review wanted PRs that need reviews. label Sep 25, 2019
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 25, 2019
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 1, 2019
@Trott
Copy link
Member

Trott commented Oct 1, 2019

Removed author label out of abundance of caution.

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Also: Should this be benchmarked?

@BridgeAR
Copy link
Member Author

BridgeAR commented Oct 1, 2019

@Trott this should not have any real performance impact. It might actually be faster due to some redundant typeof checks being removed.

BridgeAR added a commit that referenced this pull request Oct 1, 2019
This is just a refactoring to reduce code and computational overhead.

PR-URL: #29683
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Oct 1, 2019

Landed in eb05d68

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants