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: callback's error of crypto.pbkdf2 is set to undefined #18458

Closed
wants to merge 1 commit into from
Closed

crypto: callback's error of crypto.pbkdf2 is set to undefined #18458

wants to merge 1 commit into from

Conversation

BufoViridis
Copy link
Contributor

@BufoViridis BufoViridis commented Jan 30, 2018 •
edited
Loading

As the documentation for crypto.pbkdf2 states, the function on success will set the first argument to null.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@BufoViridis
update node_crypto.cc
dbd212a 
As the documentation for crypto.pbkdf2 (https://nodejs.org/dist/latest-v8.x/docs/api/crypto.html#crypto_crypto_pbkdf2_password_salt_iterations_keylen_digest_callback) say, the function on success will return the first argument set to null and not undefined.
@nodejs-github-bot 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 Jan 30, 2018
@tniessen
Copy link
Member

cc @nodejs/crypto, are we being consistent here?

bnoordhuis
bnoordhuis approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis 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 a regression test would be nice.

@tniessen Most callbacks in core use null.

tniessen
tniessen approved these changes Jan 30, 2018
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 with @bnoordhuis' comment.

indutny
indutny approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM

@tniessen tniessen self-assigned this Jan 30, 2018
@tniessen
Copy link
Member

The commit message should be refined when landing.

@tniessen
Copy link
Member

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 6, 2018
@BufoViridis @BridgeAR
src: fix crypto.pbkdf2 callback error argument
6a29630 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: nodejs#18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member

BridgeAR commented Feb 6, 2018

Landed in 6a29630

@BufoViridis thanks a lot! I fixed the subsystem and the commit message according to our guidlines while landing the PR.

@BridgeAR BridgeAR closed this Feb 6, 2018
@BufoViridis BufoViridis deleted the patch-1 branch February 7, 2018 16:49
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BufoViridis @MylesBorins
src: fix crypto.pbkdf2 callback error argument
984570d 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BufoViridis @MylesBorins
src: fix crypto.pbkdf2 callback error argument
635ea60 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@BufoViridis @MylesBorins
src: fix crypto.pbkdf2 callback error argument
3ec3c32 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
@BufoViridis @MylesBorins
src: fix crypto.pbkdf2 callback error argument
b48ca0a 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: #18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@BufoViridis @MayaLekova
src: fix crypto.pbkdf2 callback error argument
022c830 
Callbacks should always return `null` instead of `undefined` if no
error occurred.

PR-URL: nodejs#18458
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@indutny indutny indutny approved these changes

Assignees

@tniessen tniessen

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
8 participants
@BufoViridis @tniessen @BridgeAR @indutny @bnoordhuis @jasnell @MylesBorins @nodejs-github-bot