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

zlib different on OSX and Linux in newest releases #12244

Closed
flippmoke opened this issue Apr 5, 2017 · 14 comments
Closed

zlib different on OSX and Linux in newest releases #12244

flippmoke opened this issue Apr 5, 2017 · 14 comments
Assignees
Labels
macos Issues and PRs related to the macOS platform / OSX. zlib Issues and PRs related to the zlib subsystem.

Comments

@flippmoke
Copy link

It appears that after #10980, there are some issues with zlib specifically producing different results on OSX during gzip compression. This can be reproduced by the small test below, where there is a single byte difference in the zlib results. This same test does not produce different then expected results on Linux.

var zlib = require('zlib');

var uncompressed = Buffer.from("1ad40178020a05776f726c642880201a0441524541220420b8f3371a044649505322040a0255531a0449534f321a0449534f3322050a035553411a034c4154220919560e2db29dcf43401a034c4f4e220919105839b4c8a658c01a044e414d45220f0a0d556e69746564205374617465731a07504f5032303035220620b196fd8e011a06524547494f4e220220131a09535542524547494f4e220220151a02554e220320c806122f18032210098042ff011a008044ff430000ff430f08cf01121600000101020103020403050406050706080709080a091ad50178020a06776f726c64322880201a0441524541220420b8f3371a044649505322040a0255531a0449534f321a0449534f3322050a035553411a034c4154220919560e2db29dcf43401a034c4f4e220919105839b4c8a658c01a044e414d45220f0a0d556e69746564205374617465731a07504f5032303035220620b196fd8e011a06524547494f4e220220131a09535542524547494f4e220220151a02554e220320c806122f18032210098042ff011a008044ff430000ff430f08cf01121600000101020103020403050406050706080709080a09", "hex");

var compressed = Buffer.from("1f8b080000000000000393bac258c1c4c55a9e5f9493a2d1a020c5e218e4eaa8c4a2b0e3b3b9148b9b6740b0120b175368b0148b67b0bf1198345662e5620e0d769462f6710c51e2940ce3d3dd34f7bcb30390efef07e40b44586e39b12ce280148b9fa3afab123f176f685e66496a8a42704962496ab1147b807f80918181a9129bc2c6697ffb18a5d8825cdd3d815a991484a53883439de05c5129a6503f256685136c42fa12cc4a029c0d4eff19a5181a5cfe3b3330fc77e6e738cf2824c6c0c0c8c8c4c8ccc4c2cccac2c6cacec6c1cec9c1c5297515e42d36b0b78c8691bf005ccccc9baf010000", "hex");

zlib.gzip(uncompressed, {strategy: zlib.Z_DEFAULT_STRATEGY}, function(err, buffer) {
    if (buffer.toString('hex') === compressed.toString('hex')) {
        console.log("Results are the same");
    } else {
        console.log("Results are different");
    }
});
  • Version: v4.8.1
  • Platform: OS-X
  • Output: Results are the same

  • Version: v4.8.1
  • Platform: Linux
  • Output: Results are the same

  • Version: v4.8.2
  • Platform: OS-X
  • Output: Results are different

  • Version: v4.8.2
  • Platform: Linux
  • Output: Results are the same

I believe this also affects the v6 latest release, but have not tested it yet.

@vsemozhetbyt vsemozhetbyt added macos Issues and PRs related to the macOS platform / OSX. zlib Issues and PRs related to the zlib subsystem. labels Apr 5, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 5, 2017

cc/ @sam-github

@rmg
Copy link
Contributor

rmg commented Apr 5, 2017

This seems to only be compression, not decompression.

zlib.gunzip(compressed, function(err, buffer) {
    if (buffer.toString('hex') === uncompressed.toString('hex')) {
        console.log("Decompression results are the same");
    } else {
        console.log("Decompression results are different");
    }
});
  • Version: v4.8.2
  • Platform: macOS
  • Output: Decompression results are the same

Does zlib/gzip make any guarantees about always producing the same compressed output? It shouldn't be required as long as the compressed results can be decompressed by a compatible implementation to produce the same input.

@rmg rmg mentioned this issue Apr 5, 2017
2 tasks
@rmg
Copy link
Contributor

rmg commented Apr 5, 2017

I think this may be an upstream change.

In the new version of zlib, if __APPLE__ is defined (which it seems to be on our macOS builds) then OS_CODE is 19 instead of 3. Since this value is included in the gzip header, it would slightly change the resulting compression output.

Commented: https://github.com/nodejs/node/pull/10980/files#r110047791

That's my current theory, at least.

