Skip to content

fix(build): scope UBSAN flags to kvrocks targets and fix C flags typo - #3783

Open
zbyyehyehyeh wants to merge 1 commit into
apache:unstablefrom
zbyyehyehyeh:fix/ubsan-scope
Open

zbyyehyehyeh wants to merge 1 commit into
apache:unstablefrom
zbyyehyehyeh:fix/ubsan-scope

Conversation

@zbyyehyehyeh

Copy link
Copy Markdown
Contributor

Closes #3781.

ENABLE_UBSAN appends -fsanitize=undefined to the global
CMAKE_CXX_FLAGS, and dependencies are pulled in via
FetchContent_MakeAvailableWithArgs (i.e. add_subdirectory), so they
inherit the flags and get instrumented too. Combined with
-fno-sanitize-recover=all, a single UB report in a vendored dependency
aborts the whole process: under GCC 13.4, rocksdb's PerfStepTimer trips
-fsanitize=null on the first Seek, so the server cannot serve a single
read (repro in #3781).

The CI UBSAN job is clang-only, so this GCC-only report has never been
caught.

This change keeps the sanitizer flags in a variable and applies them per
target, so kvrocks' own code stays instrumented while vendored
dependencies do not. ASan/TSan are intentionally left as-is (full
instrumentation): unlike UBSAN here, they can report and continue, and
kvrocks already has suppression files for them.

Also fixes a pre-existing typo in the GNU branch that built
CMAKE_C_FLAGS from ${CMAKE_CXX_FLAGS}.

Verified locally on GCC 13.4:

$ nm build/_deps/rocksdb-build/librocksdb.a | grep -c __ubsan_handle
0
$ nm build-ubsan/kvrocks | grep -c __ubsan_handle
14

The server starts, PING succeeds and no UBSAN report is emitted at startup (previously it aborted on the first Seek).

AI assistance
Produced with qwen3.7max. I reviewed every line and verified the build and startup locally.

@github-actions

Copy link
Copy Markdown

Hi @zbyyehyehyeh,

Thank you for your pull request. Please review our Contributing Guide.

Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.

@zbyyehyehyeh

Copy link
Copy Markdown
Contributor Author

Responding to the checklist above.

  • What the change does: moves the UBSAN sanitizer flags out of the global
    CMAKE_CXX_FLAGS into a per-target variable (UBSAN_FLAGS), applied via
    target_compile_options / target_link_options. Vendored dependencies
    (rocksdb, tbb, ...) pulled in through FetchContent are no longer
    instrumented. Also fixes the GNU branch that built CMAKE_C_FLAGS from
    ${CMAKE_CXX_FLAGS}.
  • Why: with -fno-sanitize-recover=all, any UB report in a third-party
    dependency aborts the whole process. Under GCC 13.4, rocksdb's
    PerfStepTimer trips -fsanitize=null on the first Seek, so the server
    cannot serve a single read. The CI UBSAN job is clang-only, so this
    GCC-only failure was never caught (see [BUG] GCC UBSAN build aborts in rocksdb: sanitizer flags leak into FetchContent dependencies #3781).
  • How I verified it: nm shows rocksdb has zero __ubsan_handle
    references while kvrocks still has them; the server starts, PING
    succeeds, and no UBSAN report is emitted at startup.
  • AI usage: qwen3.7max helped draft the wording and the CMake block
    structure. I reviewed every line, verified the build on GCC 13.4, and
    confirmed the server starts cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] GCC UBSAN build aborts in rocksdb: sanitizer flags leak into FetchContent dependencies

1 participant