Skip to content

Commit 3b86d95

Browse files
committed
fix(buffers): make MemoryHandle's representation private, and decline the destructor (#2059, SR-AUD-088)
Rule-14 sweep: this ticket's recorded gate was "/rv/tmp/runtime/src/libraries/ RE-VERIFIED ABSENT 2026-08-04, so reference behaviour comes only from repository-contained evidence". /rv is present, and it resolves the ticket -- against the ticket's own proposed repair. PREMISE CORRECTED. .NET's type is `public unsafe struct MemoryHandle : IDisposable` (MemoryHandle.cs:12), a struct with no finalizer, so scope exit does not unpin there either. `using var h = memory.Pin();` is a language construct that calls Dispose(), not something the type does. SR-AUD-088's real content was a DOC-COMMENT promising "or let the destructor do it" -- a promise the type never kept, and which an earlier ticket had already removed. So `~MemoryHandle(){ Dispose(); }` is DECLINED rather than deferred: adding it would be a divergence, not a repair. The ticket's own copy hazard is the second, independent reason and it stands -- this is a copyable handle, so an unpinning destructor would unpin once per copy for a single pin. Dispose() needed no change; it already matches MemoryHandle.cs:41-53 statement for statement, idempotence included. What DID land is the divergence the ticket never named. .NET's three fields are all private and it publishes exactly one, Pointer, as a getter. This port published both of its two as mutable data members, so a caller could retarget a live handle, or detach its IPinnable -- which makes the subsequent Dispose() a silent no-op that leaks the pin. Both are now private, under SA-8, with SA-2's five conditions discharged. An access change, not a layout change: sizeof(MemoryHandle) is 24 before and after, so no consumer rebuilds -- the same shape as #2332's SequencePosition. Zero first-party migration sites, measured. .NET's third field, GCHandle _handle, stays deliberately absent: no moving collector, no handle to free. The pin comments are corrected rather than merely extended: ScopeExitDoesNotUnpin said ".NET-style RAII would unpin here. It does not.", which states a falsehood about .NET, and now asserts parity. Five mutations, all caught, two at compile time through absence pins that use a dependent parameter (the #2299 gcc trap). Fixture set 37/200 -> 38/204. Downstream measured: 0 sites in cna, 0 in mobile-eggbert. Gate: 17,421 run, 17,421 passed, 0 failed, 0 skipped across 38 executables (+1 on 17,420; SharpRuntimeTests_Buffers 629 -> 630; no other executable moved). Module graph unchanged at 41/93.
1 parent 839d2b3 commit 3b86d95

6 files changed

