CommunityToolkit / CommunityToolkit/dotnet

IBufferWriterExtensions.Write : actively harmful

Open
#1,208 0 comments 0 reactions 0 assignees View on GitHub
bug :bug:
Dominant language
C#
Stars
3.8k
Forks
400
PR merge metrics
No merged PRs in 30d

Description

### Describe the bug

Context: `IBufferWriter` and the `sizeHint` in `GetSpan`/`GetMemory`. While the docs mention that the result should be at least this size, this largely relates to the original spec when this was `minSize` (or similar); in reality, it is not assumed that the `sizeHint` is always respected, and consuming code typically *tests* the buffer and applies fallback behaviour. For an example, see [`BuffersExtensions`](https://github.com/dotnet/dotnet/blob/main/src/runtime/src/libraries/System.Memory/src/System/Buffers/BuffersExtensions.cs#L116)

The `sizeHint` as a *hint* rather than a *demand* is important for scenarios where a transport has page size limits, and can honour *reasonable* requests, but not *excessive* requests; the caller can still ask for what it would *like*, but typically settles for what it *gets*. It is also possible for the consumer to ask for minimal sizes and *hope* that it gets much, much more, but this has performance implications (fragmentation, multiple resize chains, etc). The point here is that the loop is mandatory and the hint policy inside it is the BCL's / provider's business to tune - either policy is correct, whereas demanding one contiguous span is not.

The implementation [here](https://github.com/CommunityToolkit/dotnet/blob/main/src/CommunityToolkit.HighPerformance/Extensions/IBufferWriterExtensions.cs#L106-L123) is actively hostile; `BuffersExtensions` is exposed via the System.Memory package so is already available, and does the right thing, specifically: when oversized, it uses a second method (inline-optimized for "it fits", pathological case doesn't inline) that loops copying down in slices.

This means this method achieves nothing useful, and can be actively harmful.

Other issues:
- active overload ambiguity on the `T` version if both namespaces in-play (and `T` is not `byte`)
- there's a silent overload hijack on the [`byte`-version](https://github.com/CommunityToolkit/dotnet/blob/main/src/CommunityToolkit.HighPerformance/Extensions/IBufferWriterExtensions.cs#L95), with the broken version taking precedence
- **THIS HITS ALL TFMs** - it is not specific to down-level and is not gated by the `#if`

Recommendations:

- on the `T`-to-`T` version, mark `[Obsolete]` citing the `BuffersExtensions` version, redirect the work via `BuffersExtensions`, and remove the `this`, making it no-longer an extension method (no runtime API break; build-time API break intentional)
- potentially also tweak the T-to-bytes version to proxy via the same after the type-punning
- fix the `Write(this IBufferWriter writer, T value)` version similarly
- (optional, perf related) possibly add a byte-to-byte version to avoid the hijack via byte-to-T, or add a `if (typeof(T) == typeof(byte))` test internally and let the JIT worry about it; both options **still leave the hijack**, note, but at least it is a hijack to a "good" version and the JIT may be able to see through the inline; the question is whether to add a new API and let the compiler deal with it, or let the JIT deal with the switch at runtime; either approach still hopes the JIT will inline

(I've audited runtimes targeted by this package; the "correct" version is always available)

### Regression

(unchanged behaviour back to Microsoft.Toolkit.HighPerformance 7.1.2)

### Steps to reproduce

``` csharp
using System;
using System.Buffers;
using CommunityToolkit.HighPerformance; // <-- delete this line and the first test passes

// Repro: CommunityToolkit.HighPerformance.IBufferWriterExtensions.Write(IBufferWriter, ReadOnlySpan)
// out-competes System.Buffers.BuffersExtensions.Write(IBufferWriter, ReadOnlySpan) for byte writers
// (concrete receiver beats generic receiver), and it demands the whole payload as a single contiguous
// span instead of looping, so any writer that hands out bounded segments blows up.
//
// net472 (System.Memory 4.6.3), run under mono:
// w.Write(span) [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(20)
// w.Write(span) [under-delivers ] FAIL ArgumentException, calls: GetSpan(20)
// BuffersExtensions.Write [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(0) Advance(8) GetSpan(12)
// BuffersExtensions.Write [under-delivers ] OK wrote 20, calls: GetSpan(0) Advance(8) GetSpan(12) Advance(8) GetSpan(4) Advance(4)
// toolkit .Write [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(20)
// toolkit .Write [under-delivers ] FAIL ArgumentException, calls: GetSpan(20)
//
// i.e. `w.Write(span)` == the toolkit method, never the BCL one. The under-delivering writer is the
// clean discriminator: the BCL loops and completes, the toolkit asks once and dies. (The throwing
// writer also kills the netfx BCL build, because System.Memory 4.6.3's WriteMultiSegment hints the
// remaining length; the current runtime version calls GetSpan() with no hint and survives - on
// net10.0 the two BCL rows are OK with calls: GetSpan(0) Advance(8) x3.)
internal static class Program
{
private static void Main()
{
byte[] payload = new byte[20];

// (demonstrates silent hijack)
// whatever `w.Write(span)` binds to, with `using CommunityToolkit.HighPerformance;` in scope
Run("w.Write(span) ", w => w.Write(new ReadOnlySpan(payload)));

// the BCL method, called explicitly
Run("BuffersExtensions.Write", w => BuffersExtensions.Write(w, payload));

// the toolkit method, called explicitly (fully qualified so we can remove the using directive)
Run("toolkit .Write ", w => CommunityToolkit.HighPerformance.IBufferWriterExtensions.Write(w, new ReadOnlySpan(payload)));
}

private static void Run(string label, Action> write)
{
foreach (bool throwOnBigHint in new[] { true, false })
{
MalignWriter writer = new MalignWriter(throwOnBigHint);
string mode = throwOnBigHint ? "throws on big hint" : "under-delivers ";
try
{
write(writer);
Console.WriteLine($"{label} [{mode}] OK wrote {writer.Written}, calls: {writer.Calls}");
}
catch (Exception ex)
{
Console.WriteLine($"{label} [{mode}] FAIL {ex.GetType().Name}, calls: {writer.Calls}");
}
}
}
}

// Hands out at most 8 bytes at a time. Per the IBufferWriter docs, GetSpan "can throw if the
// requested buffer size is not available" - so throwOnBigHint:true is a conforming writer, and
// throwOnBigHint:false is the sloppier variant plenty of hand-rolled writers actually implement.
internal sealed class MalignWriter : IBufferWriter
{
private const int SegmentSize = 8;
private readonly bool throwOnBigHint;
private byte[] current = new byte[SegmentSize];
private int used;

public MalignWriter(bool throwOnBigHint) => this.throwOnBigHint = throwOnBigHint;

public int Written { get; private set; }

public string Calls { get; private set; } = "";

public Span GetSpan(int sizeHint = 0)
{
Calls += $"GetSpan({sizeHint}) ";
if (sizeHint > SegmentSize && throwOnBigHint)
{
throw new OutOfMemoryException($"cannot supply {sizeHint} contiguous bytes");
}

if (used == current.Length)
{
current = new byte[SegmentSize];
used = 0;
}

// never more than the current segment, whatever was asked for
return new Span(current, used, current.Length - used);
}

public Memory GetMemory(int sizeHint = 0) => throw new NotSupportedException();

public void Advance(int count)
{
Calls += $"Advance({count}) ";
used += count;
Written += count;
}
}
```

### Expected behavior

1. `BuffersExtensions` is preferred
2. writers that return less than `sizeHint` from `GetSpan`/`GetMemory` still work

### Screenshots

_No response_

### IDE and version

Other

### IDE version

(not IDE related; all TFMs/runtimes, all builds)

### Nuget packages

- [ ] CommunityToolkit.Common
- [ ] CommunityToolkit.Diagnostics
- [x] CommunityToolkit.HighPerformance
- [ ] CommunityToolkit.Mvvm (aka MVVM Toolkit)

### Nuget package version(s)

8.4.0

### Additional context

_No response_

### Help us help you

Yes, I'd like to be assigned to work on this item

Contributor guide

Open the contributing guide

Research direction

Start with CommunityToolkit.HighPerformance/Extensions/IBufferWriterExtensions.cs around the T-to-T and byte overloads, then run the supplied MalignWriter reproduction against the toolkit and BuffersExtensions implementations. Done means the BCL implementation is preferred, bounded or under-delivering writers complete successfully, and the overload ambiguity and silent byte overload hijack are addressed as described.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend-api-design
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.