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] Bump to upstream 1.2.12 #68219

Closed
wants to merge 7 commits into from

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Apr 19, 2022

Also cherrypick madler/zlib@ec3df00 to restore pre-1.2.12 behavior:

The previous releases of zlib were not sensitive to incorrect CRC
inputs with bits set above the low 32. This commit restores that
behavior, so that applications with such bugs will continue to
operate as before.

Also bump zlib-intel to https://github.com/jtkukunas/zlib/tree/v1.2.12_jtk and cherrypick vanilla zlib crc32 fix (above) tozlib-intel.

Cherrypick crc32 fix to restore pre-1.2.12 behavior

madler/zlib@ec3df00

> The previous releases of zlib were not sensitive to incorrect CRC
> inputs with bits set above the low 32. This commit restores that
> behavior, so that applications with such bugs will continue to
> operate as before.
@ghost
Copy link

ghost commented Apr 19, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Also cherrypick madler/zlib@ec3df00 to restore pre-1.2.12 behavior:

The previous releases of zlib were not sensitive to incorrect CRC
inputs with bits set above the low 32. This commit restores that
behavior, so that applications with such bugs will continue to
operate as before.

Author: lambdageek
Assignees: lambdageek
Labels:

area-Infrastructure-libraries

Milestone: -

@lambdageek lambdageek requested a review from am11 April 19, 2022 15:45
@lambdageek
Copy link
Member Author

Do we have a way of keeping track of patches we apply to zlib?

Should we just wait for 1.2.12.1?

@jkotas
Copy link
Member

jkotas commented Apr 19, 2022

Do we have a way of keeping track of patches we apply to zlib?

Look for the .txt files under https://github.com/dotnet/runtime/tree/main/src/native/external

@am11
Copy link
Member

am11 commented Apr 20, 2022

Thanks! Yes the -version.txt files help during such upgrades.

The build errors are upstream issue: madler/zlib#633. We can workaround it (by explicitly adding prototypes in zlib.h) or wait until it is properly fixed in upstream.

Note that in libraries, we use this zlib on Windows arm{64} and zlib-intel on Windows x{86,64}. Could you please try to collect some numbers from dotnet/performance#2152 on Windows arm64 system?

ps - I think plan is to try to replace both implementations with a single copy of nextgen https://github.com/zlib-ng/zlib-ng (which apparently captures optimizations from other two variants and add SIMD sauce on top), validate the performance impact on supported platforms and then switch all platforms to that (currently on Unix libSystem.IO.Compression.Native is linked with system libz; in contrast, brotli is compiled with in-tree sources).

@lambdageek
Copy link
Member Author

lambdageek commented Apr 21, 2022

The build errors are upstream issue: madler/zlib#633. We can workaround it (by explicitly adding prototypes in zlib.h) or wait until it is properly fixed in upstream.

I think I'd prefer waiting until it's fixed upstream. 1.2.12 seems not fully ready, to be honest. I think a 1.2.12.1 release is probably imminent.

@jeffhandley jeffhandley added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 22, 2022
@lambdageek
Copy link
Member Author

Turned off warnings as errors for -Wstrict-prototypes for the zlib sources.

Bumped zlib-intel to v1.2.12_jtk and also cherrypicked the vanilla zlib crc32 fix to zlib-intel (it applied cleanly).

set_source_files_properties only has an effect in the "current
CMakeLists.txt" (ie not across an `add_subdirectory` call) which was an issue
for mono/mini/CMakeLists.txt
@lambdageek
Copy link
Member Author

32-bit Windows builds are unhappy because of madler/zlib#631 (comment)

@ghost
Copy link

ghost commented Apr 25, 2022

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Also cherrypick madler/zlib@ec3df00 to restore pre-1.2.12 behavior:

The previous releases of zlib were not sensitive to incorrect CRC
inputs with bits set above the low 32. This commit restores that
behavior, so that applications with such bugs will continue to
operate as before.

Also bump zlib-intel to https://github.com/jtkukunas/zlib/tree/v1.2.12_jtk and cherrypick vanilla zlib crc32 fix (above) tozlib-intel.

Author: lambdageek
Assignees: lambdageek
Labels:

NO-MERGE, area-System.IO.Compression

Milestone: -

@am11 am11 mentioned this pull request Apr 27, 2022
@jeffhandley jeffhandley marked this pull request as draft May 5, 2022 01:37
@cmoaciopm
Copy link

Latest dotnet runtime 6 is 6.0.5 currently. Can I expect to get zlib bumped to 1.2.12 in coming 6.0.6 release?

@ghost ghost closed this Jun 29, 2022
@ghost
Copy link

ghost commented Jun 29, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO.Compression NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants