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: add Hash.prototype.copy() method #29910

Closed
wants to merge 2 commits into from

Conversation

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 9, 2019

Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903

@nodejs-github-bot

This comment has been minimized.

doc/api/crypto.md Show resolved Hide resolved
@overlookmotel

This comment has been minimized.

Copy link

overlookmotel commented Oct 10, 2019

Author of #29903 here. I wasn't able to help with implementation of this feature as I don't know C/C++. But if anything I can do here on docs or testing, or anything involving JS, please shout. Really appreciate this being added to Node.

Copy link

overlookmotel left a comment

Could be good to add to end of hash.digest() docs:

Use hash.copy() first if you wish to continue to add data to the hash after hash.digest().

of the current `Hash` object.

The optional `options` argument controls stream behavior. For XOF hash
functions such as `'shake256'`, the `outputLength` option can be used to

This comment has been minimized.

Copy link
@sam-github

sam-github Oct 10, 2019

Member

https://nodejs.org/api/stream.html#stream_new_stream_transform_options doesn't have an outputLength option!

It looks like the options are "whatever crypto.createHash() accepts", and that the crypto.createHash docs are wrong, https://nodejs.org/api/crypto.html#crypto_crypto_createhash_algorithm_options, and got copied here.

You could fix the createHash docs, but that's unrelated to this feature, perhaps just change the docs here to link to them and say "same options as over there", so when the createHash docs get fixed this will be fixed, too.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Oct 15, 2019

Author Member

Personally, I don't think it's too confusing but I can split it off into a separate issue if you like.

doc/api/crypto.md Outdated Show resolved Hide resolved
@Trott Trott force-pushed the bnoordhuis:fix29903 branch from 69f0e10 to 939ad26 Oct 13, 2019
@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Oct 13, 2019

Could this suffer from problems similar to #28245 depending on OpenSSL internals, since we don't always set state[kFinalized]?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Oct 14, 2019

Could this suffer from problems similar to #28245 depending on OpenSSL internals, since we don't always set state[kFinalized]?

