Harden the cmake Address Sanitizer detection code. by pmqs · Pull Request #960 · zlib-ng/minizip-ng

pmqs · 2026-02-10T16:57:54Z

Currently the sanitizer detection code will silently ignore error cases -- like when given an unknown option or when it is asked for a valid sanitizer that isn't available for the platform. This one has stung me a few times.

This change makes it a fatal error to

ask for an address sanitizer option that doesn't exist
asks for a valid option that the current platform doesn't support.

One thng that dropped out with this change is the use of the Memory Sanitizer in build.yml, Turns out that it is not available on Ubuntu, so has been adding nothing to our test coverage. I think that may be a limitation of virtualisation. I've just disabled it for now.

This is what build.yml had when (not) detecting the memory Sanitizer:

-- Checking for Address Sanitizer Option
-- Performing Test HAVE_SANITIZER_memory
-- Performing Test HAVE_SANITIZER_memory - Failed
-- Memory sanitizer is not supported

nmoinvaz · 2026-02-11T20:53:21Z

The sanitizer CMake is too borrowed from zlib-ng, so I'm uncomfortable changing it.

Coeur · 2026-02-14T06:01:40Z

+    endif()
+    if(NOT MZ_STATUS)
+        set(MZ_SANITIZER_ENABLED OFF)
+        message(FATAL_ERROR "Cannot enable the Address Sanitizer '${MZ_SANITIZER}' option")


Let's just log "Sanitizer" without "Address", as it can be any of them.

Coeur · 2026-02-14T06:01:55Z

+    endif()
+elseif(MZ_SANITIZER_LOWER STREQUAL "none")
+    set(MZ_SANITIZER_ENABLED OFF)
+    message(STATUS " Address Sanitizer not enabled")


Let's just log "Sanitizer" without "Address", as it can be any of them.

Coeur · 2026-02-14T06:02:55Z

 add_feature_info(MZ_BUILD_UNIT_TESTS MZ_BUILD_UNIT_TESTS "Builds minizip unit test project")
 add_feature_info(MZ_BUILD_FUZZ_TESTS MZ_BUILD_FUZZ_TESTS "Builds minizip fuzzer executables")
 add_feature_info(MZ_CODE_COVERAGE MZ_CODE_COVERAGE "Builds with code coverage flags")
+add_feature_info(MZ_SANITIZER MZ_SANITIZER_ENABLED "Builds with Address Sanitizer")


Let's just log "Sanitizer Support" instead of "Address Sanitizer", as it can be any of them.

Coeur · 2026-02-14T06:04:19Z

The sanitizer CMake is too borrowed from zlib-ng, so I'm uncomfortable changing it.

The change looks good to me.

@pmqs can you also offer a PR for the zlib-ng repository eventually?

pmqs · 2026-02-14T13:54:06Z

The sanitizer CMake is too borrowed from zlib-ng, so I'm uncomfortable changing it.

The change looks good to me.

@pmqs can you also offer a PR for the zlib-ng repository eventually?

@Coeur, as it happens I'm ahead of you. Already looking at the equivalent change in zlib-ng because there is a knock-on impact with it for Sanitizer support with the Microsoft C compiler. Will write up the details in the zlib-ng PR.

I'll park this change for now & get the same fix done in the much simpler zlib-ng use case sorted first.

Coeur · 2026-02-14T14:39:34Z

-    add_thread_sanitizer()
-elseif(MZ_SANITIZER STREQUAL "Undefined")
-    add_undefined_sanitizer()
+message(STATUS "Checking for Address Sanitizer Option")


maybe remove "Address" here?

nmoinvaz · 2026-03-10T17:29:35Z

-            cxx-compiler: g++
-            cmake-args: -D MZ_SANITIZER=Memory
-            codecov: ubuntu_gcc_msan
+          # Memory Sanitizer not currenly supported on GitHub


https://stackoverflow.com/questions/71675493/how-use-memory-sanitizer-with-use-gcc
From what I have seen, MSAN is not supported with GCC, so I think this can be removed.

https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html
No --fsanitize=memory.

Yep, that's my understanding as well -- clang only

nmoinvaz · 2026-04-08T23:57:18Z