Lines changed: 316 additions & 22 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `MemoryHandle`'s representation is private, and the destructor is declined (ticket #2059, SR-AUD-088)
5+
6+
*2026-08-19.* `System::Buffers::MemoryHandle`'s two data members are now `private`, matching
7+
.NET. The RAII destructor the ticket proposed was measured against the reference and **declined**.
8+
9+
Landed under `docs/StandingApprovals.md` **SA-8** (public representation where .NET's is private
10+
→ match .NET and migrate the first-party sites), with SA-2's five conditions discharged for the
11+
source break. **No layout change**`sizeof(MemoryHandle)` is 24 before and after — so no
12+
consumer needs a rebuild.
13+
14+
---
15+
16+
## 1. The finding's premise does not survive the reference
17+
18+
SR-AUD-088 is titled *"MemoryHandle documents RAII cleanup but never unpins at scope exit"*, and
19+
#2059 was opened to add `~MemoryHandle(){ Dispose(); }`.
20+
21+
.NET's type is:
22+
23+
```csharp
24+
public unsafe struct MemoryHandle : IDisposable // MemoryHandle.cs:12
25+
{
26+
private void* _pointer;
27+
private GCHandle _handle;
28+
private IPinnable? _pinnable;
29+
...
30+
}
31+
```
32+
33+
A `struct` with **no finalizer**. Scope exit does not unpin in .NET either. `using var handle =
34+
memory.Pin();` is a *language* construct that calls `Dispose()`; it is not something the type does
35+
for you.
36+
37+
So what SR-AUD-088 actually found was a **doc-comment that promised something the type never
38+
did***"should call Dispose() explicitly (or let the destructor do it)"*. That promise was the
39+
defect, and an earlier ticket had already removed it. The behaviour it described was never wrong.
40+
41+
**Adding the destructor would therefore be a divergence from .NET, not a repair**, and #2059
42+
declines it. The ticket's own hazard analysis is the second, independent reason and it stands:
43+
this is a copyable handle, so an unpinning destructor would unpin **once per copy** for a single
44+
pin.
45+
46+
`Dispose()` needed no change either — it already matches `MemoryHandle.cs:41-53` statement for
47+
statement: unpin, clear the `IPinnable`, null the pointer, so a second call does nothing.
48+
49+
## 2. What did change
50+
51+
The divergence the ticket never named. .NET's three fields are all `private` and it publishes
52+
exactly one of them, `Pointer`, as a getter. This port published **both** of its two as mutable
53+
data members.
54+
55+
| | Was | Is |
56+
|---|---|---|
57+
| `pointer_` | public, mutable | **private** |
58+
| `pinnable_` | public, mutable | **private** |
59+
| `getPointerProperty()` | public getter | unchanged |
60+
| both constructors | public | unchanged |
61+
| `Dispose()` | public, idempotent | unchanged |
62+
| copyability | copyable | unchanged |
63+
| `sizeof(MemoryHandle)` | **24** | **24** |
64+
| a destructor | absent | **still absent, now for a stated reason** |
65+
66+
.NET's third field, `private GCHandle _handle`, stays deliberately absent here: this runtime has
67+
no moving collector, so there is no GC handle to free.
68+
69+
## 3. Why it matters
70+
71+
`pinnable_` is the dangerous one, and it fails **silently**:
72+
73+
```cpp
74+
MemoryHandle handle = buffer.Pin(0);
75+
handle.pinnable_ = nullptr; // used to compile
76+
handle.Dispose(); // now a no-op -- the pin leaks, with no diagnostic anywhere
77+
```
78+
79+
`pointer_` is the same shape one level down: a caller could retarget a live handle at an
80+
unrelated address and then dispose a handle whose pointer no longer described what was pinned.
81+
82+
## 4. To migrate
83+
84+
| Was | Now |
85+
|---|---|
86+
| `h.pointer_` (read) | `h.getPointerProperty()` |
87+
| `h.pointer_ = p` | construct a new handle: `h = MemoryHandle(p, pinnable)` |
88+
| `h.pinnable_` (read or write) | **no replacement, by design** — .NET exposes no accessor either; to release the pin, call `h.Dispose()` |
89+
90+
**Zero first-party sites needed migrating** — measured across all of `modules/` and `test/`, there
91+
were no direct accesses to either member outside the type itself. The five construction sites all
92+
go through the public constructors and are untouched.
93+
94+
## 5. Evidence
95+
96+
Five mutations, **all caught**:
97+
98+
| Mutation | Caught by |
99+
|---|---|
100+
| M1 — both members public again | `MemoryHandlePinTests.TheRepresentationIsPrivateAsInDotNet` (compile time) |
101+
| M2 — only `pinnable_` public again | the same test's second `static_assert` (compile time) |
102+
| M3 — add the destructor the ticket proposed | `MemoryHandlePinTests.ScopeExitDoesNotUnpin` |
103+
| M4 — `Dispose()` stops clearing `pinnable_` | `MemoryHandlePinTests.ExplicitDisposeUnpinsExactlyOnce` |
104+
| M5 — `Dispose()` leaves a dangling pointer | that test **and** `MemoryHandleTests.Dispose_ClearsPointer` |
105+
106+
The absence pins use a **dependent** parameter, because gcc evaluates a non-dependent `requires`
107+
eagerly and hard-errors on the access instead of yielding `false` — the #2299 trap.
108+
109+
Negative consumer fixture: `test/consumer/buffers_memoryhandle_private_negative.cpp`, four sites,
110+
all rejected. The fixture set grows to **38 fixtures / 204 sites**. Its site 3 is the detach that
111+
breaks silently rather than loudly.
112+
113+
## 6. Downstream, measured
114+
115+
Per SA-2 condition 5: `MemoryHandle` appears in **zero** places in `cna` and **zero** in
116+
`mobile-eggbert`. Neither repository was modified, and no downstream ticket is needed.

