crypto hash with multiple update() return inconsistent hex on node 0.11.x

[Bartvds]

edit-after-thefact: note: the reported the multiple update() was not the real issue, but @indutny spotted the actual problem and fixed it.

original text:

---

When I use node 0.11.12 to hash with multiple update()'s I occasionally get bad sha1 hashes. On node 0.10.x it works fine.

This can get bad results:

// data is a Buffer
crypto.createHash('sha1').update('blob ' + data.length + '\0').update(data).digest('hex');

If I concat the buffers myself it works stable:

crypto.createHash('sha1').update(Buffer.concat([new Buffer('blob ' + data.length + '\0'), data])).digest('hex');

---

Demo code:
https://github.com/Bartvds/node-sha1-demo/blob/master/index.js

Note the beefy base64 string: it is a file I got as a git-blob from the Github API (eg: the hash is known). This data is extracted from a existing test suite that worked fine for a while now in node 0.10 (from an app with some npm users so it is battle tested code too, as basic as it is).

Also note the alternate methods and how they work as expected.

---

Same code failing on travis:
https://travis-ci.org/Bartvds/node-sha1-demo/builds/21640883

Note how one build passes. In the failing log you see the multi update() gets a bad result. It is not consistent in the error: in another run it might be another hex string.

In a more complex setup the same code might return multiple different hashes for same bytes.

The weird thing is it doesn't happen for all input: a simple test with a short string works fine. If I check using the sha1 module from npm (with some console.log's) then that will give me a expected result.

[harlentan]

Hi @Bartvds,
I run your code from https://github.com/Bartvds/node-sha1-demo/blob/master/index.js , it just works as expect in node-v0.10.x, and failed in node -v0.11.x.

I think there is something wrong with the Buffer or somewhere else.
@indutny, Could you please assign this to me?

[Bartvds]

Thanks. What you see matches my result.

It was a weird glitch to spot initially as even in the erroring case it sometimes generates a correct hash in the first try and only after a few rounds starts to output bad ones (this is why this demo has the for-loop and setTimeout spam). Then it generates a whole bunch of the same bad hex.

Looks like a fun one with some lingering state somewhere in the crypto code; sadly this goes way beyond my skill level but I'm confident you'll manage :)

[indutny]

@harlentan sorry, github doesn't allow this.

The problem here is that the data is incorrectly read from base64 encoded string. Initially, we allocate a bit more space than needed for base64-decode output, and when shrinking it via buffer.length = newsize; somehow C++ still thinks that the length hasn't changed.

@harlentan do you want to work on it?

[indutny]

Simpler test case:

var crypto = require('crypto');

var b1 = new Buffer('YW55=======', 'base64');
var b2 = new Buffer('YW55', 'base64');

console.log(crypto.createHash('sha1').update(b1).digest('hex'));
console.log(crypto.createHash('sha1').update(b2).digest('hex'));

[indutny]

According to bisect, one of this commits seems to be causing the problem:

4b40358
f489649
56869d9
7373c4d
bf8dc07
fb40da8
456942a
f5e13ae
24ba9fd
3a2f273
157d2bc

[indutny]

Should be fixed by #7370, sorry @harlentan ;)

[Bartvds]

@indutny I now realise that report incorrectly assumed the problem was in the hashing (in the chaos of debugging & isolating). The Buffer.concat() that made it work threw me off I guess.

But niceone! Hope it lands soon.