apache / apache/arrow-dotnet

ArrowBuffer spans do not keep native memory alive: NativeMemoryManager's finalizer can free a buffer while a Span over it is in use

Open
#397 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
39
Forks
30
Avg merge
1d 9h
Merged PRs (30d)
16

Description

### Summary

A `ReadOnlySpan` obtained from `ArrowBuffer.Span` does not keep the buffer's memory alive. Because `MemoryAllocator.Default` allocates native memory owned by a `NativeMemoryManager` **whose finalizer frees it**, an array that becomes unreachable while spans over its buffers are still in use can have those buffers freed underneath them.

The result is a use-after-free in ordinary-looking consumer code. The failure is usually silent — freed `HGlobal` pages typically stay mapped, so the span keeps returning plausible data — and only occasionally surfaces as an `AccessViolationException`.

### Reproduction

`repro.csproj`:

```xml


Exe
net8.0
enable



```

`Program.cs`:

```csharp
using System.Buffers;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Apache.Arrow;

static StringArray BuildArray(int rows)
{
var b = new StringArray.Builder();
for (int i = 0; i < rows; i++) b.Append($"row-{i}");
return b.Build();
}

// WeakReference to the MemoryManager that owns the array's value buffer.
static WeakReference OwnerOf(StringArray a)
{
MemoryMarshal.TryGetMemoryManager>(
a.Data.Buffers[2].Memory, out var manager);
Console.WriteLine($"value buffer is owned by: {manager?.GetType().Name ?? "(managed array)"}");
return new WeakReference(manager);
}

// Stands in for the allocating work a consumer does while holding the spans.
static void DoWorkWhileHoldingSpans()
{
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool WithoutKeepAlive()
{
var array = BuildArray(100_000);
var owner = OwnerOf(array);

ReadOnlySpan offsets = MemoryMarshal.Cast(array.Data.Buffers[1].Span);
ReadOnlySpan values = array.Data.Buffers[2].Span;

DoWorkWhileHoldingSpans();

long total = 0;
for (int i = 0; i < 100_000; i++) total += offsets[i + 1] - offsets[i];
_ = values.Length;

return owner.IsAlive;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool WithKeepAlive()
{
var array = BuildArray(100_000);
var owner = OwnerOf(array);

ReadOnlySpan offsets = MemoryMarshal.Cast(array.Data.Buffers[1].Span);
ReadOnlySpan values = array.Data.Buffers[2].Span;

DoWorkWhileHoldingSpans();

long total = 0;
for (int i = 0; i < 100_000; i++) total += offsets[i + 1] - offsets[i];
_ = values.Length;

bool alive = owner.IsAlive;
GC.KeepAlive(array);
return alive;
}

Console.WriteLine($"buffer owner still alive, without GC.KeepAlive: {WithoutKeepAlive()}");
Console.WriteLine($"buffer owner still alive, with GC.KeepAlive: {WithKeepAlive()}");
```

Run with tiered compilation disabled, since tier-0 keeps locals alive to the end of the method and masks the problem:

```
DOTNET_TieredCompilation=0 dotnet run -c Release
```

Output:

```
value buffer is owned by: NativeMemoryManager
buffer owner still alive, without GC.KeepAlive: False
value buffer is owned by: NativeMemoryManager
buffer owner still alive, with GC.KeepAlive: True
```

`False` means the `NativeMemoryManager` was collected and finalized — i.e. `Marshal.FreeHGlobal` ran on the buffer — while the two spans above were still being read.

### Why it happens

Three behaviours combine:

1. `MemoryAllocator.Default` is a `NativeMemoryAllocator`, so any buffer produced by an Arrow builder lives in `Marshal.AllocHGlobal` memory.
2. `NativeMemoryManager` frees that memory from a **finalizer**.
3. `ArrowBuffer.Span` (and the `Values` properties on the typed arrays) hand out a raw span, which is a bare pointer into that native memory and keeps nothing alive.

A span over a *managed* `byte[]` is safe here, because its interior pointer is reported to the GC and roots the array. A span over native memory has no such protection, so the only thing keeping the allocation alive is a reference to the owning object — and in the code shape above there isn't one past the point where the spans are taken.

Notably, `NativeMemoryManager` already suppresses the analyzer that exists specifically to flag this:

```csharp
#pragma warning disable CA2015 // TODO: is this correct?
~NativeMemoryManager()
{
Dispose(false);
}
#pragma warning restore CA2015
```

[CA2015](https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2015) is *"Do not define finalizers for types derived from `MemoryManager`"*, and its rationale is exactly this failure mode.

### Why it is easy to miss

The freed pages usually remain mapped, so the span keeps returning correct-looking bytes and nothing is observed. It faults only when the page is genuinely returned to the OS.

We hit it as an `AccessViolationException` that killed a test host mid-run, in a dictionary encoder that took two spans off a caller-supplied array and then looped over a million rows while allocating. It appeared on a commit that changed only Markdown, having passed on the three commits before it — the code involved had been in place and "working" for some time.

### Note on `main`

The reference-counting layer added in #297 (`SharedMemoryHandle`, `ArrowBuffer.Retain()`, `ArrowBuffer : IDisposable`) makes sharing explicit, but does not close this: `SharedMemoryHandle` also defines a finalizer, and `ArrowBuffer.Span` still returns a span that roots nothing. So the hazard appears to remain on `main`.

### Possible directions

Any one of these would remove the failure mode; they are not mutually exclusive:

- **Drop the finalizers** on `NativeMemoryManager` (and `SharedMemoryHandle`), so that a missing release becomes a *leak* rather than a free-under-live-span. This is what CA2015 prescribes, and it converts a silent-corruption bug class into a diagnosable one. It is a behaviour change for callers who never dispose, but the `IDisposable`/ref-counting work already moves in that direction.
- **Make the lifetime-safe accessor the ergonomic one** — e.g. favour `Memory` (which does root the manager) over `Span`, or provide a scope type that holds the owner for the duration of a read.
- **Consider a managed-array allocator as the default**, reserving native allocation for interop and explicitly-aligned paths. This has real costs for alignment and the C Data Interface, so it is the least appealing of the three, but it would make the question moot for consumers that never touch native interop.

### Workaround for consumers

Keep the array reachable until after the last span read:

```csharp
var data = array.Data;
ReadOnlySpan values = data.Buffers[2].Span;
// ... work ...
GC.KeepAlive(array);
```

It is worth noting that this is an unwritten contract every consumer has to rediscover independently, and that code violating it looks correct and passes tests.

### Environment

- Apache.Arrow 23.0.0 (NuGet)
- .NET 8 target, .NET 10 SDK
- macOS 15 / arm64
- Also expected on `main` by inspection (see the note above)

Contributor guide

Open the contributing guide

Research direction

Start with NativeMemoryManager, SharedMemoryHandle, MemoryAllocator.Default, and ArrowBuffer.Span, then run the supplied repro with DOTNET_TieredCompilation=0. Compare the finalizer and ownership paths described in the issue and determine which lifetime policy should be adopted; done means spans cannot outlive the native allocation and the reproduction no longer permits a freed owner during reads.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, performance
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.