zlib upgrade broke known vectors

[panva]

See nodejs/citgm#1011

node -p 'process.versions.zlib'
1.2.13.1-motley
node -v
v18.18.1
node -e "result = zlib.deflateRawSync('You can trust us to stick with you through thick and thin–to the bitter end. And you can trust us to keep any secret of yours–closer than you keep it yourself. But you cannot trust us to let you face trouble alone, and go off without a word. We are your friends, Frodo.'); console.log(result); console.log(result.byteLength)"
<Buffer 6d 8f c1 0d c2 30 0c 45 57 f9 03 54 dd 01 0e ac 80 38 a6 a9 43 a2 46 31 72 1c 55 bd b1 03 1b 32 09 6e 2a 0e 48 9c 1c f9 ff ff fc 73 e3 06 ef 0a 54 5a ... 120 more bytes>
170

./node -p 'process.versions.zlib'
1.2.13.1-motley
➜  node git:(0522ac086c) ./node -e "result = zlib.deflateRawSync('You can trust us to stick with you through thick and thin–to the bitter end. And you can trust us to keep any secret of yours–closer than you keep it yourself. But you cannot trust us to let you face trouble alone, and go off without a word. We are your friends, Frodo.'); console.log(result); console.log(result.byteLength)"
<Buffer 5d 8f c1 0d 42 31 0c 43 57 f1 00 e8 ef 00 07 56 40 1c 4b 9b d2 8a 2a 41 a9 2b c4 8d 1d d8 90 49 d0 2f e2 00 b7 28 b6 5f 9c a3 0d c4 a0 a0 8f 4e 8c 0e ... 118 more bytes>
168

I suggest to revert the following commits and add unit tests for more known vectors to prevent breaking updates in the future.

• 0522ac0 from deps: update zlib to 1.2.13.1-motley-fef5869 #50085
• 660f902 from deps: update V8 to 11.8 #49639

cc @alfonsograziano @lpinca @aduh95

[lpinca]

cc: @Adenilson

[targos]

Why do you think 660f902 is involved in the issue?

[panva]

I thought maybe because it's a followup to 0522ac0 that resolves some compile warnings and wouldn't be needed if 0522ac0 was reverted?

[lpinca]

See https://chromium.googlesource.com/chromium/src/third_party/zlib.git/+/fef58692c1d7bec94c4ed3d030a45a1832a9615d. They also previously reverted it in https://chromium.googlesource.com/chromium/src/third_party/zlib.git/+/1938d49d05cb93de60aaf6b6d0e572a9aa764c72.

[panva]

It was/is my limited understanding that the zlib output is deterministic.

Whilst I can confirm the different versions can inflate/deflate each other this change in output is unexpected, to me at least.

I can mark the jose test as not reproducible to have it not check equal outputs but the cookbook of these vectors does not mention the zlib deflate output to be variable.

[targos]

I thought maybe because it's a followup to 0522ac0 that resolves some compile warnings and wouldn't be needed if 0522ac0 was reverted?

No, it's a followup to another, much less recent, zlib upgrade: 5fae8bc

[lpinca]

Whilst I can confirm the different versions can inflate/deflate each other this change in output is unexpected, to me at least.

I also tested this. Let's wait a little to see if we can get some feedback from @Adenilson.

[panva]

Sounds good, i'll also wait on marking the test as non-deterministic in the jose test suite then.

[bnoordhuis]

It was/is my limited understanding that the zlib output is deterministic.

It's not. The only requirement is that it properly round-trips. I'll go ahead and close this.

[panva]

Fair enough. Thank you @bnoordhuis

I'm left yearning more details though if you would find the time.

[Adenilson]

It was/is my limited understanding that the zlib output is deterministic.

It's not. The only requirement is that it properly round-trips. I'll go ahead and close this.

That is correct (i.e. the requirement is that it round-trips and its compatible with other implementations).

I will repost here a comment related to the subject that I made during the review of the first landed patch:
"
Changes in the algorithmic implementation in zlib may change the compressed bitstream, but the decompressed content is still the same (otherwise the data would be corrupted).

Unfortunately, there are tests that compare compressed content in different ways.

It seems to be a common behavior by client code to assume some sort of 'binary stability' when it comes to gzip compressed streams.

There are no such guarantees even between releases of zlib and I've spent quite some time suggesting that people should compare decompressed content while writing tests.
"

Basically if there are changes in the way that we perform matches (i.e. longest_match) or in the way we keep track of those in a hash table (i.e. by changing the hashing function), the compressed output will be different but still a valid GZIP stream that must be compatible with other implementations.

As an experiment, try to compress a given file using canonical zlib (it uses a Rabin-Karp hash) and compare what other forks (e.g. Chromium zlib now uses ANZAC++, before we were using CRC-32) will produce and it should be different.

But they must be compatible with each other (e.g. compress with one library, decompress with another and vice-versa) and the decompressed result must match with the original input.

I hope that helps to clarify the subject and feel free to contact me if you run in any issues.
:-)

[Adenilson]

For context, the test that triggered the revert of the original patch turned out to be flakey:
https://bugs.chromium.org/p/chromium/issues/detail?id=1487363#c20

[Adenilson]

And this were the compression performance gains thanks to the new hash on Xeon 4th gen (average +42% faster compression):