Repository navigation
gh-91349: Replace zlib with zlib-ng in Windows build - #131438
Conversation
| { | ||
| "referenceCategory": "SECURITY", | ||
| "referenceLocator": "cpe:2.3:a:zlib:zlib:1.3.1:*:*:*:*:*:*:*", | ||
| "referenceLocator": "cpe:2.3:a:zlib-ng:zlib-ng:2.2.4:*:*:*:*:*:*:*", |
There was a problem hiding this comment.
I'll admit I just guessed this, in order to unblock my testing (couldn't build at all with an invalid SBOM). If it's not right, let me know
There was a problem hiding this comment.
This is likely the CPE that would get used, although we can't be for certain until a CVE exists with it... the only one I could find with some searching is cpe:...:zlib-ng:minizip-ng which is for a different component but at least the organization is a match.
| return -1; | ||
| } | ||
| #ifdef ZLIBNG_VERSION | ||
| if (PyModule_Add(mod, "ZLIBNG_VERSION", |
There was a problem hiding this comment.
Figured we'd want some way to detect this other than looking at the text in ZLIB_VERSION
There was a problem hiding this comment.
makes sense. annoying to have optional attributes that one would use hasattr or getattr on instead of blindly accessing, but realistically nobody should care as this is more internal informational so this is fine.
There was a problem hiding this comment.
Yeah, the main place it'll be used is in pythoninfo, which already handles absent attributes.
We have more offensive optional attributes in our extension modules 😆
|
I've reviewed the three header files added to |
|
🤖 New build scheduled with the buildbot fleet by @zooba for commit 08eecb1 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F131438%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
The two failing buildbots appear unrelated, and everything else passed. All I changed in the last commit was docs, so I'm not going to rerun the buildbots unless someone thinks it's necessary or worthwhile. |
| { | ||
| "referenceCategory": "SECURITY", | ||
| "referenceLocator": "cpe:2.3:a:zlib:zlib:1.3.1:*:*:*:*:*:*:*", | ||
| "referenceLocator": "cpe:2.3:a:zlib-ng:zlib-ng:2.2.4:*:*:*:*:*:*:*", |
There was a problem hiding this comment.
This is likely the CPE that would get used, although we can't be for certain until a CVE exists with it... the only one I could find with some searching is cpe:...:zlib-ng:minizip-ng which is for a different component but at least the organization is a match.
|
I'm kicking off a pyperformance benchmarking run with this -- we have a non-zero usage of zlib in that suite. |
|
The results for this on pyperformance are basically within the noise / no change. Not all that surprising, given that there are no benchmarks specifically aimed at zlib. https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20250319-3.14.0a6+-548daa7 |
| return -1; | ||
| } | ||
| #ifdef ZLIBNG_VERSION | ||
| if (PyModule_Add(mod, "ZLIBNG_VERSION", |
There was a problem hiding this comment.
makes sense. annoying to have optional attributes that one would use hasattr or getattr on instead of blindly accessing, but realistically nobody should care as this is more internal informational so this is fine.
|
This has broken the tailcall builds on Windows: https://github.com/python/cpython/actions/runs/13967899073/job/39102464260 |
|
Probably needs some fixed preprocessor checks (hopefully only in what's in the Though I notice that build warns |
|
I think, this is because here Then, in Sorry, I cannot get the permalinks to work here the usual way I am doing it :( |
FWIW, when building with the bundled clang-cl of VS I do not have these warnings, but get the same error. |
|
Most likely zlib-ng would generate different versions of the header files I checked into the This should get a new issue (because clang-cl isn't a supported configuration, so we don't have to revert this change until it's working). Tag me on it and we can continue there. |
| <PrecompiledHeader>NotUsing</PrecompiledHeader> | ||
| <AdditionalIncludeDirectories>$(zlibNgDir);$(PySourceDir)PC;$(GeneratedZlibNgDir);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> | ||
| <PreprocessorDefinitions>%(PreprocessorDefinitions);ZLIB_COMPAT;WITH_GZFILEOP;NO_FSEEKO;HAVE_BUILTIN_ASSUME_ALIGNED;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="$(Platform) == 'Win32' or $(Platform) == 'x64'">%(PreprocessorDefinitions);X86_FEATURES;X86_HAVE_XSAVE_INTRIN;X86_SSE2;X86_SSSE3;X86_SSE42;X86_PCLMULQDQ_CRC;X86_AVX2;X86_AVX512;X86_AVX512VNNI;X86_VPCLMULQDQ_CRC</PreprocessorDefinitions> |
There was a problem hiding this comment.
Does this mean that for all executables X86_AVX512 code will be emitted and used?
See e.g. adler32_avx512.c, where the whole file is guarded with X86_AVX512 and there is code like __m512i in it.
There was a problem hiding this comment.
I believe it's detected and called dynamically based on availability, but it's all compiled at compile time.
| <PreprocessorDefinitions>%(PreprocessorDefinitions);ZLIB_COMPAT;WITH_GZFILEOP;NO_FSEEKO;HAVE_BUILTIN_ASSUME_ALIGNED;_CRT_SECURE_NO_DEPRECATE;_CRT_NONSTDC_NO_DEPRECATE;</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="$(Platform) == 'Win32' or $(Platform) == 'x64'">%(PreprocessorDefinitions);X86_FEATURES;X86_HAVE_XSAVE_INTRIN;X86_SSE2;X86_SSSE3;X86_SSE42;X86_PCLMULQDQ_CRC;X86_AVX2;X86_AVX512;X86_AVX512VNNI;X86_VPCLMULQDQ_CRC</PreprocessorDefinitions> | ||
| <PreprocessorDefinitions Condition="$(Configuration) == 'Debug'">%(PreprocessorDefinitions);ZLIB_DEBUG</PreprocessorDefinitions> | ||
| <EnableEnhancedInstructionSet Condition="$(Platform) == 'Win32' or $(Platform) == 'x64'">AdvancedVectorExtensions2</EnableEnhancedInstructionSet> |
There was a problem hiding this comment.
Unconditionally letting the compiler generate code up to AVX2 for all *.c files here might result in the binary not running on all CPUs?
The big changes here are: - Switching to zlib-ng on Windows (python/cpython#131438) - Using hmac for hashing functions (python/cpython#130157) --------- Co-authored-by: Geoffrey Thomas <geofft@ldpreload.com>
|
Hi @zooba, a heads-up on zlib-ng 2.2.4, in case it's useful to anyone: PGO builds made with Visual Studio 17.11 (MSVC 14.41) decompress deflate data wrongly on CPUs with AVX-512 (test_zlib: "incorrect data check", test_gzip: "CRC check failed"). It's the known zlib-ng bug zlib-ng/zlib-ng#1883 (MSVC before 17.13 miscompiles the AVX-512 GET_CHUNK_MAG()), worked around in zlib-ng 2.2.5 by zlib-ng/zlib-ng#1884. On a Ryzen 9 7950X, adding just that pragma to 2.2.4 fixes the build, and removing it brings the wrong output back. It doesn't affect CI or the python.org builds, which use newer compilers, but it does affect building from source with an older Visual Studio, which is how I stumbled upon it. Is there any chance of updating the Windows build to zlib-ng 2.2.5? main, 3.15 and 3.14 still bundle 2.2.4. |
|
@maikschulze Yes, that sounds like an update we'd make. Can you file a new issue? |
|
Hi, thanks for the quick response! I will file an issue and along with a pull request tomorrow. |
|
I think we keep the zlib-ng sources in a separate repo, so just file the issue without a PR as we'll need a core developer to update that. We don't accept PRs, as it's too hard to validate that the sources are unmodified (but if you've got a link to the right download page, that will be appreciated) |
Uh oh!
There was an error while loading. Please reload this page.