Skip to content

Commit e2f9dc0

Browse files
committed
fix(core): raw Buffer::BlockCopy validates its signed metadata (#2212, SR-AUD-067)
A negative count was cast straight to size_t and a negative offset formed a pointer before its own buffer. All three arguments are now rejected with ArgumentOutOfRangeException before any pointer arithmetic, using the same helper, the same order and the same exception as the sibling std::vector overload already did, and the same order .NET checks them in before its internal Memmove. The documented upper-bound limitation is unchanged: a raw pointer still carries no capacity, and that is still the caller's problem. Two premise corrections. The finding is three doors, not one: the audit demonstrated count and asserted the offsets behave "similarly", but measured they are different classes -- srcOffset < 0 is a stack-buffer-underflow, dstOffset < 0 a stack-buffer-overflow. And the predicted negative-size-param is not what this toolchain reports for any of them, because GCC 13.3 at -O1 with _FORTIFY_SOURCE inlines __memmove_chk and bypasses the libsanitizer interceptor that emits it, so ASan's ordinary shadow check fires instead. All three reports are absent afterwards, each replaced by the right exception naming the right parameter, with the heap-buffer-overflow control still firing in the same instrumented binary. +8 permanent regressions, one of which is worth calling out: a negative offset is rejected even when count == 0. That is the case the old code silently got away with, because a zero-byte memmove never touches the bad pointer -- and it is what lets two of the mutations be killed by an assertion rather than by a crash. Five mutations, five killed. Four by assertion; the fifth, deleting all three guards at once, kills only by process abort, and is reported as the weaker signal it is rather than counted as if it were the same thing. No signature, member, layout, noexcept, vtable or symbol change -- Buffer is a stateless static-only class. SR-AUD-067 flips confirmed to remediated: 175 remediated / 134 confirmed / 55 design-complete of 364, numbering frozen at 364. The SR-AUD-051 extension in the same report stays open; #2213 owns it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014joExjgZv4aWFpzADeQGZm
1 parent a817762 commit e2f9dc0

5 files changed

Lines changed: 179 additions & 1 deletion

File tree

‎audit/AUDIT_FINDINGS_INDEX.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ implementation ticket.
7878
| [SR-AUD-064](modules/core/include/System/Lazy.hpp.audit.md#sr-aud-064--medium--lazy-constructors-silently-accept-an-invalid-thread-safety-mode) | medium | confirmed | `Lazy.hpp` | Mode-taking constructors retain invalid `LazyThreadSafetyMode` values and access dispatches them as PublicationOnly instead of throwing an argument-range error. |
7979
| [SR-AUD-065](modules/core/include/System/Lazy.hpp.audit.md#sr-aud-065--medium--lazy-accepts-an-empty-factory-and-defers-failure-to-stdbad_function_call) | medium | remediated | `Lazy.hpp` | Empty `std::function` factories are accepted then fail at first `Value` with native `std::bad_function_call`, rather than deterministic constructor validation. |
8080
| [SR-AUD-066](modules/core/include/System/Lazy.hpp.audit.md#sr-aud-066--medium--publicationonly-wrongly-rejects-recursive-value-access) | medium | confirmed | `Lazy.hpp` | An unconditional reentrancy guard throws for PublicationOnly even though .NET reserves this recursive-Value exception for None and ExecutionAndPublication. |
81-
| [SR-AUD-067](modules/core/include/System/Buffer.hpp.audit.md#sr-aud-067--high--raw-bufferblockcopy-converts-negative-metadata-into-an-unbounded-memmove) | high | confirmed | `Buffer.hpp` | Raw-pointer BlockCopy accepts negative offsets/count; negative count casts to `size_t` and reaches ASan-confirmed unbounded `memmove` instead of deterministic argument validation. |
81+
| [SR-AUD-067](modules/core/include/System/Buffer.hpp.audit.md#sr-aud-067--high--raw-bufferblockcopy-converts-negative-metadata-into-an-unbounded-memmove) | high | remediated | `Buffer.hpp` | Raw-pointer BlockCopy accepts negative offsets/count; negative count casts to `size_t` and reaches ASan-confirmed unbounded `memmove` instead of deterministic argument validation. |
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. |
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. |
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. |

‎audit/modules/core/include/System/Buffer.hpp.audit.md‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,65 @@ Checked primitive-vector paths are materially improved, but the raw public
6565
overload remains ASan-unsafe for negative metadata and generic vector templates
6666
permit nontrivial lifetime corruption. No source or test was modified during
6767
this audit.
68+
69+
---
70+
71+
## SR-AUD-067 — REMEDIATED (ticket #2212, 2026-08-10, family CMS-A)
72+
73+
The original evidence above is retained unchanged. **Only SR-AUD-067 is closed by this
74+
ticket**; the SR-AUD-051 extension recorded in this same report stays `confirmed` and is
75+
closed separately by ticket #2213. **No `SR-AUD-*` identifier was created**; numbering
76+
stays frozen at 364. Family record: `docs/CoreMemorySafetyFamilyPlan.md`.
77+
78+
The raw-pointer `BlockCopy` now runs
79+
`ArgumentOutOfRangeException::ThrowIfNegative` on `srcOffset`, then `dstOffset`, then
80+
`count`, **before any pointer arithmetic** — the same helper, the same order and the same
81+
exception as the sibling `std::vector` overload's `requireValidBlockCopyRange`, and the
82+
same order .NET's `Buffer.BlockCopy` uses before its internal `Memmove`. The upper-bound
83+
limitation the header documents is unchanged: a raw pointer still carries no capacity, and
84+
that is still the caller's responsibility.
85+
86+
**Premise correction — the finding is three doors, not one, and the defect class is not
87+
the predicted one.** The audit demonstrated `count = -1` and asserted that negative offsets
88+
"similarly form invalid pointers". Measured under AddressSanitizer on 2026-08-10
89+
(`build-probe/2210_before.log`), all three are separately reproducible and they are **not
90+
the same class**:
91+
92+
| Argument | Before | After |
93+
|---|---|---|
94+
| `count = -1` | **stack-buffer-overflow** in `memmove` (`Buffer.hpp:59`) | `ArgumentOutOfRangeException` `(Parameter 'count')` |
95+
| `srcOffset = -4` | **stack-buffer-underflow** in `memmove` | `ArgumentOutOfRangeException` `(Parameter 'srcOffset')` |
96+
| `dstOffset = -4` | **stack-buffer-overflow** in `memmove` | `ArgumentOutOfRangeException` `(Parameter 'dstOffset')` |
97+
98+
The audit predicted `negative-size-param`. It is not what this toolchain reports: GCC 13.3
99+
at `-O1` with `_FORTIFY_SOURCE` inlines `__memmove_chk`, so libsanitizer's `memmove`
100+
interceptor — the code that emits `negative-size-param` — is bypassed and ASan's ordinary
101+
shadow check fires instead. The compiler additionally emits `-Wstringop-overflow=` and
102+
`-Wstringop-overread` naming `Buffer.hpp:59` for all three. After-log:
103+
`build-probe/2212_after.log`; the deliberate `heap-buffer-overflow` control still reports
104+
in the same binary, so the absence is an absence and not a dead instrumentation.
105+
106+
**Closure evidence.** 8 permanent regressions in
107+
`modules/core/tests/System/CoreMemorySafetyTests.cpp`: each of the three arguments rejected;
108+
the exact `paramName` **and** the srcOffset → dstOffset → count order; a negative offset
109+
rejected **even when `count == 0`** (the case the old code silently got away with, because
110+
`memmove` of zero bytes never touched the bad pointer); `INTCS_MIN`; no destination byte
111+
written by a rejected call; every valid copy, `count == 0`, and the overlapping
112+
`memmove` unchanged; and catchability as `System::Exception`.
113+
`SharpRuntimeTests_Core_Base` **5,600/5,600** (1 pre-existing skip); `BufferTests` and
114+
`Batch13BufferTests` unchanged and green. Whole repository builds with zero errors and zero
115+
warnings.
116+
117+
**Five mutations, five killed — four of them by assertion.** M1 delete the `srcOffset`
118+
guard → 3 fail. M2 delete the `dstOffset` guard → 3 fail. M3 reorder the three guards →
119+
the order test fails. M4 `ThrowIfNegative(count - 1)` (over-rejects a legal `count == 0`)
120+
→ the valid-call test fails. **M0, deleting all three guards at once, is reported
121+
separately and honestly: it kills by process abort, not by assertion**, because a negative
122+
`count` reaches `memmove` and crashes the runner — that abort *is* the pre-fix defect, so
123+
it is real but it is a weaker signal, which is exactly why the zero-count rejection tests
124+
were added to give M1 and M2 an assertion-level kill.
125+
126+
**Source, ABI and layout consequences: none.** Three statements added to one inline static
127+
member of a stateless class (`Buffer() = delete`, no data members). No signature, no
128+
`noexcept` specification, no virtual function, no default argument and no mangled symbol
129+
changed.

‎modules/core/include/System/Buffer.hpp‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,32 @@ namespace System {
4848
* length-aware System::Collections::ObjectSpan destination, so it is no longer an
4949
* example of an accepted limitation.
5050
*
51+
* @note The capacity of a raw buffer cannot be discovered, but that does not make
52+
* every argument unverifiable. A negative @p count used to be cast straight to
53+
* @c size_t and a negative offset used to form a pointer *before* its own buffer;
54+
* all three were AddressSanitizer-confirmed out-of-bounds `memmove` operations
55+
* (SR-AUD-067: `count < 0` a **stack-buffer-overflow**, `srcOffset < 0` a
56+
* **stack-buffer-underflow**, `dstOffset < 0` a **stack-buffer-overflow**). Each
57+
* is now rejected deterministically before any pointer arithmetic, in the same
58+
* order and with the same exception as the sibling `std::vector` overload's
59+
* `requireValidBlockCopyRange`, matching .NET's `Buffer.BlockCopy`, which checks
60+
* all three inputs before its internal `Memmove`. The upper-bound limitation is
61+
* unchanged and remains the caller's responsibility. See
62+
* `docs/CoreMemorySafetyFamilyPlan.md` (CMS-A).
63+
*
5164
* @param src Pointer to the source region.
5265
* @param srcOffset Byte offset into @p src.
5366
* @param dst Pointer to the destination region.
5467
* @param dstOffset Byte offset into @p dst.
5568
* @param count Number of bytes to copy.
69+
* @throws System::ArgumentOutOfRangeException if @p srcOffset, @p dstOffset, or
70+
* @p count is negative.
5671
*/
5772
static void BlockCopy(const void* src, intcs srcOffset,
5873
void* dst, intcs dstOffset, intcs count) {
74+
System::ArgumentOutOfRangeException::ThrowIfNegative(srcOffset, "srcOffset");
75+
System::ArgumentOutOfRangeException::ThrowIfNegative(dstOffset, "dstOffset");
76+
System::ArgumentOutOfRangeException::ThrowIfNegative(count, "count");
5977
std::memmove(static_cast<bytecs*>(dst) + dstOffset,
6078
static_cast<const bytecs*>(src) + srcOffset,
6179
static_cast<size_t>(count));

‎modules/core/tests/System/CoreMemorySafetyTests.cpp‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@
1111
#include <string>
1212
#include <vector>
1313

14+
#include "System/ArgumentOutOfRangeException.hpp"
15+
#include "System/Buffer.hpp"
16+
#include "System/Exception.hpp"
1417
#include "System/SpanSplitEnumerator.hpp"
1518

1619
using SharpRuntime::intcs;
@@ -147,3 +150,98 @@ TEST(CoreMemorySafetySplitTests, EmptySequenceOverNonTrivialElements) {
147150
EXPECT_EQ(e.getCurrentSpan().getLengthProperty(), 2);
148151
EXPECT_FALSE(e.MoveNext());
149152
}
153+
154+
// ===========================================================================
155+
// SR-AUD-067 (CMS-A) -- raw Buffer::BlockCopy validates its signed metadata
156+
// ===========================================================================
157+
158+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_NegativeCountIsRejected) {
159+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
160+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
161+
EXPECT_THROW(System::Buffer::BlockCopy(src, 0, dst, 0, -1),
162+
System::ArgumentOutOfRangeException);
163+
// No write happened: rejection precedes every byte of the memmove.
164+
EXPECT_EQ(dst[0], 0);
165+
EXPECT_EQ(dst[3], 0);
166+
}
167+
168+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_NegativeSrcOffsetIsRejected) {
169+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
170+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
171+
EXPECT_THROW(System::Buffer::BlockCopy(src, -4, dst, 0, 4),
172+
System::ArgumentOutOfRangeException);
173+
EXPECT_EQ(dst[0], 0);
174+
}
175+
176+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_NegativeDstOffsetIsRejected) {
177+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
178+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
179+
EXPECT_THROW(System::Buffer::BlockCopy(src, 0, dst, -4, 4),
180+
System::ArgumentOutOfRangeException);
181+
EXPECT_EQ(dst[0], 0);
182+
}
183+
184+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_ParamNamesAndOrderMatchTheVectorOverload) {
185+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
186+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
187+
// srcOffset is reported before dstOffset, which is reported before count --
188+
// the same order requireValidBlockCopyRange uses for the checked vector overload.
189+
auto paramOf = [&](intcs so, intcs dof, intcs n) {
190+
try { System::Buffer::BlockCopy(src, so, dst, dof, n); }
191+
catch (const System::ArgumentOutOfRangeException& e) { return std::string(e.getParamNameProperty()); }
192+
return std::string("<no throw>");
193+
};
194+
EXPECT_EQ(paramOf(-1, -1, -1), "srcOffset");
195+
EXPECT_EQ(paramOf(0, -1, -1), "dstOffset");
196+
EXPECT_EQ(paramOf(0, 0, -1), "count");
197+
}
198+
199+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_ValidCallsAreUnchanged) {
200+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
201+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
202+
System::Buffer::BlockCopy(src, 0, dst, 0, 4);
203+
EXPECT_EQ(dst[0], 1);
204+
EXPECT_EQ(dst[3], 4);
205+
206+
SharpRuntime::bytecs dst2[4] = {9, 9, 9, 9};
207+
System::Buffer::BlockCopy(src, 1, dst2, 2, 2);
208+
EXPECT_EQ(dst2[0], 9);
209+
EXPECT_EQ(dst2[2], 2);
210+
EXPECT_EQ(dst2[3], 3);
211+
212+
// count == 0 is legal and must stay legal.
213+
SharpRuntime::bytecs dst3[4] = {5, 5, 5, 5};
214+
System::Buffer::BlockCopy(src, 0, dst3, 0, 0);
215+
EXPECT_EQ(dst3[0], 5);
216+
}
217+
218+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_OverlappingRegionsStillMove) {
219+
// memmove semantics were already correct here and must remain so.
220+
SharpRuntime::bytecs buf[5] = {1, 2, 3, 4, 5};
221+
System::Buffer::BlockCopy(buf, 0, buf, 1, 4);
222+
EXPECT_EQ(buf[0], 1);
223+
EXPECT_EQ(buf[1], 1);
224+
EXPECT_EQ(buf[2], 2);
225+
EXPECT_EQ(buf[3], 3);
226+
EXPECT_EQ(buf[4], 4);
227+
}
228+
229+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_RejectionIsCatchableAsSystemException) {
230+
SharpRuntime::bytecs src[1] = {1};
231+
SharpRuntime::bytecs dst[1] = {0};
232+
EXPECT_THROW(System::Buffer::BlockCopy(src, 0, dst, 0, -1), System::Exception);
233+
}
234+
235+
TEST(CoreMemorySafetyRawBufferTests, RawBlockCopy_NegativeOffsetIsRejectedEvenWithZeroCount) {
236+
// The case the old code "got away with": a zero count means memmove copies nothing,
237+
// so a negative offset produced no observable failure while still forming a pointer
238+
// before its own buffer. It is invalid metadata either way and is rejected either way.
239+
SharpRuntime::bytecs src[4] = {1, 2, 3, 4};
240+
SharpRuntime::bytecs dst[4] = {0, 0, 0, 0};
241+
EXPECT_THROW(System::Buffer::BlockCopy(src, -4, dst, 0, 0),
242+
System::ArgumentOutOfRangeException);
243+
EXPECT_THROW(System::Buffer::BlockCopy(src, 0, dst, -4, 0),
244+
System::ArgumentOutOfRangeException);
245+
EXPECT_THROW(System::Buffer::BlockCopy(src, SharpRuntime::INTCS_MIN, dst, 0, 0),
246+
System::ArgumentOutOfRangeException);
247+
}

‎plan.sqlite3‎

4 KB
Binary file not shown.

0 commit comments

Comments
 (0)