Interesting question. I don't believe it actually hurts to copy a finalized hash object, it's just pointless (and that's why I added the guard.)

@nodejs-github-bot

This comment was marked as outdated.

bnoordhuis added 2 commits Oct 9, 2019
Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903
@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix29903 branch from 939ad26 to 0291c30 Oct 15, 2019
@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Oct 15, 2019

Example added to the documentation.

Use hash.copy() first if you wish to continue to add data to the hash after hash.digest().

I didn't use that because it can be construed as meaning that copy() after digest() should work when it doesn't.


Food for thought: we could also make digest() omnipotent by cloning the hash object internally. I.e., this would work:

const hash = crypto.createHash('sha256');
const one = hash.update('one').digest();
const two = hash.update('two').digest();

The downside is that it imposes a (possibly small) performance penalty on single-use hash objects.

@overlookmotel

This comment has been minimized.

Copy link

overlookmotel commented Oct 15, 2019

@bnoordhuis Only problem with that is that some people feel that the hash object should be reusable in that its internal state is reset after a call to .digest() (#25857). Personally, that doesn't make sense to me, but it could cause confusion it seems.

It's not so hard to call .copy() explicitly I think, and some people will blanche at even a small performance cost.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Oct 15, 2019

I don't think it is a good idea to implicitely reset the internal state of the object in .digest(), it will only lead to bugs and problems.

.copy() behaves differently than what you are describing, it does not reset the state, instead, it allows to obtain a second Hash object with the same internal state.

@nodejs-github-bot

This comment has been minimized.

@overlookmotel

This comment has been minimized.

Copy link

overlookmotel commented Oct 16, 2019

@tniessen Ah sorry I wasn't clear. I understand this is not what .copy() does. What I was trying to say is that there are seemingly two different behaviors that people might expect if you are allowed to call .digest() more than once on a hash.

With this code:

const hash = crypto.createHash('md5');
hash.update('abc');
hash.digest('hex');

hash.update('def');
const res = hash.digest('hex');

res could be expected to be either:

  1. Hash of "abcdef" (rolling hash)
  2. Hash of "def" (1st call to .digest() reset hash)

My expectation would be (1), but it seems from #25857 that some people would expect (2). Personally I think there's little merit to the arguments made by the OP in that issue. But since there are seemingly different opinions, it could lead to confusion and bugs as you say.

So I was trying to say: probably it's safer to keep behavior of .digest() as it is, to avoid any ambiguity.

Hope that make more sense.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Oct 16, 2019

@Trott Trott added the semver-minor label Oct 16, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2019

Landed in 9f203f9

@Trott Trott closed this Oct 16, 2019
Trott added a commit that referenced this pull request Oct 16, 2019
Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903

PR-URL: #29910
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@overlookmotel

This comment has been minimized.

Copy link

overlookmotel commented Oct 16, 2019

Fantastic. Thanks very much @bnoordhuis for implementing this and everyone else for reviewing.

Can I ask: Is this likely to get into a Node 12.x release before it goes LTS?

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2019

Can I ask: Is this likely to get into a Node 12.x release before it goes LTS?

Probably not before it goes LTS but probably the next release after that one. /ping @nodejs/releasers in case I'm wrong about that.

@overlookmotel

This comment has been minimized.

Copy link

overlookmotel commented Oct 16, 2019

Ah OK great. My mistake - I was confusing the Active and Maintenance phases in terms of when new features can be added, so was wrongly thinking this might not land in an LTS release line until 14.x.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Oct 16, 2019

@overlookmotel Thanks for the clarification, I absolutely agree. Resetting the state in .digest() (or silently copying the state) will only lead to confusion.

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

vsemozhetbyt commented Oct 17, 2019

It seems the YAML processing is broken for this doc section. IIRC, we usually use this structure with PR URL field only in history entries.

addedin

Trott added a commit to Trott/io.js that referenced this pull request Oct 18, 2019
This fixes YAML that gets incorrectly processed by our tooling.

Refs: nodejs#29910 (comment)
@Trott Trott referenced this pull request Oct 18, 2019
3 of 3 tasks complete
Trott added a commit to Trott/io.js that referenced this pull request Oct 18, 2019
This fixes YAML that gets incorrectly processed by our tooling.

Refs: nodejs#29910 (comment)

PR-URL: nodejs#30016
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903

PR-URL: #29910
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
This fixes YAML that gets incorrectly processed by our tooling.

Refs: #29910 (comment)

PR-URL: #30016
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Make it possible to clone the internal state of a Hash object
into a new Hash object, i.e., to fork the state of the object.

Fixes: #29903

PR-URL: #29910
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
This fixes YAML that gets incorrectly processed by our tooling.

Refs: #29910 (comment)

PR-URL: #30016
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos added a commit that referenced this pull request Nov 5, 2019
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
@targos targos referenced this pull request Nov 5, 2019
targos added a commit that referenced this pull request Nov 5, 2019
Notable changes:

* cli:
  * Added a new flag (`--trace-uncaught`) that makes Node.js print the
    stack trace at the time of throwing uncaught exceptions, rather than
    at the creation of the `Error` object, if there is any. This is
    disabled by default because it affects GC behavior.
    #30025
* crypto
  * Added `Hash.prototype.copy()` method. It returns a new `Hash` object
    with its internal state cloned from the original one.
    #29910
* dgram
  * Added source-specific multicast support. This adds methods to
    Datagram sockets to support RFC 4607
    (https://tools.ietf.org/html/rfc4607) for IPv4 and IPv6.
    #15735
* fs
  * Added a `bufferSize` option to `fs.opendir()`. It allows to control
    the number of entries that are buffered internally when reading from
    the directory. #30114
* meta
  * Added Chengzhong Wu (https://github.com/legendecas) to
    collaborators. #30115

PR-URL: #30262
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.