modules/buffers/tests/System/Buffers/BuffersContractPinTests.cpp

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,10 @@ static_assert(sizeof(ArrayBufferWriter<char>) == 40,
6767
static_assert(sizeof(StandardFormat) == 2,
6868
"#2052 must not have added state");
6969
static_assert(sizeof(MemoryHandle) == 24,
70-
"#2059's move-only or refcounted semantics would change this");
70+
"#2059 made the two members PRIVATE, which is an access change and not a layout "
71+
"change -- 24 before and 24 after, so no consumer rebuilds. A destructor or "
72+
"refcounted semantics would change this, and #2059 measured the reference and "
73+
"declined both.");
7174

7275
TEST(BuffersLayoutPinTests, LayoutsAreStaticallyAsserted) {
7376
SUCCEED() << "The static_asserts above are the test; this keeps it visible in the suite.";
@@ -244,7 +247,22 @@ TEST(ReadOnlySequenceSegmentPinTests, EverySequenceReportsASingleSegment) {
244247
}
245248

246249
// ===========================================================================
247-
// SR-AUD-088 — MemoryHandle performs no RAII cleanup (blocked #2059)
250+
// SR-AUD-088 — MemoryHandle performs no RAII cleanup (#2059 RESOLVED).
251+
//
252+
// THE FINDING'S PREMISE DOES NOT SURVIVE THE REFERENCE, and the pins below used to
253+
// restate it. .NET's MemoryHandle is `public unsafe struct MemoryHandle : IDisposable`
254+
// (MemoryHandle.cs:12) -- a value type with no finalizer -- so scope exit does not unpin
255+
// THERE EITHER. `using var h = memory.Pin();` is a language construct that calls
256+
// Dispose(); it is not something the type does.
257+
//
258+
// What SR-AUD-088 actually found was a DOC-COMMENT that promised "or let the destructor
259+
// do it". That promise was the defect and it is gone. The behaviour it described was
260+
// never wrong.
261+
//
262+
// #2059 therefore declines the destructor rather than deferring it: adding one would be
263+
// a divergence from .NET, and the copy hazard below is the second, independent reason.
264+
// What #2059 DID land is the divergence the ticket never named -- the two data members
265+
// were public here and are private in .NET -- under SA-8, at zero migration sites.
248266
// ===========================================================================
249267

250268
namespace {
@@ -262,9 +280,11 @@ TEST(MemoryHandlePinTests, ScopeExitDoesNotUnpin) {
262280
{
263281
MemoryHandle handle = p.Pin(0);
264282
EXPECT_EQ(handle.getPointerProperty(), &p.value);
265-
} // .NET-style RAII would unpin here. It does not. #2059.
283+
} // No unpin here -- and .NET does not unpin here either (MemoryHandle.cs:12, a
284+
// struct with no finalizer). This asserts PARITY, not a known gap.
266285
EXPECT_EQ(p.unpinCount, 0)
267-
<< "if this becomes 1, MemoryHandle has grown a destructor -- #2059 has landed";
286+
<< "if this becomes 1, MemoryHandle has grown a destructor and now diverges from "
287+
".NET -- #2059 measured that and declined it";
268288
}
269289

270290
TEST(MemoryHandlePinTests, ExplicitDisposeUnpinsExactlyOnce) {
@@ -278,8 +298,9 @@ TEST(MemoryHandlePinTests, ExplicitDisposeUnpinsExactlyOnce) {
278298
}
279299

280300
TEST(MemoryHandlePinTests, ACopyStillReferencesTheSamePinnable) {
281-
// This is why #2059 cannot simply add a destructor: an unpinning destructor on a
282-
// freely copyable aggregate would unpin once per copy.
301+
// The SECOND reason #2059 declined the destructor, independent of the reference: an
302+
// unpinning destructor on a freely copyable handle would unpin once per copy for a
303+
// single pin. .NET has the same copy semantics and the same absence of a destructor.
283304
CountingPinnable p;
284305
MemoryHandle original = p.Pin(0);
285306
MemoryHandle copy = original;
@@ -288,6 +309,39 @@ TEST(MemoryHandlePinTests, ACopyStillReferencesTheSamePinnable) {
288309
EXPECT_EQ(p.unpinCount, 2) << "two handles, two Unpins -- one pin";
289310
}
290311

312+
namespace {
313+
/** True iff `T::pointer_` is reachable from outside the type. */
314+
template <typename T>
315+
concept HasPublicPointerMember = requires(T h) { h.pointer_; };
316+
template <typename T>
317+
concept HasPublicPinnableMember = requires(T h) { h.pinnable_; };
318+
}
319+
320+
TEST(MemoryHandlePinTests, TheRepresentationIsPrivateAsInDotNet) {
321+
// .NET publishes only `Pointer`, as a getter (MemoryHandle.cs:35). Both fields are
322+
// private there and are now private here.
323+
//
324+
// The parameter is DEPENDENT on purpose: gcc evaluates a non-dependent `requires`
325+
// eagerly and hard-errors on the access instead of yielding false (the #2299 trap).
326+
static_assert(!HasPublicPointerMember<MemoryHandle>,
327+
"#2059: pointer_ is private -- read it with getPointerProperty()");
328+
static_assert(!HasPublicPinnableMember<MemoryHandle>,
329+
"#2059: pinnable_ is private, and .NET publishes no accessor for it at all");
330+
331+
// What did NOT change, asserted so the pin proves the boundary is an access change and
332+
// nothing else: the two public constructors, the getter, copyability and the size.
333+
CountingPinnable p;
334+
MemoryHandle handle(&p.value, &p);
335+
EXPECT_EQ(handle.getPointerProperty(), &p.value);
336+
MemoryHandle copied = handle;
337+
EXPECT_EQ(copied.getPointerProperty(), &p.value);
338+
EXPECT_EQ(MemoryHandle().getPointerProperty(), nullptr);
339+
static_assert(std::is_copy_constructible_v<MemoryHandle>, "still copyable");
340+
static_assert(sizeof(MemoryHandle) == 24, "an access change moves no layout");
341+
handle.Dispose();
342+
copied.Dispose();
343+
}
344+
291345
// ===========================================================================
292346
// SR-AUD-086 — the leading '+' asymmetry (#2060 RESOLVED).
293347
//

modules/core/include/System/Buffers/MemoryHandle.hpp

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,24 +16,26 @@ struct IPinnable;
1616
* Wraps a raw pointer to pinned memory and optionally holds a reference to an
1717
* IPinnable that must be unpinned on disposal.
1818
*
19-
* @warning **This type performs no RAII cleanup: the caller MUST call Dispose()
20-
* explicitly.** There is no destructor that calls Dispose, and the inherited
21-
* `~IDisposable` does nothing, so letting a MemoryHandle go out of scope leaves its
22-
* IPinnable pinned. The header previously claimed the destructor would do it; it
23-
* never did (SR-AUD-088).
19+
* @note **The caller must call Dispose() explicitly. Scope exit does not unpin — and
20+
* that MATCHES .NET, which is why no destructor is added here.** .NET's
21+
* `MemoryHandle` is a `public unsafe struct` with no finalizer
22+
* (`MemoryHandle.cs:12`), so letting one go out of scope leaves its `IPinnable`
23+
* pinned there too. `using var handle = memory.Pin();` is a *language* construct that
24+
* calls `Dispose()`; it is not something the type does for you.
2425
*
25-
* Adding such a destructor is **not** a small correction and is deliberately not made
26-
* here: this is a copyable aggregate with public members, so an unpinning destructor
27-
* would make every copy unpin the same IPinnable more than once. A correct repair needs
28-
* move-only or reference-counted semantics on a type that `Memory.hpp` and
29-
* `ReadOnlyMemory.hpp` include, i.e. reaching all of `Core.Base`. That is blocked ticket
30-
* **#2059** (CCF-019); see docs/BuffersNamespaceReviewPlan.md §4.11. The behaviour
31-
* documented above is pinned by a permanent test, so it cannot change silently.
26+
* SR-AUD-088 reported that this header *promised* RAII cleanup — "should call
27+
* Dispose() explicitly (or let the destructor do it)" — that it never performed. That
28+
* promise was the defect and it has been removed. Ticket **#2059** then measured the
29+
* proposed repair against the reference and **declined it**: adding
30+
* `~MemoryHandle(){ Dispose(); }` would be a divergence from .NET rather than a repair,
31+
* and the hazard is real as well as theoretical — this is a copyable handle, so an
32+
* unpinning destructor would unpin once per copy for a single pin. The absence is pinned
33+
* by `MemoryHandlePinTests`.
34+
*
35+
* `Dispose()` is idempotent, exactly as .NET's is (`MemoryHandle.cs:41-53`): it unpins,
36+
* clears the `IPinnable` and nulls the pointer, so a second call does nothing.
3237
*/
3338
struct MemoryHandle : System::IDisposable {
34-
void* pointer_ = nullptr;
35-
IPinnable* pinnable_ = nullptr;
36-
3739
/** @brief Constructs a default (null) MemoryHandle. */
3840
MemoryHandle() = default;
3941

@@ -57,6 +59,17 @@ struct MemoryHandle : System::IDisposable {
5759
* Defined after IPinnable is complete (see IPinnable.hpp).
5860
*/
5961
void Dispose() override;
62+
63+
private:
64+
// PRIVATE, matching .NET's `private void* _pointer` / `private IPinnable? _pinnable`
65+
// (MemoryHandle.cs:14-16). This port published both, so a caller could retarget a live
66+
// handle at an unrelated address, or detach its IPinnable and leak the pin, behind the
67+
// owner's back. .NET publishes only `Pointer`, and only as a getter.
68+
//
69+
// .NET's third field, `private GCHandle _handle`, is deliberately absent: this runtime
70+
// has no moving collector, so there is no GC handle to free.
71+
void* pointer_ = nullptr;
72+
IPinnable* pinnable_ = nullptr;
6073
};
6174

6275
} // namespace System::Buffers

plan.sqlite3

0 Bytes
Binary file not shown.
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
// SPDX-License-Identifier: MIT
2+
// Copyright (c) Robert Vokac and contributors
3+
//
4+
// Negative compile fixture for ticket #2059 (SR-AUD-088, cause B-D).
5+
//
6+
// #2059 made System::Buffers::MemoryHandle's two components private, matching .NET's
7+
// `private void* _pointer` / `private IPinnable? _pinnable` (MemoryHandle.cs:14-16).
8+
// .NET publishes exactly one of them, `Pointer`, and only as a getter. This port
9+
// published both as mutable data members, so a caller could retarget a live handle at
10+
// an unrelated address, or detach its IPinnable and leak the pin, behind the owner's
11+
// back -- and could then call Dispose() on a handle whose pointer no longer matched
12+
// what was actually pinned.
13+
//
14+
// Nothing in this repository ever touched them -- measured, ZERO first-party direct
15+
// accesses -- so there was no first-party migration. What a CONSUMER loses is the
16+
// spellings that reach a public data member. Each is compiled on its own below; the
17+
// #else branches are the migrated spellings.
18+
//
19+
// Migration: build handles with the two-argument constructor (or take one from
20+
// IPinnable::Pin) and read the address with getPointerProperty(). There is no
21+
// replacement for reading or writing pinnable_, deliberately: .NET exposes no such
22+
// accessor, and a caller that needs to release the pin calls Dispose().
23+
//
24+
// NOTE what this fixture does NOT claim. #2059 also DECLINED to add
25+
// `~MemoryHandle(){ Dispose(); }`, because .NET's MemoryHandle is a struct with no
26+
// finalizer and does not unpin at scope exit either. That absence is parity, not a gap,
27+
// and it is pinned inside the repository by MemoryHandlePinTests rather than here --
28+
// a fixture can only prove that a spelling is rejected, not that a behaviour is absent.
29+
//
30+
// Records: docs/Migration-MemoryHandlePrivateRepresentation.md,
31+
// docs/NegativeConsumerFixtureValidation.md.
32+
//
33+
// NEGATIVE-FIXTURE: component=Buffers
34+
#include <type_traits>
35+
36+
#include "System/Buffers/IPinnable.hpp"
37+
#include "System/Buffers/MemoryHandle.hpp"
38+
39+
#ifndef SHARP_RUNTIME_NEGATIVE_SITE
40+
#define SHARP_RUNTIME_NEGATIVE_SITE 0
41+
#endif
42+
43+
using System::Buffers::IPinnable;
44+
using System::Buffers::MemoryHandle;
45+
46+
namespace {
47+
struct CountingPinnable final : IPinnable {
48+
int value = 7;
49+
MemoryHandle Pin(SharpRuntime::intcs) override { return MemoryHandle(&value, this); }
50+
void Unpin() override {}
51+
};
52+
} // namespace
53+
54+
int main() {
55+
CountingPinnable pinnable;
56+
MemoryHandle handle = pinnable.Pin(0);
57+
58+
#if SHARP_RUNTIME_NEGATIVE_SITE == 1
59+
// NEGATIVE(memoryhandle-direct-pointer-write): is private within this context
60+
// | private
61+
handle.pointer_ = nullptr;
62+
#else
63+
handle = MemoryHandle(nullptr);
64+
#endif
65+
66+
#if SHARP_RUNTIME_NEGATIVE_SITE == 2
67+
// NEGATIVE(memoryhandle-direct-pointer-read): is private within this context
68+
// | private
69+
void* raw = handle.pointer_;
70+
(void)raw;
71+
#else
72+
void* raw = handle.getPointerProperty();
73+
(void)raw;
74+
#endif
75+
76+
#if SHARP_RUNTIME_NEGATIVE_SITE == 3
77+
// NEGATIVE(memoryhandle-detach-pinnable): is private within this context
78+
// | private
79+
// THE SITE THAT BREAKS SILENTLY RATHER THAN LOUDLY, and the reason the member is
80+
// private at all: detaching the IPinnable makes the subsequent Dispose() a no-op, so
81+
// the pin leaks with no diagnostic anywhere. It compiles, it runs, and it is wrong.
82+
handle.pinnable_ = nullptr;
83+
#else
84+
// There is no migrated spelling, by design -- releasing the pin is what Dispose() is.
85+
handle.Dispose();
86+
#endif
87+
88+
#if SHARP_RUNTIME_NEGATIVE_SITE == 4
89+
// NEGATIVE(memoryhandle-aggregate-init): no matching function
90+
// | private
91+
// | could not convert
92+
// | cannot convert
93+
MemoryHandle braced{&pinnable.value, &pinnable, nullptr};
94+
(void)braced;
95+
#else
96+
MemoryHandle braced{&pinnable.value, &pinnable};
97+
braced.Dispose();
98+
#endif
99+
100+
// UNCHANGED, and asserted so the fixture proves what did NOT break: both public
101+
// constructors, the getter, copyability, and the size -- this is an access change and
102+
// moves no layout, so no consumer needs a rebuild for it.
103+
const MemoryHandle defaulted{};
104+
MemoryHandle fromPointer(&pinnable.value);
105+
MemoryHandle copied = fromPointer;
106+
static_assert(std::is_copy_constructible_v<MemoryHandle>, "still copyable");
107+
static_assert(sizeof(MemoryHandle) == 24, "#2059 moved no layout");
108+
copied.Dispose();
109+
fromPointer.Dispose();
110+
return (defaulted.getPointerProperty() == nullptr) ? 0 : 1;
111+
}

0 commit comments

Comments
 (0)