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::Hash update method error output is missing Buffer #25487

Closed
amitzur opened this issue Jan 14, 2019 · 7 comments
Closed

crypto::Hash update method error output is missing Buffer #25487

amitzur opened this issue Jan 14, 2019 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@amitzur
Copy link
Contributor

amitzur commented Jan 14, 2019

  • Version: 10.10.0
  • Platform: Linux amit-ThinkPad-X1-Carbon-6th 4.15.0-43-generic #46-Ubuntu SMP Thu Dec 6 14:45:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: crypto

The Hash class in the crypto module has an update method which may receive {string | Buffer | TypedArray | DataView} in the data argument.
(https://github.com/nodejs/node/blob/master/doc/api/crypto.md#hashupdatedata-inputencoding)

The error message output if an invalid argument type is passed is missing Buffer.
(https://github.com/nodejs/node/blob/master/lib/internal/crypto/hash.js#L65)

I believe this is a good first bug, and would like to fix it myself. AFAICT, a test should be added here:
https://github.com/nodejs/node/blob/master/test/parallel/test-crypto-hash.js

If this issue is verified, I just need a pointer to how to run the tests in test/parallel.

@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. labels Jan 14, 2019
@bnoordhuis
Copy link
Member

Thanks for the excellent bug report. Yes, that looks like a genuine oversight to me, pull request most welcome.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 14, 2019

If this issue is verified, I just need a pointer to how to run the tests in test/parallel.

make test-parallel or python tools/test.py -J parallel

(edit: or just make test to run everything)

@sam-github
Copy link
Contributor

Running make test is a good idea to do just before making a pull request. Besides just running the tests, it will check for style errors in the js and markdown files and other common problems, saving you from making the PR, and then having CI immediately fail it.

@amitzur
Copy link
Contributor Author

amitzur commented Jan 14, 2019

Great point. I'm working on it. Thanks!

@amitzur
Copy link
Contributor Author

amitzur commented Jan 15, 2019

Any idea why I'm getting this error during make -j4 test ?
btw, running python tools/test.py -J parallel/test-fs-stat-bigint.js works fine. I hope it's the same test.

AssertionError [ERR_ASSERTION]: atimeMs is not a safe integer, difference should < 1.
Number version 1547551430183.6504, BigInt version 1547551430179n
    at verifyStats (/home/amit/projects/node/test/parallel/test-fs-stat-bigint.js:72:7)
    at fs.lstat (/home/amit/projects/node/test/parallel/test-fs-stat-bigint.js:120:7)
    at FSReqCallback.oncomplete (fs.js:160:5)

Node version: both 10.10.0 and 8.12.0

@bnoordhuis
Copy link
Member

@amitzur That's probably #24593. Summary: flaky test (on some systems.)

@amitzur
Copy link
Contributor Author

amitzur commented Jan 16, 2019

thanks @bnoordhuis . I assumed this so I allowed myself to file a PR already. And indeed the checks passed.

targos pushed a commit that referenced this issue Feb 10, 2019
Fixes: #25487

PR-URL: #25533
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants