diff --git a/CHANGELOG.md b/CHANGELOG.md index 665ed84..acebf70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,27 @@ dated version block (`## [X.Y.Z] — YYYY-MM-DD`) when a release PR closes a mil back to `translated`, clearing the `i18n-freshness` flag the `v1.1.1` release raised. Documentation-only; no API change. +### Fixed + +- **`InstrumentedPool` data race on the growth counter ([BUG-0001](docs/bugs/2026/06/BUG-0001-instrumented-pool-growth-counter-data-race.md)).** + `notify_if_grew()` read and wrote the non-atomic `last_growths_` on the allocation + hot path, while the decorator is documented as safe to drive concurrently over a + thread-safe pool — a data race (UB) under `MUTEX` + dynamic growth. `last_growths_` + is now `std::atomic` and advanced with a `compare_exchange`, so the growth event is + emitted once per growth without a race. A concurrent `InstrumentedPool` case was + added to the ThreadSanitizer stress suite. Header-only; no API change. +- **`InstrumentedPool::deallocate` no longer underflows `live_` ([BUG-0002](docs/bugs/2026/06/BUG-0002-instrumented-pool-live-counter-underflow.md)).** + A foreign or double-freed pointer (a no-op in the core, ADR-0012) used to decrement + the unsigned `live_` counter unconditionally, wrapping it to `SIZE_MAX` and + corrupting `stats()`. The decrement now clamps at zero. The C header's + `memory_pool_free` note was corrected to stop claiming the Decorator *detects* + double-free (it counts but cannot distinguish one). Header/doc-only; no API change. +- **`InstrumentedPool` move-assignment now emits `destroyed` for the replaced pool + ([BUG-0003](docs/bugs/2026/06/BUG-0003-instrumented-pool-move-assign-missing-destroyed-event.md)).** + Move-assigning over an instrumented pool released its `Pool` and observers without + notifying `PoolEvent::destroyed`, asymmetric with the destructor. It now notifies + before reassignment. Header-only; no API change. + ## Released versions Each released version is an **immutable** entry under [`docs/changelog/`](docs/changelog/) — one file per release, newest first ([ADR-0038](docs/adr/0038-changelog-version-split.md)). They are never edited after release; only the `Unreleased` block above changes during development. diff --git a/docs/bugs/2026/06/BUG-0001-instrumented-pool-growth-counter-data-race.md b/docs/bugs/2026/06/BUG-0001-instrumented-pool-growth-counter-data-race.md new file mode 100644 index 0000000..14b326c --- /dev/null +++ b/docs/bugs/2026/06/BUG-0001-instrumented-pool-growth-counter-data-race.md @@ -0,0 +1,68 @@ +--- +id: BUG-0001 +title: Data race on InstrumentedPool::last_growths_ under concurrent use +status: fixed +severity: high +reporter: third-party +discovered: 2026-06-15 +affected-versions: ">=0.6.0,<1.1.2" +fixed-in: v1.1.2 +--- + +# BUG-0001: Data race on InstrumentedPool::last_growths_ under concurrent use + +## Summary + +`InstrumentedPool::notify_if_grew()` read **and wrote** the non-atomic member +`last_growths_` on the hot allocation path, while the class contract explicitly +permits driving the decorator concurrently over a thread-safe pool — a data race +(C++ memory-model UB). + +## Environment + +- **Affected versions:** `>=0.6.0,<1.1.2` (`InstrumentedPool` landed in Milestone 6). +- **Configuration:** `PBR_MEMORY_POOL_THREAD_SAFETY = MUTEX` **and** a dynamic + (growing) pool, driven from multiple threads. Not reachable under `LOCKFREE` + (dynamic growth is disabled there, ADR-0024 §2, so the growth counter stays 0) + nor under `NONE` (single-threaded by contract). + +## Reproduction + +Wrap a dynamic `MUTEX` pool in an `InstrumentedPool` and allocate from several +threads so the pool grows repeatedly. `notify_if_grew()` — called from +`allocate()` / `try_allocate()` — does `if (growths > last_growths_) { last_growths_ = growths; … }` +on a plain `std::size_t`, concurrently. ThreadSanitizer flags the race; the +pre-fix `concurrency_stress_test` did not exercise `InstrumentedPool` at all. + +## Expected vs. actual + +- **Expected:** the contract says the decorator "is safe to wrap a thread-safe + (`MUTEX`/`LOCKFREE`) pool and drive it concurrently". +- **Actual:** concurrent allocations race on `last_growths_` — undefined behaviour + (a data race is UB regardless of the "approximate metrics" caveat, which only + excuses the *value*, not the race). + +## Root cause + +`last_growths_` was a plain `std::size_t` while every other counter was a +`std::atomic`. The growth-detection read-modify-write was therefore unsynchronized. + +## Impact + +UB in a Tier-1 supported configuration (`MUTEX` + dynamic growth + concurrency + +instrumentation). High severity: the unsafe access is on the allocation hot path. + +## Fix / workaround + +Made `last_growths_` a `std::atomic`; `notify_if_grew()` now advances +it with a relaxed `compare_exchange_weak`, so the winning thread notifies `grew` +exactly once per observed growth and concurrent callers neither race nor +double-notify. The move ctor/assignment load/store it atomically. Added an +`InstrumentedPool`-over-dynamic-pool concurrency case to `concurrency_stress_test` +(run under the MUTEX ThreadSanitizer CI job). + +## References + +- Fixing change: branch `fix/instrumented-pool-correctness` — see the `CHANGELOG` `Fixed` entry. +- [`instrumented_pool.hpp`](../../../../src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp) — `notify_if_grew`, `last_growths_`. +- [ADR-0025](../../../adr/0025-decorator-for-instrumented-pool.md) (Decorator) · [ADR-0026](../../../adr/0026-observer-for-pool-lifecycle-events.md) (Observer / growth event) · [ADR-0024](../../../adr/0024-dynamic-growth-synchronization-and-creation-surface.md) (LOCKFREE never grows). diff --git a/docs/bugs/2026/06/BUG-0002-instrumented-pool-live-counter-underflow.md b/docs/bugs/2026/06/BUG-0002-instrumented-pool-live-counter-underflow.md new file mode 100644 index 0000000..bc8d653 --- /dev/null +++ b/docs/bugs/2026/06/BUG-0002-instrumented-pool-live-counter-underflow.md @@ -0,0 +1,75 @@ +--- +id: BUG-0002 +title: InstrumentedPool::deallocate underflows live_ on a foreign or double-freed pointer +status: fixed +severity: medium +reporter: third-party +discovered: 2026-06-15 +affected-versions: ">=0.6.0,<1.1.2" +fixed-in: v1.1.2 +--- + +# BUG-0002: InstrumentedPool::deallocate underflows live_ on a foreign or double-freed pointer + +## Summary + +`InstrumentedPool::deallocate()` unconditionally decremented the unsigned `live_` +counter for any non-null pointer, even though the core `memory_pool_free` silently +ignores foreign / misaligned pointers (ADR-0012). A foreign pointer (or a +double-free) therefore drove `live_` below zero, **wrapping to `SIZE_MAX`** and +corrupting `stats()` / `write_summary()`. + +## Environment + +- **Affected versions:** `>=0.6.0,<1.1.2`. +- **Configuration:** any; triggered by a caller passing a non-null pointer that the + core treats as a no-op (out-of-range / misaligned / already-freed). + +## Reproduction + +```cpp +InstrumentedPool pool{Pool(64, 4)}; +void* a = pool.try_allocate(); +pool.deallocate(a); // live_ -> 0 +int stack_var = 0; +pool.deallocate(&stack_var); // foreign: core no-op, but live_ decremented -> SIZE_MAX +// pool.stats().live_ == SIZE_MAX +``` + +## Expected vs. actual + +- **Expected:** `live_` reflects outstanding blocks and never underflows; a free the + core rejects must not corrupt the diagnostic. +- **Actual:** `live_` underflowed to `SIZE_MAX`; the summary reported absurd values. + +## Root cause + +The counter update was gated only on `block != nullptr`, not on whether the core +actually accepted the free — and the core gives no acceptance signal. `live_` being +`std::size_t`, the over-decrement wrapped. + +Related documentation defect: the C header (`memory_pool.h`) claimed the Decorator +*addresses* double-free. It does not — it counts deallocations but cannot +distinguish a double-free from a legitimate free. The header wording was corrected +in the same change (the chosen scope is to fix the counter and tell the truth, not +to implement double-free detection — that would be a separate feature). + +## Impact + +Corrupted diagnostics (not memory corruption — the core stays safe). Medium: it +surfaces only on caller misuse, but the wrong-by-`SIZE_MAX` counter is badly +misleading, and the header over-promised. + +## Fix / workaround + +`deallocate()` now decrements `live_` with a clamp-at-zero `compare_exchange_weak` +loop, so a rejected/foreign/double free can never underflow it. `deallocations_` +still counts every non-null call (its documented meaning). The header's double-free +note was rewritten to stop promising Decorator detection. + +## References + +- Fixing change: branch `fix/instrumented-pool-correctness` — see the `CHANGELOG` `Fixed` entry. +- [`instrumented_pool.hpp`](../../../../src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp) — `deallocate`. +- [`memory_pool.h`](../../../../src/main/cpp/it/d4np/memorypool/memory_pool.h) — `memory_pool_free` double-free note. +- [ADR-0012](../../../adr/0012-foreign-pointer-and-out-of-range-pointer-policy.md) (foreign-pointer no-op) · [ADR-0025](../../../adr/0025-decorator-for-instrumented-pool.md) (Decorator). diff --git a/docs/bugs/2026/06/BUG-0003-instrumented-pool-move-assign-missing-destroyed-event.md b/docs/bugs/2026/06/BUG-0003-instrumented-pool-move-assign-missing-destroyed-event.md new file mode 100644 index 0000000..c383d68 --- /dev/null +++ b/docs/bugs/2026/06/BUG-0003-instrumented-pool-move-assign-missing-destroyed-event.md @@ -0,0 +1,64 @@ +--- +id: BUG-0003 +title: InstrumentedPool move-assignment does not notify destroyed for the replaced pool +status: fixed +severity: low +reporter: third-party +discovered: 2026-06-15 +affected-versions: ">=0.6.0,<1.1.2" +fixed-in: v1.1.2 +--- + +# BUG-0003: InstrumentedPool move-assignment does not notify destroyed for the replaced pool + +## Summary + +`InstrumentedPool::operator=(InstrumentedPool&&)` released the destination's pool +and overwrote its observer list without emitting `PoolEvent::destroyed` for the +pool being replaced. Observers attached to the destination's prior contents +silently lost their subject — asymmetric with the destructor, which does notify. + +## Environment + +- **Affected versions:** `>=0.6.0,<1.1.2`. +- **Configuration:** any; triggered by move-assigning onto an `InstrumentedPool` + that already has registered observers. + +## Reproduction + +```cpp +RecordingObserver obs; +InstrumentedPool dest{Pool(64, 4)}; +dest.add_observer(obs); +dest = InstrumentedPool{Pool(64, 4)}; // replaces dest's pool +// before the fix: obs.destroyed_ == 0 (the replaced pool went away silently) +``` + +## Expected vs. actual + +- **Expected:** the pool being replaced is going away, so its observers should see + `destroyed` — the same guarantee the destructor gives. +- **Actual:** no `destroyed` event was delivered on move-assignment. + +## Root cause + +`operator=` overwrote `pool_` and `observers_` without first notifying the existing +observers. Not a leak (the moved-into `Pool` is properly released), but a +lifecycle-event asymmetry. + +## Impact + +Low / cosmetic: observers relying on a balanced `destroyed` per subject miss one on +move-assignment. No memory-safety consequence. + +## Fix / workaround + +`operator=` now calls `notify(PoolEvent::destroyed)` at the top of the +self-assignment-guarded block, before the pool and observer list are overwritten — +symmetric with the destructor. Covered by a new move-assignment observer test. + +## References + +- Fixing change: branch `fix/instrumented-pool-correctness` — see the `CHANGELOG` `Fixed` entry. +- [`instrumented_pool.hpp`](../../../../src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp) — `operator=`, `~InstrumentedPool`. +- [ADR-0026](../../../adr/0026-observer-for-pool-lifecycle-events.md) (Observer lifecycle events). diff --git a/docs/bugs/README.md b/docs/bugs/README.md index 44e05e1..6e7f7c3 100644 --- a/docs/bugs/README.md +++ b/docs/bugs/README.md @@ -73,11 +73,12 @@ checked by `python tools/consistency_lint.py` (the `bugs` check). ## Index -Newest first, grouped by year and month. *(No defects recorded yet — the ledger is empty -as of `v1.1.0`.)* +Newest first, grouped by year and month. + +### 2026 — June - +| [BUG-0001](2026/06/BUG-0001-instrumented-pool-growth-counter-data-race.md) | Data race on `InstrumentedPool::last_growths_` under concurrent use | `fixed` | high | 2026-06-15 | +| [BUG-0002](2026/06/BUG-0002-instrumented-pool-live-counter-underflow.md) | `InstrumentedPool::deallocate` underflows `live_` on a foreign / double-freed pointer | `fixed` | medium | 2026-06-15 | +| [BUG-0003](2026/06/BUG-0003-instrumented-pool-move-assign-missing-destroyed-event.md) | `InstrumentedPool` move-assignment does not notify `destroyed` for the replaced pool | `fixed` | low | 2026-06-15 | diff --git a/src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp b/src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp index d7e7475..737f7b0 100644 --- a/src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp +++ b/src/main/cpp/it/d4np/memorypool/instrumented_pool.hpp @@ -131,11 +131,14 @@ class InstrumentedPool { allocation_failures_(other.allocation_failures_.load(std::memory_order_relaxed)), live_(other.live_.load(std::memory_order_relaxed)), peak_live_(other.peak_live_.load(std::memory_order_relaxed)), observers_(std::move(other.observers_)), - last_growths_(other.last_growths_) {} + last_growths_(other.last_growths_.load(std::memory_order_relaxed)) {} - /** Move-assign; releases the current pool and re-seeds the counters + observers. */ + /** Move-assign; releases the current pool and re-seeds the counters + observers. + * The pool being replaced is going away, so its observers get a `destroyed` + * event first — symmetric with the destructor (BUG-0003). */ InstrumentedPool& operator=(InstrumentedPool&& other) noexcept { if (this != &other) { + notify(PoolEvent::destroyed); pool_ = std::move(other.pool_); allocations_.store(other.allocations_.load(std::memory_order_relaxed), std::memory_order_relaxed); deallocations_.store(other.deallocations_.load(std::memory_order_relaxed), std::memory_order_relaxed); @@ -144,7 +147,7 @@ class InstrumentedPool { live_.store(other.live_.load(std::memory_order_relaxed), std::memory_order_relaxed); peak_live_.store(other.peak_live_.load(std::memory_order_relaxed), std::memory_order_relaxed); observers_ = std::move(other.observers_); - last_growths_ = other.last_growths_; + last_growths_.store(other.last_growths_.load(std::memory_order_relaxed), std::memory_order_relaxed); } return *this; } @@ -191,7 +194,13 @@ class InstrumentedPool { void deallocate(void* block) noexcept { if (block != nullptr) { deallocations_.fetch_add(1U, std::memory_order_relaxed); - live_.fetch_sub(1U, std::memory_order_relaxed); + // Clamp at zero: a foreign or already-freed pointer is a no-op in the + // core (ADR-0012), so an unconditional fetch_sub would underflow `live_` + // to SIZE_MAX on such a call (BUG-0002). Decrement only while positive. + std::size_t live = live_.load(std::memory_order_relaxed); + while (live > 0U && !live_.compare_exchange_weak(live, live - 1U, std::memory_order_relaxed)) { + // `live` is reloaded by compare_exchange_weak on failure; retry. + } } pool_.deallocate(block); } @@ -250,9 +259,16 @@ class InstrumentedPool { * the dynamic pool grew, so notify `grew`. */ void notify_if_grew() noexcept { const std::size_t growths = ::memory_pool_growths(pool_.native_handle()); - if (growths > last_growths_) { - last_growths_ = growths; - notify(PoolEvent::grew); + // Advance the high-water growth count atomically; the thread that wins the + // compare-exchange notifies `grew` exactly once per observed growth, so + // concurrent callers neither race on the counter nor double-notify (BUG-0001). + std::size_t last = last_growths_.load(std::memory_order_relaxed); + while (growths > last) { + if (last_growths_.compare_exchange_weak(last, growths, std::memory_order_relaxed)) { + notify(PoolEvent::grew); + break; + } + // `last` is reloaded by compare_exchange_weak on failure; re-check. } } @@ -263,7 +279,9 @@ class InstrumentedPool { std::atomic live_{0U}; std::atomic peak_live_{0U}; std::vector observers_; - std::size_t last_growths_{0U}; + // Atomic: read+advanced on the hot allocation path (`notify_if_grew`), which the + // class contract allows to run concurrently over a thread-safe pool (BUG-0001). + std::atomic last_growths_{0U}; }; } // namespace it::d4np::memorypool diff --git a/src/main/cpp/it/d4np/memorypool/memory_pool.h b/src/main/cpp/it/d4np/memorypool/memory_pool.h index e1117f9..210e32b 100644 --- a/src/main/cpp/it/d4np/memorypool/memory_pool.h +++ b/src/main/cpp/it/d4np/memorypool/memory_pool.h @@ -163,8 +163,10 @@ void* memory_pool_alloc(memory_pool_t* pool); * pool state is bit-identical before and after a no-op call. Note that * the policy does NOT detect *double-free* on a legitimately-in-range, * already-on-the-free-list pointer; that case remains undefined - * behaviour and is addressed by the optional Decorator instrumentation - * landing in Milestone 6. + * behaviour. The optional `InstrumentedPool` Decorator (Milestone 6) + * counts deallocation calls but does not detect a double-free either — + * it cannot distinguish one from a legitimate free — so it must not be + * relied on to make a double-free safe. * * @param pool Pool the block originally came from. * @param block Block to release, or `NULL`, or a foreign pointer. diff --git a/src/test/cpp/it/d4np/memorypool/concurrency_stress_test.cpp b/src/test/cpp/it/d4np/memorypool/concurrency_stress_test.cpp index 0e36a17..69e1dfa 100644 --- a/src/test/cpp/it/d4np/memorypool/concurrency_stress_test.cpp +++ b/src/test/cpp/it/d4np/memorypool/concurrency_stress_test.cpp @@ -35,12 +35,14 @@ #define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN +#include #include #include #include #include #include +#include #include #include #include @@ -49,6 +51,7 @@ #if PBR_MEMORY_POOL_THREAD_SAFETY != PBR_MEMORY_POOL_THREAD_SAFETY_NONE +using it::d4np::memorypool::InstrumentedPool; using it::d4np::memorypool::Pool; namespace { @@ -180,6 +183,45 @@ TEST_CASE("a vended block is exclusively owned while held (no double-vend)") { CHECK_FALSE(violated.load(std::memory_order_relaxed)); } +TEST_CASE("concurrent InstrumentedPool over a dynamic pool is race-free on the growth counter (BUG-0001)") { + std::optional opt = InstrumentedPool::make_dynamic(BLOCK_SIZE, 64U, 2U); + if (!opt.has_value()) { + return; // LOCKFREE: dynamic mode rejected (ADR-0024 §2) — no growth path to race on + } + InstrumentedPool& pool = *opt; + + // Drive concurrent allocations that force repeated growth. Before the fix, + // notify_if_grew() read+wrote the non-atomic last_growths_ on this hot path; + // ThreadSanitizer flags that race under MUTEX. Each thread keeps its blocks so + // nothing is freed mid-flight (the focus is the growth-counter access). + constexpr int PER_THREAD = 2000; + std::vector> grabbed(THREAD_COUNT); + std::vector threads; + threads.reserve(THREAD_COUNT); + for (unsigned t = 0; t < THREAD_COUNT; ++t) { + threads.emplace_back([&pool, &grabbed, t] { + std::vector& mine = grabbed.at(t); + for (int i = 0; i < PER_THREAD; ++i) { + void* const block = pool.try_allocate(); + if (block != nullptr) { + mine.push_back(block); + } + } + }); + } + for (std::thread& th : threads) { + th.join(); + } + + CHECK(pool.stats().allocation_failures_ == 0U); // dynamic: grows rather than failing + + for (const std::vector& v : grabbed) { + for (void* const p : v) { + pool.deallocate(p); + } + } +} + #else // PBR_MEMORY_POOL_THREAD_SAFETY != NONE TEST_CASE("concurrency stress is gated to thread-safe builds") { diff --git a/src/test/cpp/it/d4np/memorypool/instrumented_pool_test.cpp b/src/test/cpp/it/d4np/memorypool/instrumented_pool_test.cpp index 8b2e21f..89e5b62 100644 --- a/src/test/cpp/it/d4np/memorypool/instrumented_pool_test.cpp +++ b/src/test/cpp/it/d4np/memorypool/instrumented_pool_test.cpp @@ -222,3 +222,26 @@ TEST_CASE("an observer is notified of growth on a dynamic pool (ADR-0026)") { opt->deallocate(block); } } + +TEST_CASE("InstrumentedPool::deallocate clamps live_ at zero on a foreign pointer (BUG-0002)") { + InstrumentedPool pool{Pool(BLOCK_SIZE, 4U)}; + void* const a = pool.try_allocate(); + REQUIRE(a != nullptr); + pool.deallocate(a); // live_ -> 0 + + int stack_var = 0; + pool.deallocate(&stack_var); // foreign pointer: core no-op (ADR-0012); must not underflow + + const PoolStats s = pool.stats(); + CHECK(s.live_ == 0U); // clamped at zero, not wrapped to SIZE_MAX + CHECK(s.deallocations_ == 2U); // both non-null calls counted (documented meaning) +} + +TEST_CASE("InstrumentedPool move-assignment notifies destroyed for the replaced pool (BUG-0003)") { + RecordingObserver obs; // out-lives both pools below + InstrumentedPool dest{Pool(BLOCK_SIZE, 4U)}; + dest.add_observer(obs); + + dest = InstrumentedPool{Pool(BLOCK_SIZE, 4U)}; // replaces dest's pool + CHECK(obs.destroyed_ == 1); // the replaced pool announced destruction +}