Add ENABLE_UBSAN build option - #13610
Conversation
Adds UndefinedBehaviorSanitizer as a build option alongside the existing ENABLE_ASAN and ENABLE_TSAN. UBSan instruments arithmetic, shifts, and type loads rather than replacing the allocator or thread runtime, so unlike asan and tsan it composes with them rather than being mutually exclusive. Three presets: a hidden ubsan preset plus dev-ubsan and dev-asan-ubsan. vptr is excluded because a plugin loaded with dlopen does not reliably share type identity with the main image, so the check reports the plugin boundary rather than a real defect.
bryancall
added
Build
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds an ENABLE_UBSAN build option and corresponding CMake presets to enable UndefinedBehaviorSanitizer alongside existing ASAN/TSAN support.
Changes:
- Introduces
ENABLE_UBSANCMake option. - Adds a UBSan CMake configuration block (intentionally outside the ASAN/TSAN mutual-exclusion chain) with
vptrexcluded. - Adds hidden
ubsanpreset and visibledev-ubsan/dev-asan-ubsanpresets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| CMakePresets.json | Adds UBSan-related presets to enable ENABLE_UBSAN (including a combined ASAN+UBSan dev preset). |
| CMakeLists.txt | Adds ENABLE_UBSAN option and applies UBSan compile/link flags when enabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The checks are instrumentation, but they call into a runtime library that
some distributions package separately from the compiler. Fedora ships
libasan with gcc but puts libubsan in its own package, so a GCC build with
ENABLE_UBSAN compiles for a while and then stops at
ld.bfd: cannot find libubsan.so.1.0.0: No such file or directory
with nothing to connect that to the option that caused it. Establish the
runtime at configure time instead and name the missing piece.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Two corrections to the flag set. The comment claimed findings are non-fatal, but nothing in the build asked for that. Most checks recover by default, which is why a run reports every site rather than stopping at the first, but the default set differs between compilers and a few checks cannot recover at all: clang rejects -fsanitize-recover=unreachable outright. Ask for recovery explicitly so the behavior does not depend on the compiler, and say in the comment which checks still abort. -fno-sanitize=vptr is compile time instrumentation and does nothing on the link line, so drop it there. Only -fsanitize=undefined is needed at link, to pull in the runtime.
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to build configuration/presets and appears technically sound, with only minor messaging/comment clarity nits identified.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Two comment corrections. The rationale named only asan as something UBSan composes with, but the block sits outside the asan/tsan chain precisely so it works with either, so name both and note that only the asan pairing has a preset. The configure check fails either because the toolchain does not know -fsanitize=undefined or because it does and cannot link the runtime, but the message described only the second. Cover both and keep the libubsan package note as the GCC specific hint. Renamed the result variable to match, since it is not only about the runtime.
There was a problem hiding this comment.
🟡 Changes recommended
The new configure-time fatal error message gives distro-specific package guidance (“libubsan”) that can be misleading on non-Fedora platforms, so it should be made distribution-agnostic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
The message named the GCC runtime as "the libubsan package", which is the Fedora spelling. Debian and Ubuntu call it libubsan1, so naming one distribution's convention as though it were universal sends readers on the wrong errand. Say that the runtime ships separately under a name that varies instead, which stays actionable without being wrong anywhere.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are self-contained to build configuration/presets and include appropriate configure-time validation to avoid late link failures when UBSan is enabled.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
[approve ci clang-analyzer] |
Adds UndefinedBehaviorSanitizer as a build option next to the existing
ENABLE_ASANandENABLE_TSAN.Why this is not part of the asan/tsan either-or chain
ASAN replaces the allocator and TSAN replaces the thread runtime, so those two are mutually exclusive and the existing code enforces that with an
if/elseif. UBSan only instruments arithmetic, shifts, and type loads, so it composes with either of them. The new block sits outside that chain, which is what letsdev-asan-ubsanproduce-fsanitize=address,undefinedin a single build.Presets
A hidden
ubsanpreset matching the shape of the existing hiddenasanandtsanpresets, plus two visible ones:Two deliberate choices
vptris excluded. That check needs a matchingtype_infoat every polymorphic access, and a plugin loaded withdlopendoes not reliably share type identity with the main image, so it reports the plugin boundary rather than a real defect.Findings are non-fatal, so a single run reports every distinct site instead of stopping at the first. Setting
UBSAN_OPTIONS=halt_on_error=1at runtime aborts instead, which is what a gating job would want.No CI job turns this on
Enabling it anywhere would be a separate change. The option is inert until someone asks for one of the presets.
What it reports on the current tree
For context on whether the option earns its place, it finds real defects. One is a hasher buffer declared as a plain
chararray with a polymorphic object constructed into it, which makes every access through it misaligned on the URL hash and cache key path. Another is the plugin entry point, declared at the call site asvoid (*)(int, char **)while every plugin in the tree definesTSPluginInit(int, const char *argv[]).Those fixes are separate pull requests. This one only adds the option so the findings are reproducible.
Testing
Configured and built with clang 22 on Fedora 44 using
dev-ubsanwith autest and experimental plugins enabled. Build and install both clean,ctest179/179 passing, and the resulting binary links the UBSan runtime handlers.