Has this been updated with the latest changes from zlib-ng?

pmqs · 2026-04-09T08:54:29Z

Has this been updated with the latest changes from zlib-ng?

Yes, it has.

nmoinvaz · 2026-04-21T06:25:22Z

@pmqs can you please rebase this and see if those Windows builds are fixed? I would do it myself, but last time it messed it up for you.

pmqs · 2026-04-21T09:22:27Z

@pmqs can you please rebase this and see if those Windows builds are fixed? I would do it myself, but last time it messed it up for you.

@nmoinvaz try that now

nmoinvaz · 2026-04-22T00:49:12Z

Rather than deleting the MSAN job, change it to use clang, matching zlib-ng's approach:

rename to "Ubuntu Clang MSAN"
set compiler: clang-20 / cxx-compiler: clang++-20
in the apt install step, add libclang-rt-20-dev (provides the MSAN runtime) — without it, link fails even though compile succeeds

coderabbitai · 2026-04-22T13:43:38Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c3716845-4379-454d-abe8-06a42695783d

📥 Commits

Reviewing files that changed from the base of the PR and between 57792a1 and 8ec0b09.

📒 Files selected for processing (1)

.github/workflows/build.yml

🚧 Files skipped from review as they are similar to previous changes (1)

.github/workflows/build.yml

Walkthrough

This PR switches the CI MSAN job from GCC to Clang, updates MSAN-related CI tooling and build flags, normalizes and lowercases CMake sanitizer option handling, and turns several sanitizer detection failures (Memory/Thread/UB) into fatal CMake errors while adjusting ASan handling on MSVC.

Changes

Cohort / File(s)	Summary
CI Workflow Configuration `.github/workflows/build.yml`	Replaced Ubuntu GCC MSAN matrix entry with an Ubuntu Clang MSAN entry (`clang-20`/`clang++-20`), added `ninja-build`, `llvm-20`, `libclang-rt-20-dev`, set CMake to Ninja and Debug (`-GNinja -DMZ_SANITIZER=Memory`, `build-config: Debug`), set `gcov-exec: llvm-cov-20 gcov`, added `msan-options: use_sigaltstack=0`, and removed previous `codecov` matrix flag.
CMake sanitizer options & detection `CMakeLists.txt`, `cmake/detect-sanitizer.cmake`	Changed `MZ_SANITIZER` cache default from `AUTO` to `OFF`, expanded/normalized allowed values to lowercase, added `string(TOLOWER ...)` normalization and explicit empty/unknown handling, added `add_feature_info(MZ_SANITIZER ...)`, made Memory/Thread/Undefined sanitizer support checks fatal errors (previously STATUS), and adjusted ASan handling on MSVC to use `add_compile_options("/fsanitize=...","/Zi")` while avoiding link-time `/fsanitize`.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Harden the cmake Address Sanitizer detection code.' accurately describes the main change: making CMake sanitizer detection stricter by treating unknown or unsupported options as fatal errors.
Description check	✅ Passed	The description clearly explains the change: making sanitizer detection fail fast for unknown or unsupported options, and documents the resulting adjustment to build.yml (switching from GCC MSAN to Clang MSAN due to platform limitations).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b074582b-6336-49c5-9a24-b172582c5a7d

📥 Commits

Reviewing files that changed from the base of the PR and between 2f074dc and 57792a1.

📒 Files selected for processing (3)

.github/workflows/build.yml
CMakeLists.txt
cmake/detect-sanitizer.cmake

coderabbitai · 2026-04-22T13:43:41Z

+            build-config: Debug
+            # https://github.com/llvm/llvm-project/issues/55785
+            msan-options: use_sigaltstack=0


⚠️ Potential issue | 🟠 Major

build-config: Debug and msan-options are both effectively no-ops here.

Two problems in the same matrix entry:

build-config: Debug is consumed at build/test time (--config ${{ matrix.build-config || 'Release' }} on lines 260 and 263), but the configure step on line 252 hardcodes -D CMAKE_BUILD_TYPE=Release, and this job uses -GNinja (a single-config generator) so --config is ignored. Net result: the MSAN job compiles and runs a Release binary, which is exactly the configuration you don't want for triaging sanitizer reports (inlining/opt noise, poor origin tracking).

