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

Why crypto.createHash() is not writable until setEncoding() is called? #12269

Closed
iximiuz opened this issue Apr 7, 2017 · 8 comments
Closed
Labels
crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem.

Comments

@iximiuz
Copy link

iximiuz commented Apr 7, 2017

  • Version: v6.9.1
  • Platform: Darwin 15.2.0 Darwin Kernel Version 15.2.0: Fri Nov 13 19:56:56 PST 2015; root:xnu-3248.20.55~2/RELEASE_X86_64 x86_64

Please look at the code below:

var hasher = crypto.createHash('sha256'); 
// hasher.setEncoding('hex');
console.log(hasher.writable); 

So, if one use hasher with the legacy stream.pipe() implementation there will be an error, because the hash will be always calculated from the empty value.

It's easy to reproduce:

const crypto = require('crypto'); 
const request = require('request');  // this package uses oldschool streams

var hasher = crypto.createHash('sha256'); 
// Uncomment the line below to fix!
// hasher.setEncoding('hex');
console.log(hasher.writable); 
request('http://ya.ru').pipe(hasher).on('finish', function() {
    console.log('Hash is', hasher.read()); 
});
@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem. labels Apr 7, 2017
@bnoordhuis
Copy link
Member

cc @nodejs/streams - this doesn't come out of lib/crypto.js as far as I can tell.

@mcollina
Copy link
Member

mcollina commented Apr 7, 2017

@bnoordhuis I will have a look. I think it's LazyTransform.

@calvinmetcalf
Copy link
Contributor

it's coming out of lazyTransform, does removing the console log change anything ?

@mcollina
Copy link
Member

mcollina commented Apr 7, 2017

I think the problem is that https://github.com/nodejs/node/blob/master/lib/internal/streams/legacy.js#L11-L91 does not cause the lazyness of LazyTransform triggered.

@calvinmetcalf
Copy link
Contributor

@mcollina but in the example, the console.log will actually trigger LazyTransform

@mcollina
Copy link
Member

mcollina commented Apr 7, 2017

@calvinmetcalf why? the lazyness is on _writableState.

@calvinmetcalf
Copy link
Contributor

ignore me, I thought he was calling console.log on _writableState

@iximiuz
Copy link
Author

iximiuz commented Apr 7, 2017

I found out the root of the problem. Actually this code can reproduce it:

var hasher = crypto.createHash('sha256'); 
const _ = hasher._writableState;  // we don't even have to call .setEnconding() here
console.log(hasher.writable);

So, the main reason is the LazyTransform class and its delayed parent's constructor calls. LazyTransform calls the ctr of the Transform (which extends Duplex which extends Writable) only on the first access to _readableState, or _writableState, or _transformState. However, this.writable = true is set only in the Writable's constructor.

A fix could be another backward compatibility hack - setting this.writable = true in the LazyTransorm thin constructor. Another one could be triggering access to this._writableState in the legacy stream.Stream.pipe().

mcollina added a commit to mcollina/node that referenced this issue Apr 14, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs#12269
evanlucas pushed a commit that referenced this issue Apr 25, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this issue May 1, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this issue May 2, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue May 15, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: #12269
PR-URL: #12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Makes LazyTransform writable by Streams1 by assigning .writable = true
before the actual classes are loaded.

Fixes: nodejs/node#12269
PR-URL: nodejs/node#12380
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants