Skip to content

Commit 3803d14

Browse files
committed
fix(core): guard ArraySegment's enumeration doors (#2215, SR-AUD-054 residual)
A range-for over a DEFAULT ArraySegment silently performed zero iterations. .NET's GetEnumerator() throws InvalidOperationException (ArraySegment.cs:95-99). So does this port now, and begin()/end() are no longer noexcept. These were the last unguarded door in the type: #2214 guarded the other ten and could not touch these two, because the guard requires exactly this exception-specification change -- which is why the residual existed. First landing under SA-10. A default segment is not an empty segment: it has no array at all, so reporting zero elements makes the two indistinguishable and hands a caller who forgot to initialise a clean, plausible, wrong result. THE GUARD ASKS WHETHER THE ARRAY IS PRESENT, NEVER WHETHER THE COUNT IS ZERO -- an empty-but-real segment still iterates zero times, including one over an empty std::vector where data() may itself be null, and a test pins that. All five SA-2 conditions discharged: migration note; a four-site negative consumer fixture whose fourth site is the shape that breaks SILENTLY (a helper whose own noexcept is computed from the door), taking the set to 18 fixtures / 135 sites; downstream ticket #2365; the full gate at 17,283 run, 0 failed; and the measured impact -- zero ArraySegment sites in either consumer. Three mutations. Two caught; the third is caught as a CRASH rather than a failure, and inherently so -- it lets a default segment reach array_->data() through a null pointer, which is precisely the SEGV this finding recorded, and no test can assert on undefined behaviour.
1 parent 33eed3c commit 3803d14

7 files changed

