Sitelet https://github.com/nodejs/node/issues/50138
Skip to content

zlib upgrade broke known vectors #50138

Closed as not planned
Closed as not planned
zlib upgrade broke known vectors#50138
Labels
dependenciesPRs that add, update, or configure Node.js dependencies.invalidIssues and PRs that are invalid.zlibIssues and PRs related to the zlib module and its compression dependencies.

Description

@panva
panva
opened on Oct 11, 2023

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.

cc @alfonsograziano @lpinca @aduh95

Activity

  1. panva
    added
    zlibIssues and PRs related to the zlib module and its compression dependencies.
    dependenciesPRs that add, update, or configure Node.js dependencies.
    on Oct 11, 2023

  2. lpinca commented on Oct 11, 2023

    @lpinca
    lpinca
    on Oct 11, 2023
    Member

    cc: @Adenilson

  3. targos commented on Oct 11, 2023

    @targos
    targos
    on Oct 11, 2023
    Member

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

  4. panva commented on Oct 11, 2023

    @panva
    panva
    on Oct 11, 2023
    MemberAuthor

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

  5. lpinca commented on Oct 11, 2023

    @lpinca
    lpinca
    on Oct 11, 2023
    Member

    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.

  6. panva commented on Oct 11, 2023

    @panva
    panva
    on Oct 11, 2023
    MemberAuthor

    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.

  7. targos commented on Oct 11, 2023

    @targos
    targos
    on Oct 11, 2023
    Member

    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

  8. lpinca commented on Oct 11, 2023

    @lpinca
    lpinca
    on Oct 11, 2023
    Member

    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.

  9. panva commented on Oct 11, 2023

    @panva
    panva
    on Oct 11, 2023
    MemberAuthor

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

  10. bnoordhuis commented on Oct 11, 2023

    @bnoordhuis
    bnoordhuis
    on Oct 11, 2023
    Member

    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.

  11. bnoordhuis
    closed this as not plannedon Oct 11, 2023
  12. bnoordhuis
    added
    invalidIssues and PRs that are invalid.
    on Oct 11, 2023

  13. panva commented on Oct 11, 2023

    @panva
    panva
    on Oct 11, 2023
    MemberAuthor

    Fair enough. Thank you @bnoordhuis

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

  14. Adenilson commented on Oct 11, 2023

    @Adenilson
    Adenilson
    on Oct 11, 2023

    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.
    :-)

  15. Adenilson commented on Oct 11, 2023

    @Adenilson
    Adenilson
    on Oct 11, 2023

    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

  16. Adenilson commented on Oct 11, 2023

    @Adenilson
    Adenilson
    on Oct 11, 2023

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

  17. novemberborn
    added 4 commits that reference this issue on Oct 22, 2023

    Change snapshot tests to compare decompressed bitstreams

    92512c8

    Change snapshot tests to compare decompressed bitstreams

    a7c1142

    Change snapshot tests to compare decompressed bitstreams

    922bc91

    Drop Node.js 16, upgrade minimal 18 and 20, test 21

    03a6723
  18. wraithgar
    added a commit that references this issue on Dec 1, 2023

    chore: fix tests for zlib differences between node versions (#340)

    a06a459
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    dependenciesPRs that add, update, or configure Node.js dependencies.invalidIssues and PRs that are invalid.zlibIssues and PRs related to the zlib module and its compression dependencies.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions