dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.41k stars 4.76k forks source link

SseFormatter internal buffer doesn't grow properly and throws #110028

Closed BrennanConroy closed 21 hours ago

BrennanConroy commented 1 day ago

Description

The ArrayBuffer type that is being used by PooledByteBufferWriter does not grow/create a new buffer if 0 is passed in as the byteCount.

https://github.com/dotnet/runtime/blob/1474fc3fafca26b4b051be7dacdba8ac2804c56e/src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/PooledByteBufferWriter.cs#L15-L17

https://github.com/dotnet/runtime/blob/1474fc3fafca26b4b051be7dacdba8ac2804c56e/src/libraries/Common/src/System/Net/ArrayBuffer.cs#L117-L122

This means that if you write greater than 256 bytes at one time it will throw due to using 0 as the size hint:

System.ThrowHelper.ThrowArgumentOutOfRangeException(System.ExceptionArgument) in ThrowHelper.cs
    System.Buffers.BuffersExtensions.WriteMultiSegment<T>(System.Buffers.IBufferWriter<T>, System.ReadOnlySpan<T>, System.Span<T>) in BuffersExtensions.cs
    System.Buffers.BuffersExtensions.Write<T>(System.Buffers.IBufferWriter<T>, System.ReadOnlySpan<T>) in BuffersExtensions.cs
    Microsoft.AspNetCore.Http.Connections.Internal.Transports.ServerSentEventsServerTransport.ProcessRequestAsync.AnonymousMethod__6_1(System.Net.ServerSentEvents.SseItem<System.Buffers.ReadOnlySequence<byte>>, System.Buffers.IBufferWriter<byte>) in ServerSentEventsServerTransport.cs
    System.Net.ServerSentEvents.SseFormatter.WriteAsyncCore<T>(System.Collections.Generic.IAsyncEnumerable<System.Net.ServerSentEvents.SseItem<T>>, System.IO.Stream, System.Action<System.Net.ServerSentEvents.SseItem<T>, System.Buffers.IBufferWriter<byte>>, System.Threading.CancellationToken) in SseFormatter.cs

Reproduction Steps

Write more than 256 bytes at a time with SseFormatter.WriteAsync

static async IAsyncEnumerable<SseItem<ReadOnlyMemory<byte>>> Item()
{
    await Task.CompletedTask;
    yield return new SseItem<ReadOnlyMemory<byte>>(new byte[257]);
}

await SseFormatter.WriteAsync(Item(), new MemoryStream(), (item, writer) =>
{
    writer.Write(buffer.FirstSpan);
}, default);

Expected behavior

Can write more than 256 bytes with SseFormatter.

Actual behavior

Throws:

System.ThrowHelper.ThrowArgumentOutOfRangeException(System.ExceptionArgument) in ThrowHelper.cs
    System.Buffers.BuffersExtensions.WriteMultiSegment<T>(System.Buffers.IBufferWriter<T>, System.ReadOnlySpan<T>, System.Span<T>) in BuffersExtensions.cs
    System.Buffers.BuffersExtensions.Write<T>(System.Buffers.IBufferWriter<T>, System.ReadOnlySpan<T>) in BuffersExtensions.cs
    Microsoft.AspNetCore.Http.Connections.Internal.Transports.ServerSentEventsServerTransport.ProcessRequestAsync.AnonymousMethod__6_1(System.Net.ServerSentEvents.SseItem<System.Buffers.ReadOnlySequence<byte>>, System.Buffers.IBufferWriter<byte>) in ServerSentEventsServerTransport.cs
    System.Net.ServerSentEvents.SseFormatter.WriteAsyncCore<T>(System.Collections.Generic.IAsyncEnumerable<System.Net.ServerSentEvents.SseItem<T>>, System.IO.Stream, System.Action<System.Net.ServerSentEvents.SseItem<T>, System.Buffers.IBufferWriter<byte>>, System.Threading.CancellationToken) in SseFormatter.cs

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

dotnet-policy-service[bot] commented 1 day ago

Tagging subscribers to this area: @dotnet/area-system-buffers See info in area-owners.md if you want to be subscribed.

dotnet-policy-service[bot] commented 1 day ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

stephentoub commented 1 day ago

Why does https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5ac14/src/libraries/System.Memory/src/System/Buffers/BuffersExtensions.cs#L118 not pass the span length as a hint?

Is this a holdover from when sizeHint was minimumLength and these places where no value was provided were never revisited? Here we're throwing away useful information the writer could be using to make an informed decision about what to allocate and what to give back.

BrennanConroy commented 22 hours ago

You would probably want to cap the value so the LOH isn't used.

eiriktsarpalis commented 19 hours ago

Why does

runtime/src/libraries/System.Memory/src/System/Buffers/BuffersExtensions.cs

not pass the span length as a hint?

I steered clear from that extension in the implementation for this reason, but I suppose this didn't account for the extension being used by custom delegates.

stephentoub commented 19 hours ago

You would probably want to cap the value so the LOH isn't used.

That's a concern for the implementations of the interface. It's a hint.

BrennanConroy commented 12 hours ago

That's a concern for the implementations of the interface. It's a hint.

The GetSpan method on PipeWriter says it is the minimum length to be returned: https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/PipeWriter.cs,75

/// <param name="sizeHint"> The minimum length of the returned <see cref="System.Span{T}" />. If 0, a non-empty buffer of arbitrary size is returned.

IBufferWriter says the same thing https://source.dot.net/#System.Memory/System/Buffers/IBufferWriter.cs,36