Skip to content

src: minor cleanup and simplification of crypto::Hash#35651

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:crypto-hash-cleanup
Closed

src: minor cleanup and simplification of crypto::Hash#35651
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:crypto-hash-cleanup

Conversation

@jasnell
Copy link
Copy Markdown
Member

@jasnell jasnell commented Oct 14, 2020 •
edited
Loading

Some minor cleanups and simplification for crypto::Hash

@addaleax @targos... I was working on this because valgrind --leak-check=full is reporting the following when crypto.createHash() is called but I can't seem to be able to track down the uninitialized memory...

==29245== Conditional jump or move depends on uninitialised value(s)
==29245==    at 0xA985F9: node::crypto::Hash::New(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/james/node/node/out/Release/node)
==29245==    by 0xBB3E9A: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (in /home/james/node/node/out/Release/node)
==29245==    by 0xBB4D57: v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) (in /home/james/node/node/out/Release/node)
==29245==    by 0x14B1278: Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit (in /home/james/node/node/out/Release/node)
==29245==    by 0x14466A0: Builtins_JSBuiltinsConstructStub (in /home/james/node/node/out/Release/node)
==29245==    by 0x152EE22: Builtins_ConstructHandler (in /home/james/node/node/out/Release/node)
==29245==    by 0x144A521: Builtins_InterpreterEntryTrampoline (in /home/james/node/node/out/Release/node)
==29245==    by 0x14465A9: Builtins_JSConstructStubGeneric (in /home/james/node/node/out/Release/node)
==29245==    by 0x152EE22: Builtins_ConstructHandler (in /home/james/node/node/out/Release/node)
==29245==    by 0x144A521: Builtins_InterpreterEntryTrampoline (in /home/james/node/node/out/Release/node)
==29245==    by 0x1444458: Builtins_ArgumentsAdaptorTrampoline (in /home/james/node/node/out/Release/node)
==29245==    by 0x144A521: Builtins_InterpreterEntryTrampoline (in /home/james/node/node/out/Release/node)
==29245==  Uninitialised value was created by a stack allocation
==29245==    at 0xA98310: node::crypto::Hash::New(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/james/node/node/out/Release/node)

After digging in, I've tracked down the above warning to the HashInit call inside Hash::New ... But, here's the fun part, adding any printf inside HashInit makes the initialized jump/move disappear.

This depends on the recent semver-major crypto refactor but can be backported.

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

@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. dont-land-on-v14.x labels Oct 14, 2020
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 14, 2020

Review requested:

  • @nodejs/crypto
  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 14, 2020
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 15, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Trott
Trott previously approved these changes Oct 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Micro-nit that can totally be ignored: First word after the subsystem in the commit message should be an imperative verb. Maybe src: simplify crypto::Hash implementation?

@Trott Trott dismissed their stale review October 15, 2020 11:27

LGTM but I'm going to wait for a domain expert to weigh in first....

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Oct 16, 2020 •
edited by jasnell
Loading

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

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Oct 16, 2020

ping @nodejs/crypto @tniessen @addaleax

(btw, @tniessen ... I noticed that you're not in the nodejs/crypto team... really do think you'd be a great addition there)

@tniessen
Copy link
Copy Markdown
Member

tniessen commented Oct 16, 2020

(btw, @tniessen ... I noticed that you're not in the nodejs/crypto team... really do think you'd be a great addition there)

That's really weird, I am sure I've been in that team for years. Thanks for the ping.

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Oct 16, 2020

I may have just overlooked your name in the list.

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2020
@github-actions github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2020
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Oct 19, 2020

Landed in 1103b15...84a7880

@github-actions github-actions Bot closed this Oct 19, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 19, 2020
@jasnell @nodejs-github-bot
src: minor cleanup and simplification of crypto::Hash
84a7880 
Signed-off-by: James M Snell <[email protected]>

PR-URL: #35651
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jasnell @nodejs-github-bot @tniessen @Trott