From 5efdbb3208a170e8c2355ecb3caf78aced8895c8 Mon Sep 17 00:00:00 2001 From: Guo Chen Date: Sat, 15 Aug 2026 22:22:47 +0800 Subject: [PATCH 1/2] fix: Stop the iterator reading one limb past the end `Fbitset::const_iterator::get_next` advanced the limb pointer and then dereferenced it before checking it against the sentinel. When the last limb was exhausted, that read one limb past the end of the storage. The value read is discarded right away, so the iteration result is correct, but the read itself is undefined behaviour, and it faults under the address sanitizer. It is reached by any complete walk over a bit set, so it happens constantly. Read the limb only after the bound check. The explicit specialization of `is_no_ext` also needed to be `inline`. Without it the specialization has external linkage and is emitted in every translation unit including the header, so a program including this header from two translation units failed to link with a duplicate symbol. For a header-only library that is a hard restriction, and it is what the new second test translation unit ran into first. Add that second translation unit as a test, which both covers the linkage and walks every supported configuration to exhaustion, and add a CI job building the tests with the address and undefined behaviour sanitizers. The out-of-bounds read is invisible to the plain test run, so without the sanitizer job nothing would catch it coming back. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 32 ++++++++++++++++++++ include/fbitset.hpp | 19 ++++++++++-- test/CMakeLists.txt | 1 + test/multi_tu.cpp | 63 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 test/multi_tu.cpp diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9ddcba..bb0cbce 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -38,3 +38,35 @@ jobs: - name: Run tests run: ./build/test/testmain + + sanitize: + runs-on: ubuntu-latest + permissions: + contents: read + + steps: + - name: Checkout repository + uses: actions/checkout@v7 + with: + submodules: recursive + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y cmake g++ make + + # Out-of-bounds accesses in the bit manipulation are invisible to the + # plain test run, since the values read are normally discarded. + - name: Configure CMake with the address and undefined behaviour sanitizers + run: > + cmake -B build-sanitize -DCMAKE_BUILD_TYPE=Debug + -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer" + -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" + + - name: Build + run: cmake --build build-sanitize + + - name: Run tests + run: ./build-sanitize/test/testmain + env: + UBSAN_OPTIONS: halt_on_error=1:print_stacktrace=1 diff --git a/include/fbitset.hpp b/include/fbitset.hpp index 82aaa63..4eba4b5 100644 --- a/include/fbitset.hpp +++ b/include/fbitset.hpp @@ -32,9 +32,14 @@ using Size = size_t; namespace internal { /** Utility for checking if the container is `No_ext`. + * + * The explicit specialization needs to be `inline`. Without it the + * specialization has external linkage and gets emitted in every + * translation unit including this header, so a program including the + * header twice fails to link with a duplicate symbol. */ - template constexpr bool is_no_ext = false; - template <> constexpr bool is_no_ext = true; + template inline constexpr bool is_no_ext = false; + template <> inline constexpr bool is_no_ext = true; // // Bit operations using C++20 header @@ -523,13 +528,21 @@ class Fbitset : public Fbitset_base { * * The copy of the current limb and base will also be updated to keep * the invariant. + * + * Note that the current limb is only read after the incremented + * pointer has been checked against the sentinel. Reading it + * unconditionally would dereference one past the last limb whenever + * the final limb gets exhausted, which is undefined behaviour even + * though the value read is discarded right away. */ void get_next() noexcept { while (curr_ < last_ && limb_ == 0) { ++curr_; - limb_ = *curr_; base_ += LIMB_BITS; + if (curr_ < last_) { + limb_ = *curr_; + } } } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index dfdf8c3..e2a80f9 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -16,6 +16,7 @@ FetchContent_MakeAvailable(Catch2) add_executable(testmain testmain.cpp basic.cpp + multi_tu.cpp ) target_link_libraries(testmain PRIVATE Catch2::Catch2) diff --git a/test/multi_tu.cpp b/test/multi_tu.cpp new file mode 100644 index 0000000..11f1669 --- /dev/null +++ b/test/multi_tu.cpp @@ -0,0 +1,63 @@ +/** Tests that the header can be used from more than one translation unit. + * + * This file exists mostly so that the header is included by a second + * translation unit of the test driver. A header-only library has to link + * cleanly in that case, which requires every namespace-scope definition in it + * to have either internal or inline linkage. + * + * The iteration test here also walks a bit set all the way to exhaustion for + * every supported configuration, which is where the iterator used to read one + * limb past the end of the storage. That read is only observable under a + * sanitizer, so the CI has a job building the tests with the address + * sanitizer. + */ + +#include +#include +#include + +#include + +namespace fbitset { +template +std::ostream& operator<<(std::ostream& os, const Fbitset& bs) +{ + return os << "{Fbitset}"; +} +} + +#include + +using namespace fbitset; + +TEST_CASE("Fbitset can be iterated to exhaustion from a second translation unit") +{ + constexpr Size N_BITS = 64; + + auto walk = [N_BITS](auto bits, const std::vector& idxes) { + for (auto i : idxes) { + bits.set(i); + } + + std::vector res{}; + for (auto i = bits.begin(); i; ++i) { + res.push_back(*i); + } + CHECK(res == idxes); + + // Walking an empty bit set has to stop straight away. + decltype(bits) empty(N_BITS); + CHECK(!empty.begin()); + }; + + // A bit in the very last limb of each configuration, so that the iterator + // has to run off the end of the storage to terminate. + std::vector idxes = { 0, 31, 63 }; + + walk(Fbitset<1, uint64_t, No_ext>(N_BITS), idxes); + walk(Fbitset<2, uint32_t, No_ext>(N_BITS), idxes); + walk(Fbitset<2, uint64_t, std::vector>(N_BITS), idxes); + walk(Fbitset<2, uint32_t, std::vector>(N_BITS), idxes); + walk(Fbitset<1, uint32_t, std::vector>(N_BITS), idxes); + walk(Fbitset<>(N_BITS), idxes); +} From e7793a0d680368f2529500648019d186835cc44c Mon Sep 17 00:00:00 2001 From: Guo Chen Date: Sat, 15 Aug 2026 23:27:04 +0800 Subject: [PATCH 2/2] test: Drop the duplicated stream operator from the second unit Addressing review feedback. `test/basic.cpp` defines a stream operator for `Fbitset` so that Catch2 does not try to treat it as a range. The new translation unit repeated that definition, but it never checks an `Fbitset` directly, so it does not need one. Two copies of the same template in different translation units is exactly the kind of hazard this pull request is about, so drop the copy rather than keep it in step by hand. Co-Authored-By: Claude Opus 5 --- test/multi_tu.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/multi_tu.cpp b/test/multi_tu.cpp index 11f1669..3947ff8 100644 --- a/test/multi_tu.cpp +++ b/test/multi_tu.cpp @@ -13,19 +13,10 @@ */ #include -#include #include #include -namespace fbitset { -template -std::ostream& operator<<(std::ostream& os, const Fbitset& bs) -{ - return os << "{Fbitset}"; -} -} - #include using namespace fbitset;