Skip to content

Commit 856b0a2

Browse files
committed
fix(buffers): a disposed pool owner throws, and default is not Empty (#2056, #2057)
#2056(a) -- MemoryPoolHeapOwner_::getMemoryProperty had no disposed check and returned a zero-length Memory after Dispose(), so a caller could not tell a disposed owner from a live Rent(0). It now throws ObjectDisposedException, which is what .NET's ArrayMemoryPoolBuffer.Memory does. Why a flag and not .NET's own discriminator, recorded as a decision rather than a comment. .NET needs no flag: it nulls _array and tests `array is null`. The port's natural equivalent, unique_ptr<vector<T>>, would even make the object SMALLER. It was rejected because reset() frees the storage deterministically where clear() + shrink_to_fit() is non-binding, and half (b) is still open -- that would turn a latent use-after-free from "usually survives" into "always broken" while nothing yet fixes it. The storage lifetime is left byte-identical and a test asserts it, so a future change adopting the null discriminator must confront half (b) at the same time. Half (b) -- a Memory<T> obtained before Dispose keeping a pointer and length over released storage -- is a Memory<T> ownership change in Core.Base, cannot be defended against from this type, and stays pinned as unfixed rather than letting half a repair look whole. #2057 -- a default-constructed ReadOnlySequence<T> enumerated one segment where .NET yields zero, and getEmpty() returned the same object so the two were indistinguishable. .NET's Empty is new ReadOnlySequence<T>(Array.Empty<T>()) -- array-backed, one segment -- while default has a null start object and MoveNext returns false immediately for it. The review's analysis was exactly right: a std::vector cannot express the difference, because both hold an empty one. The new member is the port's _startObject != null. Every other observable stays identical between the two, and that is asserted, because that identity is why a member was needed at all. The trap the tests cover: (nullptr, 0) is a VALID buffer-backed sequence with no elements and must still enumerate one segment. Only the default constructor yields none, and a mutation setting the flag from `length > 0` is caught by exactly that case. Both types grow by 8 bytes under SA-3, pinned by the existing static_asserts. modules/buffers is header-only and nothing outside it includes either type; both consumers reference System::Buffers in zero places. +2 net tests. Five mutations, all caught. One pre-existing test asserted the old disposed behaviour and was rewritten rather than deleted. Gate: 17,240 run, 0 failed, 38 executables -- green. SR-AUD-071 (half a) and SR-AUD-074 -> remediated. #2058 stays open. Record: docs/Migration-BuffersDisposedOwnerAndDefaultSequence.md.
1 parent cbaff4f commit 856b0a2

8 files changed

Lines changed: 248 additions & 47 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

audit/AUDIT_FINDINGS_INDEX.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,10 @@ implementation ticket.
8282
| [SR-AUD-068](modules/core/include/System/ValueType.hpp.audit.md#sr-aud-068--medium--valuetype-is-publicly-constructible-and-defaults-to-identity-rather-than-net-value-semantics) | medium | confirmed | `ValueType.hpp`, `ValueTypeTests.cpp` | Public C++ `ValueType` is constructible and uses identity/address defaults, where .NET has an abstract fieldwise-value base; direct tests lock in the divergent fallback. **Reviewed 2026-08-11 (#2322); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `Equals` is `this == &other`, `GetHashCode` is the object address narrowed to `intcs`, `ToString` is the literal `"System.ValueType"`, and the implicit public default constructor makes `System::ValueType v;` compile. **Measured consumer surface: nothing derives from it in production** — the only derived types anywhere are `SimpleValueType` and `ConcreteValueType` in `ValueTypeTests.cpp`, and `System::Void` documents that it deliberately does *not* derive from it, so the finding's harm has no in-repository instance and the exposure is entirely downstream. **Not autonomous, on two different grounds.** Field-by-field `Equals`/`GetHashCode` and a runtime-type-name `ToString` are reflection, which `CLAUDE.md` lists as a **permanent deviation, out of scope** — not a TODO. What remains is making the incompatible default unreachable, and every route is a **public source break** in a shipped header (a `protected` constructor, matching .NET's own `protected ValueType()`, or an abstract class) — the same class as SR-AUD-063. Options priced in #2322. `docs/CoreOwnedFindingsReviews2317.md`. |
8383
| [SR-AUD-069](modules/core/include/System/SequencePosition.hpp.audit.md#sr-aud-069--medium--sequenceposition-exposes-mutable-public-representation-instead-of-an-opaque-readonly-position) | medium | confirmed | `SequencePosition.hpp`, `Batch6BuffersTests.cpp` | Public mutable `void*`/integer components let callers rewrite a returned position, unlike .NET's private readonly opaque representation; tests never protect that boundary. **Reviewed 2026-08-12 (#2331); STILL CONFIRMED — it is a CONJUNCTION, and its value-contract clause is REMEDIATED while its representation clause is `needs_user` (#2332).** **Measured consumer surface:** seven files mention the type, and **direct field access exists in exactly one place — inside `SequencePosition` itself**; every other use already goes through the constructor and `GetObject()`/`GetInteger()`, so the encapsulation repair's first-party cost is zero. The two closest siblings settle the convention: `System::Index` and `System::Range` are classes with private fields and public `Equals`/`GetHashCode`. **#2331 (`done`), purely additive:** `Equals(const SequencePosition&)` and `GetHashCode()` are added and `operator==`/`!=` delegate to `Equals`, so the named and operator forms cannot drift. The hash is `((h1 << 5) + h1) ^ h2` evaluated in `uintcs` — signed overflow here would be UB, the CCF-004 class already recorded for `detail::tupleHashCombine` — with the pointer fold guarded by `if constexpr (sizeof(std::uintptr_t) > sizeof(uintcs))` because `bits >> 32` on a 32-bit target is a shift at the operand width. **It is deliberately not .NET's hash value** and the header says so; that is *not* the unverifiable-reference-text class of #2321/#2323, because a hash value is documented unstable in .NET too and this port already ships two hashes that differ from .NET's (`Range::GetHashCode`'s `397`, and `System::HashCode`'s per-process `std::random_device` seed). **+7 tests**, closing the finding's own list — equal and unequal **non-null** segments (every pre-existing direct test used `nullptr`), default equality, hash agreement, hash stability, and the caveat that component equality is not sequence-location identity — all obeying `docs/HashAssertionContractRule.md` (R1 asserted, no R2 pair, segment dependence stated over a family of eight, one R3 pin naming its property: a null segment folds to zero so the hash reduces to the integer's own bits). Three mutations, three caught. No layout, vtable, signature or `noexcept` change. **#2332 (`needs_user`):** making `object_`/`integer_` private is a public source break (structured bindings, designated initialisers, direct assignment) with zero first-party cost and unmeasurable downstream cost; layout is unaffected because all members keep the same access. The header now documents the divergence without taking the decision. `docs/CoreOwnedFindingsReviews2317.md`. |
8484
| [SR-AUD-070](modules/buffers/include/System/Buffers/ArrayBufferWriter.hpp.audit.md#sr-aud-070--medium--arraybufferwritert-silently-requires-a-default-constructible-t) | medium | remediated | `ArrayBufferWriter.hpp`, `MemoryPool.hpp` | Vector resize and `Clear` impose an undocumented default-constructor requirement on `T`; a valid non-default-constructible type fails to compile at `GetSpan`, and MemoryPool repeats the constraint. **Remediated (#2054, 2026-08-04, family B-C, `docs/BuffersNamespaceReviewPlan.md` §4.4/§23.4):** each requirement is now stated in the owning type's Doxygen block and `static_assert`ed **at the point where it was already enforced**, so *exactly the same set of programs compiles* and only the diagnostic changes — measured site by site against the pre-change headers materialised from `b294738`. **Two premise corrections.** (a) The site count is **six**, not two: the review already promoted `ArrayBufferWriter::checkAndResizeBuffer`, `::Clear`, `MemoryPoolHeapOwner_`'s constructor and `SharedArrayPool<T>::Rent` + `ArrayPool<T>::Return(clearArray=true)` to first-class sites, and implementation found the sixth, `ArrayBufferWriter(intcs initialCapacity)`, which resizes on its own. `SequenceReader<T>::TryRead`/`TryPeek` are two further `T{}` sites whose requirement was **already documented** (the CCF-014 contract); they gain the assert but were never silent. (b) "fails to compile at `GetSpan`" understates where it bites: `GetSpan`/`GetMemory` are `virtual` overrides and `ArrayPool`'s `Rent`/`Return` are `virtual`, so their bodies are instantiated **for the vtable** — measured, `ArrayBufferWriter<NoDefault> w;` and `ArrayPool<NoDefault>::Shared().Return(v, false)` were already rejected before this ticket. Naming the type and `sizeof` stay legal for any `T`, which is why no assert is at class scope. Copy-assignability, which `Clear` and `Return(…, true)` also require, was undocumented too and is now asserted. 13 negative consumer sites (`test/consumer/buffers_generic_requirements_negative.cpp`) prove the rejected half; 9 tests in `BuffersGenericRequirementsTests.cpp` pin the accepted half. No runtime code, signature, layout, vtable or `noexcept` specification changed. |
85-
| [SR-AUD-071](modules/buffers/include/System/Buffers/MemoryPool.hpp.audit.md#sr-aud-071--high--memorypool-owner-permits-post-dispose-access-and-invalidates-retained-memory-into-a-native-fault) | high | confirmed (design-complete) | `MemoryPool.hpp`, `Batch6BuffersTests.cpp` | A disposed owner returns empty Memory rather than throwing; retained pre-dispose Memory retains length over freed vector storage and ASan confirms a native null-read fault. |
85+
| [SR-AUD-071](modules/buffers/include/System/Buffers/MemoryPool.hpp.audit.md#sr-aud-071--high--memorypool-owner-permits-post-dispose-access-and-invalidates-retained-memory-into-a-native-fault) | high | remediated | `MemoryPool.hpp`, `Batch6BuffersTests.cpp` | A disposed owner returns empty Memory rather than throwing; retained pre-dispose Memory retains length over freed vector storage and ASan confirms a native null-read fault. **HALF (a) REMEDIATED (#2056, 2026-08-17); half (b) is still open and stays pinned as such.** `MemoryPoolHeapOwner_::getMemoryProperty` had no disposed check and returned a **zero-length** `Memory` after `Dispose()`, so a caller could not tell a disposed owner from a live `Rent(0)`. It now throws `ObjectDisposedException`, which is what .NET's `ArrayMemoryPoolBuffer.Memory` does (`ArrayMemoryPool.ArrayMemoryPoolBuffer.cs:18-25`). `sizeof` 32 → 40 under SA-3. **Why a flag and not .NET's own discriminator, recorded as a decision rather than a comment**: .NET needs no flag — it nulls `_array` and tests `array is null` — and the port's natural equivalent, a `unique_ptr<vector<T>>`, would even make the object *smaller*. It was rejected because `reset()` frees the storage **deterministically** where `clear() + shrink_to_fit()` is non-binding, and half (b) is still open: that would turn a latent use-after-free from "usually survives" into "always broken" while nothing yet fixes it. The storage lifetime is left byte-identical and a test asserts it, so a future change adopting the null discriminator must confront half (b) at the same time. **Half (b)** — a `Memory<T>` obtained before `Dispose()` keeping a pointer and length over released storage — is a `Memory<T>` ownership change in `Core.Base`, cannot be defended against from this type, and remains pinned as unfixed rather than letting half a repair look whole. +1 net test, two mutations both caught. Downstream measured: zero `System::Buffers` sites in either consumer. `docs/Migration-BuffersDisposedOwnerAndDefaultSequence.md`. |
8686
| [SR-AUD-072](modules/buffers/include/System/Buffers/ReadOnlySequence.hpp.audit.md#sr-aud-072--high--raw-pointer-readonlysequence-construction-dereferences-invalid-pointerlength-metadata) | high | remediated | `ReadOnlySequence.hpp`, `Batch6BuffersTests.cpp` | The raw pointer constructor forms vector iterators without null or signed-length validation; `nullptr, 1` reaches UBSan/ASan-confirmed null dereference. |
8787
| [SR-AUD-073](modules/buffers/include/System/Buffers/ReadOnlySequence.hpp.audit.md#sr-aud-073--high--tryget-accepts-positions-before-the-sequence-start-and-forms-out-of-bounds-views) | high | remediated | `ReadOnlySequence.hpp`, `Batch6BuffersTests.cpp` | `TryGet` ignores position provenance and lower bound; a forged before-start position exposes data outside a slice and negative position yields ASan-confirmed heap overflow. |
88-
| [SR-AUD-074](modules/buffers/include/System/Buffers/ReadOnlySequence.hpp.audit.md#sr-aud-074--medium--default-readonlysequence-enumerates-one-empty-segment-instead-of-none) | medium | confirmed (design-complete) | `ReadOnlySequence.hpp`, `Batch6BuffersTests.cpp` | C++ collapses default and explicit-empty sequence state, so default enumeration returns one empty segment where .NET default returns none. |
88+
| [SR-AUD-074](modules/buffers/include/System/Buffers/ReadOnlySequence.hpp.audit.md#sr-aud-074--medium--default-readonlysequence-enumerates-one-empty-segment-instead-of-none) | medium | remediated | `ReadOnlySequence.hpp`, `Batch6BuffersTests.cpp` | C++ collapses default and explicit-empty sequence state, so default enumeration returns one empty segment where .NET default returns none. **REMEDIATED (#2057, 2026-08-17).** A default-constructed `ReadOnlySequence<T>` enumerated **one** segment where .NET yields **zero**, and `getEmpty()` returned the same object so the two were indistinguishable. .NET's `Empty` is `new ReadOnlySequence<T>(Array.Empty<T>())` (`ReadOnlySequence.cs:26`) — array-backed, so it has a start object and yields one segment — while `default` has a null one and `Enumerator.MoveNext` returns false immediately for it (`:642-647`). A `std::vector` cannot express the difference, since both hold an empty one, which is exactly why the review recorded that "distinguishing them needs state the type does not have": the new member is the port's `_startObject != null`, taking `sizeof(ReadOnlySequence<int>)` 32 → 40 under SA-3. **Every other observable stays identical between the two** — length, `IsEmpty`, `Start`, `End`, `IsSingleSegment` — which is asserted, because that identity is why a member was needed at all. The trap the tests cover: `(nullptr, 0)` is a **valid buffer-backed** sequence with no elements and must still enumerate one segment; only the default constructor yields none, and a mutation setting the flag from `length > 0` is caught by it. +3 net tests, three mutations all caught. #2058 is a separate ticket and stays open. Downstream measured: zero `System::Buffers` sites in either consumer. `docs/Migration-BuffersDisposedOwnerAndDefaultSequence.md`. |
8989
| [SR-AUD-075](modules/buffers/include/System/Buffers/SequenceReader.hpp.audit.md#sr-aud-075--medium--failed-sequencereader-tryread-and-trypeek-retain-stale-output-instead-of-returning-default) | medium | remediated | `SequenceReader.hpp`, `Batch6BuffersTests.cpp` | False TryRead/TryPeek calls leave caller output unchanged; current .NET assigns default to its `out` value, avoiding stale-data reuse. |
9090
| [SR-AUD-076](modules/buffers/include/System/Buffers/ArrayPool.hpp.audit.md#sr-aud-076--medium--arraypoolcreate-configuration-is-silently-ignored-and-invalid-limits-are-accepted) | medium | remediated | `ArrayPool.hpp`, `ArrayPoolTests.cpp` | Configured Create discards both limits and accepts zero values, where .NET validates positive configuration and uses it to establish pool buckets. |
9191
| [SR-AUD-077](modules/buffers/include/System/Buffers/SearchValues.hpp.audit.md#sr-aud-077--medium--searchvaluest-documents-equality-only-support-but-imposes-an-undocumented-stdhash-requirement) | medium | remediated | `SearchValues.hpp`, `Batch16BuffersTests.cpp` | Public template documentation promises equality-only support, but unordered_set requires `std::hash<T>` and equality-only values fail to compile. **Remediated (#2054, 2026-08-04, family B-C, `docs/BuffersNamespaceReviewPlan.md` §4.4/§23.4):** the `@tparam` sentence that promised equality-only support is replaced by an explicit statement that this port requires equality **and** a usable `std::hash<T>` — strictly more than .NET's `IEquatable<T>` — with the workaround (supply a `std::hash<T>` specialization) named. Both constructors `static_assert` the two halves separately, so an equality-only `T` now fails with that sentence instead of `'std::__hash_enum<…>::~__hash_enum()' is private within this context`. The asserts are in the constructor bodies, not at class scope: `SearchValues<EqualityOnly>*` and `sizeof(SearchValues<EqualityOnly>)` compile today and still do. Two negative consumer sites (one per constructor) and the `EqualityOnly` predicate pins in `BuffersGenericRequirementsTests.cpp` hold both directions. Storing the values in a hash container remains this port's design; making the requirement true rather than the documentation is a separate, unraised question. |
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — a disposed pool owner throws, and `default` is not `Empty` (tickets #2056, #2057)
5+
6+
*2026-08-17.* Two `System::Buffers` types gain the state .NET has. Both are object-layout
7+
changes under `docs/StandingApprovals.md` SA-3, so **downstream consumers must be recompiled**.
8+
No source change is needed.
9+
10+
`modules/buffers` is header-only and **no module outside it** includes either type, so the
11+
repository-internal impact is nil.
12+
13+
---
14+
15+
## 1. #2056(a) — a disposed `IMemoryOwner` throws
16+
17+
| | Was | Is |
18+
|---|---|---|
19+
| `owner->getMemoryProperty()` after `Dispose()` | a **zero-length** `Memory` | `ObjectDisposedException` |
20+
| a live `Rent(0)` | a zero-length `Memory` | **unchanged** — and now distinguishable from a disposed owner |
21+
| `Dispose()` twice | idempotent | **unchanged** |
22+
| `sizeof(MemoryPoolHeapOwner_<int>)` | 32 | **40** |
23+
24+
.NET's `ArrayMemoryPoolBuffer.Memory` does exactly this
25+
(`ArrayMemoryPool.ArrayMemoryPoolBuffer.cs:18-25`).
26+
27+
**Why a flag and not .NET's own discriminator.** .NET needs no flag: it nulls `_array` and tests
28+
`array is null`. The port's natural equivalent is `std::unique_ptr<std::vector<T>>`, which would
29+
even make the object *smaller* — and it was rejected on purpose. `reset()` frees the storage
30+
**deterministically**, where `clear() + shrink_to_fit()` is non-binding. Half (b) of #2056
31+
below — is still open, and turning its latent use-after-free from "usually survives" into
32+
"always broken" while nothing yet fixes it would be a practical regression dressed as parity.
33+
The storage lifetime is therefore left exactly as it was, and a test says so.
34+
35+
**Half (b) is still open.** A `Memory<T>` obtained *before* `Dispose()` still keeps a pointer and
36+
a length over storage the owner may have released. `getMemoryProperty()` cannot defend against
37+
it — by the time the caller holds the `Memory`, the owner is no longer in the path — and
38+
repairing it is a `Memory<T>` ownership change in `Core.Base`. It stays pinned as unfixed rather
39+
than letting half a repair look whole.
40+
41+
## 2. #2057`default` enumerates nothing, `Empty` enumerates one segment
42+
43+
| | Was | Is |
44+
|---|---|---|
45+
| `ReadOnlySequence<T>{}` enumerated | **1** segment | **0** |
46+
| `ReadOnlySequence<T>::getEmpty()` enumerated | 1 segment | **1** |
47+
| length, `IsEmpty`, `Start`, `End`, `IsSingleSegment` for either || **unchanged, and identical to each other** |
48+
| any buffer-backed sequence, including `(nullptr, 0)` | 1 segment | **1** |
49+
| `sizeof(ReadOnlySequence<int>)` | 32 | **40** |
50+
51+
.NET's `Empty` is `new ReadOnlySequence<T>(Array.Empty<T>())` (`ReadOnlySequence.cs:26`) — an
52+
array-backed sequence with a non-null start object, so it yields one segment. `default` has a
53+
null start object and `Enumerator.MoveNext` returns false immediately for it (`:642-647`).
54+
55+
A `std::vector` cannot express that difference: `default` and `Empty` both hold an empty one,
56+
which is exactly why the review recorded that "distinguishing them needs state the type does not
57+
have". The new member is the port's `_startObject != null`.
58+
59+
Note the trap the tests cover: `(nullptr, 0)` is a **valid buffer-backed** sequence with no
60+
elements, and must still enumerate one segment. Only the default constructor yields none.
61+
62+
## 3. To migrate
63+
64+
* If you called `getMemoryProperty()` on a disposed owner expecting an empty `Memory`, catch
65+
`ObjectDisposedException` or stop calling it. .NET has always thrown there.
66+
* If you relied on a default-constructed `ReadOnlySequence<T>` yielding one empty segment, use
67+
`getEmpty()`, which still does.
68+
* Rebuild any consumer: both types grew.
69+
70+
## 4. Downstream, measured
71+
72+
Neither `cna` nor `mobile-eggbert` references `System::Buffers`**zero sites in both**. Neither
73+
repository was modified.

0 commit comments

Comments
 (0)