msan-options: use_sigaltstack=0 is defined but never exported. There is no reference to matrix.msan-options anywhere in the env: blocks of the Compile source code or Run test cases steps, so MSAN_OPTIONS is never set and the sigaltstack workaround (the reason the llvm issue is even linked on line 36) won't take effect at runtime.

Suggested wiring:

🔧 Proposed fix

- name: Generate project files shell: bash run: | cmake -S . -B ${{ matrix.build-dir || '.' }} ${{ matrix.cmake-args }} \ -D MZ_BUILD_TESTS=ON \ -D MZ_BUILD_UNIT_TESTS=ON \ -D BUILD_SHARED_LIBS=OFF \ - -D CMAKE_BUILD_TYPE=Release + -D CMAKE_BUILD_TYPE=${{ matrix.build-config || 'Release' }} env: CC: ${{ matrix.compiler }} CXX: ${{ matrix.cxx-compiler }} CFLAGS: ${{ matrix.cflags }} LDFLAGS: ${{ matrix.ldflags }} - name: Compile source code run: cmake --build ${{ matrix.build-dir || '.' }} --config ${{ matrix.build-config || 'Release' }} - name: Run test cases run: ctest --output-on-failure -C ${{ matrix.build-config || 'Release' }} working-directory: ${{ matrix.build-dir }} + env: + MSAN_OPTIONS: ${{ matrix.msan-options }}

nmoinvaz · 2026-04-23T02:04:25Z

@pmqs thanks for your help! I will fix the MSAN issues from here.

pmqs · 2026-04-23T12:29:59Z

@pmqs thanks for your help! I will fix the MSAN issues from here.

No worries @nmoinvaz . My fix was a work-on-progress that I didn't get a chance to look at yesterday. Thanks for finishing it off.

Coeur suggested changes Feb 14, 2026

View reviewed changes

Coeur reviewed Feb 14, 2026

View reviewed changes

Coeur approved these changes Feb 14, 2026

View reviewed changes

pmqs mentioned this pull request Feb 15, 2026

Harden the sanitizer support zlib-ng/zlib-ng#2157

Merged

pmqs force-pushed the sanitizer branch 2 times, most recently from 53fc513 to 5bede13 Compare March 10, 2026 16:36

nmoinvaz reviewed Mar 10, 2026

View reviewed changes

pmqs force-pushed the sanitizer branch from e643872 to ddbf041 Compare March 10, 2026 19:03

pmqs added 3 commits April 22, 2026 14:34

Hardening of cmake sanitizer detection code

345a10f

Remove Ubuntu GCC MSAN from build.yml

bcca34d

Rework gcc MSAN build

57792a1

pmqs force-pushed the sanitizer branch from cd82819 to 57792a1 Compare April 22, 2026 13:38

coderabbitai Bot reviewed Apr 22, 2026

View reviewed changes

Typo - use MZ_SANITIZER rather than WITH_SANITIZER

8ec0b09

nmoinvaz approved these changes Apr 23, 2026

View reviewed changes

nmoinvaz merged commit 3e10f4d into zlib-ng:develop Apr 23, 2026
29 of 30 checks passed

pmqs deleted the sanitizer branch April 23, 2026 14:42

coderabbitai Bot mentioned this pull request Apr 24, 2026

Build fails with MZ_PPMD enabled: relocation R_X86_64_PC32 cannot be used against symbol '...'; recompile with -fPIC #979

Closed

Uh oh!

Conversation

pmqs commented Feb 10, 2026

Uh oh!

nmoinvaz commented Feb 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Coeur commented Feb 14, 2026

Uh oh!

pmqs commented Feb 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmoinvaz commented Apr 8, 2026

Uh oh!

pmqs commented Apr 9, 2026

Uh oh!

nmoinvaz commented Apr 21, 2026

Uh oh!

pmqs commented Apr 21, 2026

Uh oh!

nmoinvaz commented Apr 22, 2026

Uh oh!

coderabbitai Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nmoinvaz commented Apr 23, 2026

Uh oh!

pmqs commented Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Apr 22, 2026 •

edited

Loading