GeezerVison

CI: https://ci.nodejs.org/job/node-test-pull-request/28223/

CI: https://ci.nodejs.org/job/node-test-pull-request/28224/

Do we know why they aren't included upstream? If it's because of compatibility or something like that we'd need to be careful.

madler/zlib#346 seems related

CI: https://ci.nodejs.org/job/node-test-pull-request/28229/

Not sure what is up with the Windows failures. I'm not even able to successfully install the 2017 or 2019 build tools or community edition to duplicate the problem myself and the CI logs show no output.

/cc @nodejs/build ?

https://ci.nodejs.org/job/node-compile-windows/31073/nodes=win-vs2019/console

worrisome or not? I don't know.

21:14:10 > git push [email protected]:binary_tmp.git :jenkins-node-test-commit-windows-fanned-6e9f471ac49c9f102c51858e3d03313e8b8e10f6-bin-win-vs2019 
21:14:11 Warning: Permanently added '169.60.150.88' (ECDSA) to the list of known hosts.
21:14:12 error: unable to delete 'jenkins-node-test-commit-windows-fanned-6e9f471ac49c9f102c51858e3d03313e8b8e10f6-bin-win-vs2019': remote ref does not exist
21:14:12 error: failed to push some refs to '[email protected]:binary_tmp.git'

build continued, failed at

21:14:16 creating config.gypi
21:14:16 creating config.status
21:14:16 creating config.mk
21:14:16 running: 
21:14:16     python tools/gyp_node.py --no-parallel -Dconfiguring_node=1 -f msvs -G msvs_version=auto
21:14:16 Error running GYP
21:14:16 gyp: name 'llvm_version' is not defined while evaluating condition 'OS!="win" or llvm_version!="0.0"' in deps\zlib\zlib.gyp while loading dependencies of node.gyp while trying to load node.gyp
21:14:16 
21:14:16 > if errorlevel 1 goto create-msvs-files-failed 
21:14:16 
21:14:16 > echo Failed to create vc project files. 
21:14:16 Failed to create vc project files.

possibly related to #30954 ?

Is it possible chromium only builds with llvm on Windows, so the .gyp file you pulled in doesn't work for node?

Is it possible chromium only builds with llvm on Windows, so the .gyp file you pulled in doesn't work for node?

Chromium doesn't use gyp any more -- the gyp file is all ours. configure doesn't currently set llvm_version for Windows at all:

All current references in our gyp files to llvm_version are inside other condition blocks that have already excluded Windows.

CI: https://ci.nodejs.org/job/node-test-pull-request/28240/

Do we expose the bindings for zlib? What guarantees do we have for ABI compatibility between V8 versions? What about security releases? Not blocking, just want to ensure that coupling these dependencies won't put us in a weird place.

Do we expose the bindings for zlib?

Yes, we do. From https://nodejs.org/dist/latest-v13.x/docs/api/addons.html#addons_c_addons:

• Node.js includes other statically linked libraries including OpenSSL. These other libraries are located in the deps/ directory in the Node.js source tree. Only the libuv, OpenSSL, V8 and zlib symbols are purposefully re-exported by Node.js and may be used to various extents by Addons.

Since we expose the bindings I would love to see some historical data about how much V8 has updated zlib and what ABI assurances we have. It might even make sense to duplicate the zlib and periodically update it ourselves, but that might be overkill

CI: https://ci.nodejs.org/job/node-test-pull-request/28242/

Everything is passing now. AFAIK there should be no ABI issues. Do we have something in CI to check this sort of thing?

Yeah, this looks good API/ABI-wise

I would love to see some historical data about how much V8 has updated zlib

This seems relevant: https://cs.chromium.org/chromium/src/third_party/zlib/patches/README?g=0

If this information is updated, there are only three patches applied on top of upstream zlib, one of them being purely build infrastructure changes. The relevant changes would be in https://cs.chromium.org/chromium/src/third_party/zlib/patches/0001-simd.patch?g=0, which are (probably) the same changes available in the zlib provided by Intel (https://software.intel.com/en-us/articles/how-to-use-zlib-with-intel-ipp-optimization). The last patch is one line change which shouldn't affect API/ABI.

Edit: looking at Chromium git history, the patches listed above are outdated. There's also ARM optimizations, among other changes.

FWIW, other companies also have their zlib forks with optimizations which are not merged upstream:

Intel: https://github.com/jtkukunas/zlib
Cloudflare: https://github.com/cloudflare/zlib

CI: https://ci.nodejs.org/job/node-test-pull-request/28378/

CI: https://ci.nodejs.org/job/node-test-pull-request/28391/

For @nodejs/releasers this has broken builds from the source tarball and would need to land on, e.g. 12.x, with whatever fixes it.

@mscdex PTAL at #31800

I added a maintenance guide. It makes clear what parts of this are from upstream, and what are maintained by Node.js.

I also took a shot at updating to head of upstream, because then it might be worth trying to fix all the compiler warnings, but there were non-trivial changes and I couldn't get it to build after half an hour of fiddling with our zlib.gyp file. There isn't any particular feature I wanted from upstream, but was hoping it would either fix the compiler warnings, or accept patches to fix them.

gcc-9 warnings

I'm surprised it passes cleanly in a build through Chromium bots (we enable quite a few warning flags for clang, also got MSAN/ASAN active and fuzzers).

A quick look confirms that the impacted code is not part of the optimizations per-si, as it comes straight from zlib upstream:
https://github.com/madler/zlib/blob/master/infback.c#L479
https://github.com/nodejs/node/blob/master/deps/zlib/infback.c#L479

I think the fall through case is on purpose here, therefore a false positive.

Finally, I noticed that the hash imported seems to be missing the fix for an uninitialized jump (landed on 23th Jan):
https://cs.chromium.org/chromium/src/third_party/zlib/patches/0003-uninitializedjump.patch
https://github.com/nodejs/node/tree/master/deps/zlib/patches

If updating to Chromium's ToT (Top of Tree) is too hard, I would recommend to at least apply the fix on what you guys are shipping.

I'm working in some new optimizations and when I land this new work (will take a few months), I can provide a heads up, but then it would be the time to get in-sync with ToT, I'm afraid.

if you want to search for other optimisations this repo summarize it :

https://github.com/matbech/zlib

I'm unsure what is the best way to reach out to node.js developers, but in face of the newly reported CVE impacting zlib (https://nvd.nist.gov/vuln/detail/CVE-2018-25032), I just wanted to share that we have backported the fix to Chromium's zlib all the way back in 2018 (madler/zlib#605 (comment)).

From a security point of view, if you are running Chromium's zlib you should be fine.

I'm actively inspecting the patches featured in zlib 1.2.12 and porting them to Chromium.

The work updating Chromium's zlib to zlib 1.2.12 is tracked here:
https://bugs.chromium.org/p/chromium/issues/detail?id=1032721