@MylesBorins MylesBorins self-assigned this Apr 6, 2017
@MylesBorins
Copy link
Contributor

/cc @nodejs/lts

@shigeki
Copy link
Contributor

shigeki commented Apr 6, 2017

The change of version made of header was made in madler/zlib@ce12c5c in order to follow the spec of https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT.
I don't think this is an issue because a compressed binary is platform dependent as it was already different between Linux and Windows and it does not affect compress/decompress functionaries.

@rmg
Copy link
Contributor

rmg commented Apr 6, 2017

@MylesBorins I'm not sure it would be worth the effort to float a patch for this on an lts branch.

@flippmoke is this causing an actual problem? Or is it just a change in behaviour that wasn't expected?

@flippmoke
Copy link
Author

@rmg It is not an actual issue with out code base, we were just using node.js zlib to compare against our C++ implementation inside a node application (https://github.com/mapnik/node-mapnik) for regular testing. We have been building off the system zlib 1.2.8 in OSX so suddenly the results were not the same. I would have just figured it was a slight difference due to a new zlib version but the fact that linux results were still the same made me wonder if it was a bug.

We could change our testing techniques and perhaps and ignore the header of the gzip compressed binary when we compare the results.

@sam-github
Copy link
Contributor

Or perhaps don't compare the zipped data, but unzip it and compare that to an expected value? That would be robust against implementation.

@flippmoke
Copy link
Author

@sam-github we could definitely do that, but part of the testing was to compare that all of our options that could be passed in for different configurations of zlib were all going in and creating reasonable results as well, so it really is nice to compare directly to another compressed result. I think if I can properly determine where the header of the zlib compression ends that I should be able to only test against the results after that point and achieve the same results.

rmg added a commit to strongloop/microgateway that referenced this issue Apr 13, 2017
The problem is that gzip headers include a platform identifier.
Previously, zlib was incorrectly treating macOS as just a "generic
UNIX", but now it is actually using the correct header. The updated zlib
made its way in to recent versions of node and started uncovering a new
source of platform-specific assumptions that people didn't know they
were making.

To fix this, instead of hard-coding the expected gzip compression
result, we can just generate them on the testing platform.

This is an instance of nodejs/node#12244.
rmg added a commit to strongloop/microgateway that referenced this issue Apr 13, 2017
The problem is that gzip headers include a platform identifier.
Previously, zlib was incorrectly treating macOS as just a "generic
UNIX", but now it is actually using the correct header. The updated zlib
made its way in to recent versions of node and started uncovering a new
source of platform-specific assumptions that people didn't know they
were making.

To fix this, instead of hard-coding the expected gzip compression
result, we can just generate them on the testing platform.

This is an instance of nodejs/node#12244.

Signed-off-by: Ryan Graham <r.m.graham@gmail.com>
@rmg
Copy link
Contributor

rmg commented Apr 13, 2017

I just saw another instance of this causing an inexplicable test failure and the only reason I knew the cause was because I had dug in to this issue, and even that was only possible because @flippmoke had already done the hard work of identifying that it was a gzip header change.

Spelling it out, I now realize how unreasonable it would be to expect the average node user to figure out what happened.

@nodejs/lts I take back my previous stance. This part of the zlib upgrade should be reverted as part of an LTS patch release.

@MylesBorins
Copy link
Contributor

@rmg could you please send a PR with the revert to both 4.x and 6.x?

@sam-github
Copy link
Contributor

@MylesBorins I am looking at this now. I'll PR a test and revert of the header change.

springmeyer pushed a commit to mapnik/node-mapnik that referenced this issue Apr 19, 2017
pulkitsinghal pushed a commit to pulkitsinghal/microgateway that referenced this issue Jun 18, 2017
The problem is that gzip headers include a platform identifier.
Previously, zlib was incorrectly treating macOS as just a "generic
UNIX", but now it is actually using the correct header. The updated zlib
made its way in to recent versions of node and started uncovering a new
source of platform-specific assumptions that people didn't know they
were making.

To fix this, instead of hard-coding the expected gzip compression
result, we can just generate them on the testing platform.

This is an instance of nodejs/node#12244.

Signed-off-by: Ryan Graham <r.m.graham@gmail.com>
@Trott
Copy link
Member

Trott commented Aug 2, 2017

Given that #12404 was closed without merging, should this remain open? I strongly suspect 4.x is going to go EOL before this is fixed, but someone is welcome to prove me wrong by fixing it. :-D

@sam-github
Copy link
Contributor

should close, the PR was rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos Issues and PRs related to the macOS platform / OSX. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants