fix: Stop the iterator reading one limb past the end

.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

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 <typename T> constexpr bool is_no_ext = false;
-    template <> constexpr bool is_no_ext<No_ext> = true;
+    template <typename T> inline constexpr bool is_no_ext = false;
+    template <> inline constexpr bool is_no_ext<No_ext> = true;
 
     //
     // Bit operations using C++20 <bit> header
@@ -523,13 +528,21 @@ class Fbitset : public Fbitset_base<N, L, E> {
          *
          * 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_;
+                }
             }
         }
 

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)

test/multi_tu.cpp

@@ -0,0 +1,54 @@
+/** 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 <cstdint>
+#include <vector>
+
+#include <fbitset.hpp>
+
+#include <catch2/catch_test_macros.hpp>
+
+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<Size>& idxes) {
+        for (auto i : idxes) {
+            bits.set(i);
+        }
+
+        std::vector<Size> 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<Size> 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<uint64_t>>(N_BITS), idxes);
+    walk(Fbitset<2, uint32_t, std::vector<uint32_t>>(N_BITS), idxes);
+    walk(Fbitset<1, uint32_t, std::vector<uint32_t>>(N_BITS), idxes);
+    walk(Fbitset<>(N_BITS), idxes);
+}