Lines changed: 274 additions & 41 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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ implementation ticket.
6565
| [SR-AUD-051](modules/core/include/System/Array.hpp.audit.md#sr-aud-051--high--raw-pointer-arraycopy-applies-unchecked-memcpy-to-arbitrary-objects-and-negative-lengths) | high | remediated | `Array.hpp`, `Buffer.hpp` | Raw-pointer `Array::Copy` and generic Buffer typed-vector BlockCopy use unchecked byte copy for arbitrary types; ASan confirms invalid nontrivial destruction. Array additionally accepts negative signed metadata. |
6666
| [SR-AUD-052](modules/core/include/System/Array.hpp.audit.md#sr-aud-052--medium--array-delegate-overloads-do-not-validate-empty-stdfunction-at-the-public-boundary) | medium | remediated | `Array.hpp` | Empty `std::function` inputs yield `bad_function_call` only if reached, or a silent normal result for empty arrays, instead of a deterministic argument error. |
6767
| [SR-AUD-053](modules/core/include/System/Array.hpp.audit.md#sr-aud-053--low--arraymaxlengthproperty-reports-int32max-not-the-net-runtime-limit) | low | confirmed | `Array.hpp` | `MaxLengthProperty()` exposes `INT32_MAX`, 56 higher than current .NET `Array.MaxLength`, without a documented vector-adaptation decision. **Reviewed 2026-08-12 (#2327); STILL CONFIRMED, nothing implemented — `needs_user`.** Live: `Array.hpp:38` returns `std::numeric_limits<intcs>::max()` = 2,147,483,647 against .NET's `0x7FFFFFC7` = 2,147,483,591; the gap is 56. **Measured consumer surface: nothing uses it** — across `modules/`, `tests/`, `test/` and `bench/` the name occurs once outside its own definition, `ArrayTests.cpp:386` `EXPECT_GT(..., 0)`; no production call site, no `static_assert`, no array sizing, no allocation or indexing path, no serialized form. **Premise addition the finding does not state: the port already disagrees with itself.** `System::Buffers::MemoryPool<T>::MaxArrayLength` (`MemoryPool.hpp:42`) is a **public** `static constexpr intcs = 0x7FFFFFC7` documented "Matches .NET's Array.MaxLength (Array.cs)" and pinned to that literal by `MemoryPoolTests.MaxBufferSize_MatchesArrayMaxLength`; `ArrayBufferWriter<T>::MaxArrayLength` is the same value, private. The .NET number is already the repository's answer everywhere the limit is *enforced*; the only place publishing a different one is the property named after the .NET constant, where it is enforced nowhere. **Why it is still a decision:** the value is a public `constexpr` in a shipped header, so lowering it changes a downstream `constexpr` context's answer and makes a downstream `n > MaxLengthProperty()` guard newly reject 56 values — a surface this repository cannot measure (CNA/mobile-eggbert are outside its boundary), and the precedent for a public constant's value changing is `needs_user` (SR-AUD-130/#2326). The finding's own remediation is a disjunction, matching `CLAUDE.md` checklist item 5. **Deliberately not done:** pinning `== INT32_MAX`, which would encode the undecided branch as the tested contract. Options priced in #2327. `docs/CoreOwnedFindingsReviews2317.md`. |
68-
| [SR-AUD-054](modules/core/include/System/ArraySegment.hpp.audit.md#sr-aud-054--high--default-arraysegment-operations-silently-succeed-or-dereference-null-instead-of-throwing) | high | remediated | `ArraySegment.hpp` | Default-segment operations omit .NET's invalid-state guard; `ToArray` can silently succeed and `Slice(0)` reaches sanitizer-confirmed null dereference. **Residual (#2215, needs_user):** `begin()`/`end()`, the `GetEnumerator()` counterpart, still iterate a default segment zero times because guarding them needs an approved `noexcept` drop. |
68+
| [SR-AUD-054](modules/core/include/System/ArraySegment.hpp.audit.md#sr-aud-054--high--default-arraysegment-operations-silently-succeed-or-dereference-null-instead-of-throwing) | high | remediated | `ArraySegment.hpp` | Default-segment operations omit .NET's invalid-state guard; `ToArray` can silently succeed and `Slice(0)` reaches sanitizer-confirmed null dereference. **Residual (#2215, needs_user):** `begin()`/`end()`, the `GetEnumerator()` counterpart, still iterate a default segment zero times because guarding them needs an approved `noexcept` drop. **RESIDUAL NOW REMEDIATED TOO (#2215, 2026-08-18).** #2214 guarded ten of the type's array-touching doors and could not touch `begin()`/`end()`, because the guard requires dropping their `noexcept` — an exception-specification change, now covered by `docs/StandingApprovals.md` SA-10. All four enumeration doors are guarded, so a **default** segment raises `InvalidOperationException` where a range-`for` used to perform **zero iterations silently**, which is the same defect shape the other ten had: a clean, plausible, wrong result instead of a diagnostic. **The guard asks whether the array is present, never whether the count is zero** — an empty-but-real segment still iterates zero times, including one over an empty `std::vector` where `data()` may itself be null, and a test pins that distinction. SA-2's five conditions all discharged, including a four-site negative consumer fixture whose fourth site is the shape that breaks *silently* — a helper whose own `noexcept` is **computed** from the door. Three mutations: two caught, and the third **caught as a crash rather than a failure, inherently** — it lets a default segment reach `array_->data()` through a null pointer, which is the very SEGV this finding recorded, and no test can assert on undefined behaviour. Downstream measured: **zero** `ArraySegment` sites in either consumer (#2365). `docs/Migration-ArraySegmentEnumerationGuard.md`. |
6969
| [SR-AUD-055](modules/core/include/System/ArraySegment.hpp.audit.md) | medium | confirmed | `ArraySegment.hpp` | Vector `CopyTo` resizes an undersized destination rather than preserving ArraySegment's fixed-capacity destination/error contract. **Reviewed 2026-08-12 (#2328); STILL CONFIRMED — `needs_user`; one doc clause landed as #2329.** **Premise correction: four tests pin the resize, not one.** Besides `CopyTo_VectorWithOffset_ExpandsDest`, `ArraySegmentTests.CopyTo_Vector_CopiesAllElements` and `CopyTo_Vector_PartialSegment` both copy into an **empty** `dest` through the one-argument overload the finding treats only as "forwards to that behavior", and `CoreMemorySafetyOverlapTests.NonOverlappingCopiesKeepTheirPreviousResults` (`CoreMemorySafetyTests.cpp:837-838`) carries the comment `// SR-AUD-055's resize, unchanged` — ticket #2214 repaired this file's default-state and overlap defects and **deliberately preserved** the resize. **Premise addition: there is a repository convention and this is the one place that breaks it.** Of the **26** `void CopyTo(std::vector<...>&, ...)` overloads in `modules/`, **20 reject** a short destination — 13 with their own guard, five through the shared `System::Collections::detail::requireValidCopyDestination`, and `ImmutableList`'s two forwarding forms — and `ArraySegment`'s two-argument overload is the **only** one that grows a caller-sized destination. (`BitArray`'s two forms *replace* the destination outright: a third shape, a different .NET member, not this finding.) `modules/core` may not depend on `Collections`, so a repair writes its own guard rather than reusing the helper. **Why it is a decision:** the reject branch is a runtime behaviour break on currently accepted input — calls that succeed today start throwing and four first-party tests invert — the class this repository has always escalated. Both options priced in #2328. **#2329 (`done`, documentation only):** the one-argument overload's comment required "capacity for at least Count elements" and described writes "via `push_back`", both false and directly contradicted by the overload it forwards to; it now states what the code does and records that the resize-versus-reject question is open. `docs/CoreOwnedFindingsReviews2317.md`. |
7070
| [SR-AUD-056](modules/core/tests/System/InterfaceTests2.cpp.audit.md#sr-aud-056--medium--observable-fixture-discards-the-subscription-handle-and-permits-post-completion-delivery) | medium | remediated | `InterfaceTests2.cpp` | The direct observable fixture returns a null subscription and permits `OnNext` after completion, leaving required unsubscription and terminal-state behavior unasserted. **Remediated (#2301 review, #2302 implementation, 2026-08-11)** by all five of the report's own suggested assertions plus the fixture behaviour they require. This is a test-contract defect, so the fixture and its assertions are the whole of it and **no production file was touched**. Re-measured first: the production interfaces were the right oracle all along - `IObservable<T>::Subscribe` already returns `std::shared_ptr<IDisposable>` and documents what it is for, and `IObserver<T>` already documents that no `OnNext` or `OnCompleted` follows a terminal call; only the fixture contradicted them, and the finding's own note that no first-party `IObservable<T>` implementation exists still holds, so nothing shipped was wrong. `IntObservable2::Subscribe` now returns a real `Subscription` that unsubscribes exactly its own observer, rejects a null observer with `System::ArgumentNullException`, and is a no-op on a second `Dispose()`; the provider records a terminal state so `Emit`/`Complete`/`Fail` deliver nothing after one, and `Fail` supplies the `OnError` path the report notes was never covered. Two documented design choices: the observer list lives in a `State` the provider and every subscription **co-own** - the `SortedSet<T>`/#1786 pattern - so disposal is safe in any destruction order and there is **no ownership cycle**, answering the last "other missing assertions" bullet; and `~Subscription()` deliberately does **not** dispose, because unsubscription is tied to `Dispose()` rather than to the handle's lifetime - measured, since an RAII destructor made both pre-existing cases fail, both discarding the handle. **Seven cases added, none retired** (2 -> 9); the two pre-existing keep their names, with a non-null subscription assertion added to the first. Both defects the finding names were re-introduced as mutations, rebuilt and relinked: returning `nullptr` from `Subscribe` fails `Subscribe_ReturnsADisposableSubscription` **and** the pre-existing `Subscribe_AndReceiveValues`; removing the terminal guard from `Emit` fails `NoValueIsDeliveredAfterCompletion` **and** `OnError_IsTerminalToo` - so the report's "a future implementation can copy this fixture's wrong behavior without a failing regression" risk is closed in both directions. Deliberately **not** pinned: subscription ordering, which no interface documents. Still open and outside this finding: the `ParseableInt` and provider-`nullptr` bullets. `docs/CoreObservableFixtureContractPlan.md`. |
7171
| [SR-AUD-057](modules/core/include/System/Index.hpp.audit.md#sr-aud-057--high--unchecked-net-index-offset-semantics-use-signed-c-overflow) | high | remediated | `Index.hpp`, `Range.hpp` | The intentionally unvalidated index/range offset path executes signed C++ overflow for a maximal from-end Index and `INT_MIN` length rather than implementing .NET's defined unchecked arithmetic. |
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `ArraySegment<T>`'s enumeration doors reject a default segment (ticket #2215)
5+
6+
*2026-08-18.* A range-`for` over a **default** `ArraySegment<T>` silently performed zero
7+
iterations. .NET's `GetEnumerator()` throws `InvalidOperationException`. So does this port now,
8+
and `begin()`/`end()` are **no longer `noexcept`**.
9+
10+
Landed under `docs/StandingApprovals.md` **SA-10** with SA-2's five conditions. This is a public
11+
**signature** change — an exception specification — not a layout change. `sizeof` is unchanged.
12+
13+
---
14+
15+
## 1. What changed
16+
17+
| Call on a **default** segment | Was | Is |
18+
|---|---|---|
19+
| `seg.begin()`, `seg.end()` | `nullptr` | `InvalidOperationException`, *"The underlying array is null."* |
20+
| the `const` overloads | `nullptr` | the same |
21+
| `for (auto& x : seg)` | **zero iterations, silently** | `InvalidOperationException` |
22+
| `noexcept(seg.begin())` | `true` | **`false`** |
23+
| any **non-default** segment, including an empty one || **unchanged** |
24+
25+
`begin()` and `end()` are this port's counterpart of `GetEnumerator()`, and .NET's calls
26+
`ThrowInvalidOperationIfDefault()` first (`ArraySegment.cs:95-99`). They were the **last
27+
unguarded door** in the type: #2214 guarded the other ten and could not touch these two, because
28+
the guard requires exactly this exception-specification change.
29+
30+
## 2. Why silent emptiness was the wrong answer
31+
32+
A default segment is not an empty segment — it has no array at all. Reporting zero elements makes
33+
the two indistinguishable, so a caller who forgot to initialise a segment gets a clean, plausible,
34+
wrong result instead of a diagnostic. That is the same reasoning SR-AUD-054 applied to the ten
35+
doors #2214 fixed, where `ToArray`, all three `CopyTo` forms, `Contains` and `IndexOf` also
36+
"completed silently, reporting an empty result instead of an invalid state".
37+
38+
An **empty but real** segment still iterates zero times and still compares `begin() == end()`
39+
including one over an empty `std::vector`, where `data()` may itself be null. The guard asks
40+
whether the **array** is present, never whether the count is zero. A test pins that, and a
41+
mutation that confuses the two is caught.
42+
43+
## 3. To migrate
44+
45+
**Runtime code needs no change.** Every call that compiled before still compiles, and every
46+
non-default segment returns the same pointers. Only a default segment behaves differently, and
47+
only by reporting a fault it previously hid.
48+
49+
**Compile-domain code may need a change.** A `noexcept(...)` assertion on these doors, or a
50+
function whose own `noexcept` is *computed* from them, now sees `false`:
51+
52+
```cpp
53+
// before
54+
static_assert(noexcept(seg.begin()));
55+
template <class T> auto first(ArraySegment<T>& s) noexcept(noexcept(s.begin())) { ... }
56+
57+
// after: drop the assertion, or ask the question that is still noexcept
58+
if (seg.getArrayProperty() != nullptr) { /* safe to traverse */ }
59+
```
60+
61+
`getArrayProperty()`, `getOffsetProperty()` and `getCountProperty()` remain `noexcept` and are the
62+
intended way to ask whether a segment is usable without risking a throw. The negative consumer
63+
fixture `test/consumer/core_arraysegment_enumeration_negative.cpp` pins all four broken spellings
64+
and both surviving ones.
65+
66+
## 4. Downstream, measured
67+
68+
Per SA-2 condition 5: neither `cna` nor `mobile-eggbert` references `ArraySegment` at all —
69+
**zero sites in both**, and zero `noexcept(...)` assertions over any `begin()`/`end()`. Neither
70+
repository was modified. The downstream ticket is **#2365**.
71+
72+
## 5. Evidence
73+
74+
Three mutations. Two caught outright — removing the guard from `begin()`, and from the `const`
75+
`end()`. The third is **caught as a crash rather than a failure, and inherently so**: making the
76+
guard skip any segment whose count is zero lets a default segment reach `array_->data()` through a
77+
null pointer, which is precisely the SEGV SR-AUD-054 recorded. No test can assert on undefined
78+
behaviour, so the executable simply dies — which is detection, and is reported as such rather than
79+
counted as a clean catch.

modules/core/include/System/ArraySegment.hpp

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,11 @@ namespace System {
5050
* but `ArgumentOutOfRangeException` rather than .NET's `InvalidOperationException`,
5151
* because `count_ == 0` made the range check fire first.
5252
*
53-
* @note `begin()`/`end()` are the port's `GetEnumerator()` counterpart and are
54-
* **not** guarded, because guarding them requires dropping their `noexcept` -- an
55-
* exception-specification change this repository treats as approval-gated (ticket
56-
* #1854's precedent). That residual is ticket #2215, and the current zero-iteration
57-
* behaviour is pinned by a test that must be inverted when it ships.
53+
* @note `begin()`/`end()` ARE guarded since ticket #2215, which dropped their
54+
* `noexcept` under `docs/StandingApprovals.md` SA-10. They are the port's
55+
* `GetEnumerator()` counterpart and .NET's `GetEnumerator()` calls this same check
56+
* (`ArraySegment.cs:95-99`), so a range-`for` over a default segment now throws
57+
* instead of silently performing zero iterations.
5858
*
5959
* @throws System::InvalidOperationException if this is a default segment.
6060
* @see docs/CoreMemorySafetyFamilyPlan.md (family CMS-B)
@@ -196,22 +196,29 @@ namespace System {
196196
/**
197197
* @brief Returns a pointer to the first element of the segment.
198198
*
199-
* @warning **This is the port's `GetEnumerator()` counterpart, and unlike every other
200-
* array-touching member it does NOT reject a default segment** -- it returns
201-
* `nullptr`, so a range-`for` over a default segment performs zero iterations where
202-
* .NET's `GetEnumerator()` throws `InvalidOperationException`. Adding the guard means
203-
* dropping `noexcept` here and on `end()`, an exception-specification change this
204-
* repository treats as approval-gated (ticket #1854's precedent). Tracked as ticket
205-
* **#2215**; a test pins the current behaviour so that pin inverts the day #2215
206-
* ships. See docs/CoreMemorySafetyFamilyPlan.md §10.
199+
* This is the port's `GetEnumerator()` counterpart, and like every other
200+
* array-touching member it rejects a default segment. .NET's `GetEnumerator()` calls
201+
* `ThrowInvalidOperationIfDefault()` first (`ArraySegment.cs:95-99`).
202+
*
203+
* **These four are deliberately NOT `noexcept`**, and that is the whole of ticket
204+
* #2215: they used to be, which is why they were the last unguarded door in the type
205+
* and why a range-`for` over a default segment silently performed zero iterations.
206+
* The `noexcept` drop is a public signature change and landed under
207+
* `docs/StandingApprovals.md` SA-10 with SA-2's five conditions. See
208+
* docs/Migration-ArraySegmentEnumerationGuard.md.
209+
*
210+
* @throws System::InvalidOperationException if this is a default segment.
207211
*/
208-
T* begin() noexcept { return array_ ? array_->data() + offset_ : nullptr; }
209-
/** @brief Returns a pointer past the last element of the segment. */
210-
T* end() noexcept { return array_ ? array_->data() + offset_ + count_ : nullptr; }
211-
/** @brief Returns a const pointer to the first element of the segment. */
212-
const T* begin() const noexcept { return array_ ? array_->data() + offset_ : nullptr; }
213-
/** @brief Returns a const pointer past the last element of the segment. */
214-
const T* end() const noexcept { return array_ ? array_->data() + offset_ + count_ : nullptr; }
212+
T* begin() { throwIfDefault(); return array_->data() + offset_; }
213+
/** @brief Returns a pointer past the last element of the segment.
214+
* @throws System::InvalidOperationException if this is a default segment. */
215+
T* end() { throwIfDefault(); return array_->data() + offset_ + count_; }
216+
/** @brief Returns a const pointer to the first element of the segment.
217+
* @throws System::InvalidOperationException if this is a default segment. */
218+
const T* begin() const { throwIfDefault(); return array_->data() + offset_; }
219+
/** @brief Returns a const pointer past the last element of the segment.
220+
* @throws System::InvalidOperationException if this is a default segment. */
221+
const T* end() const { throwIfDefault(); return array_->data() + offset_ + count_; }
215222

216223
// -----------------------------------------------------------------------
217224
// Slice

0 commit comments

Comments
 (0)