Add configurable SSE parser buffer limit#132275
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParserOptions.cs:28
SseParserOptions<T>.MaxBufferSizeis documented as "-1 to use the default limit", but values < -1 currently flow through and (because of the_maxBufferSize >= 0guard) effectively disable the limit. It would be safer to validate thatMaxBufferSizeis either -1 or >= 0 and throwArgumentOutOfRangeExceptionotherwise.
/// <summary>Gets the parser to use to transform each payload of bytes into a data element.</summary>
public SseItemParser<T> ItemParser { get; }
/// <summary>Gets or sets the maximum buffer size, or -1 to use the default limit.</summary>
public int MaxBufferSize { get; set; } = -1;
src/libraries/System.Net.ServerSentEvents/tests/SseParserTests.cs:34
- There’s no test coverage for invalid
MaxBufferSizevalues (e.g., < -1). Adding a focused test would ensure the new option can’t be used to silently disable buffer limiting via negative values.
[Fact]
public void Options_DefaultMaxBufferSize()
{
var options = new SseParserOptions<string>(delegate { return ""; });
Assert.Equal(-1, options.MaxBufferSize);
}
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/libraries/System.Net.ServerSentEvents/tests/SseParserTests.cs:996
- This helper decodes payload bytes via bytes.ToArray(), which adds an extra allocation for every parsed event. Encoding.UTF8.GetString(ReadOnlySpan) can be used directly (as in SseParser.Create(Stream)).
private static SseParser<string> CreateParser(Stream stream) =>
CreateParser(stream, static (_, bytes) => Encoding.UTF8.GetString(bytes.ToArray()));
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser.cs:14
- The XML doc summary for the non-generic Create(Stream) overload references SseItem{T}, but this method always returns SseParser. This produces incorrect public docs/intellisense and broken cross-references.
/// <summary>Creates a parser for parsing a <paramref name="sseStream"/> of server-sent events into a sequence of <see cref="SseItem{T}"/> values.</summary>
src/libraries/System.Net.ServerSentEvents/tests/SseParserTests.cs:916
- These tests convert the ReadOnlySpan payload to an array before decoding. The product code path already uses Encoding.UTF8.GetString(ReadOnlySpan) directly; using ToArray() here adds per-event allocations and increases test runtime/GC pressure unnecessarily.
This issue also appears on line 995 of the same file.
var options = new SseParserOptions<string>(static (_, bytes) => Encoding.UTF8.GetString(bytes.ToArray()))
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:565
- MaxBufferSize enforcement currently happens only via GrowBuffer(minimumLength). That means the parser can still buffer more than the configured limit without calling GrowBuffer (e.g., initial Rent(1024) when MaxBufferSize < 1024, or when ArrayPool returns a bucket size larger than the requested minimumLength). To honor the contract, the code needs to validate the actual buffered byte counts (e.g., after incrementing _lineLength in FillLineBuffer/FillLineBufferAsync and after appending to _dataLength) against _maxBufferSize, not just the requested growth size.
/// <summary>Grows the buffer, returning the existing one to the ArrayPool and renting an ArrayPool replacement.</summary>
private void GrowBuffer([NotNull] ref byte[]? buffer, int minimumLength)
{
if (_maxBufferSize >= 0 && minimumLength > _maxBufferSize)
{
throw new InvalidDataException(SR.InvalidDataException_SseExceededMaxLength);
}
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:320
ShiftOrGrowLineBufferIfNecessarythrows as soon as_lineLength == _maxBufferSizewhen the buffer is full. This can incorrectly throw when the stream ends exactly at the configured limit (no more bytes available), because the parser never gets a chance to perform the final read that would observe EOF and discard pending data per spec.
else if (_lineLength == _lineBuffer.Length)
{
if (_lineLength >= _maxBufferSize)
{
throw new InvalidDataException(SR.InvalidDataException_SseExceededMaxLength);
}
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:550
FillLineBufferAsynchas the samecount == 0ambiguity as the synchronous path: a 0-length read completes immediately with 0 and is indistinguishable from EOF. This can cause an incorrect EOF transition rather than an over-limit exception (or vice versa) depending on stream state. Mirror the synchronous fix by doing a 1-byte async read whencount == 0.
ShiftOrGrowLineBufferIfNecessary();
int offset = _lineOffset + _lineLength;
int bytesRead = await _stream.ReadAsync(_lineBuffer.AsMemory(offset, GetLineBufferReadCount(_lineBuffer.Length - offset)), cancellationToken).ConfigureAwait(false);
src/libraries/System.Net.ServerSentEvents/tests/SseParserTests.cs:951
- The added max-buffer tests don’t cover the important boundary case where
MaxBufferSizeis exactly the enforced minimum and the stream ends exactly at that length. This is the scenario most likely to surface off-by-one / EOF-vs-limit bugs in the new limit enforcement logic.
[Theory]
[InlineData(false, MinConfigurableMaxBufferSize + 1)]
[InlineData(true, MinConfigurableMaxBufferSize + 1)]
public async Task Parse_MaxBufferSize_AllowsConfiguredLimit(bool useAsync, int maxBufferSize)
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParserOptions.cs:26
- MaxBufferSize is an integer byte limit, but the XML docs currently just say "maximum buffer size". Please clarify the unit (bytes) in the public API docs to avoid ambiguity for consumers.
/// <summary>Gets or sets the maximum buffer size, or -1 to use the default limit.</summary>
/// <exception cref="ArgumentOutOfRangeException">The value set is less than -1.</exception>
/// <remarks>Values below an internal minimum are treated as that minimum, as buffers smaller than that don't meaningfully reduce memory usage.</remarks>
public int MaxBufferSize
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser.cs:35
- The new expression-bodied Create(Stream, SseItemParser) constructs SseParserOptions before validating sseStream. If both arguments are null, this changes the exception/param order vs the previous implementation (itemParser null is thrown before sseStream null). Keep argument validation order stable by validating sseStream before constructing options.
public static SseParser<T> Create<T>(Stream sseStream, SseItemParser<T> itemParser) =>
Create(sseStream, new SseParserOptions<T>(itemParser));
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Co-authored-by: mrek-msft <188900745+mrek-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:519
- Configured MaxBufferSize can be exceeded when ArrayPool rents a buffer larger than requested: FillLineBuffer uses the full rented array length to decide how many bytes to read, so if _lineBuffer.Length > _maxBufferSize the parser may read and buffer beyond the configured limit instead of throwing InvalidDataException.
int offset = _lineOffset + _lineLength;
int count = _lineBuffer.Length - offset;
if (count == 0)
{
int probeBytesRead = _stream.Read(new byte[1], 0, 1);
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:558
- FillLineBufferAsync has the same max-limit bypass as the sync path: it only checks
offset == _lineBuffer.Lengthand then reads to the end of the rented array. If ArrayPool returns a buffer larger than_maxBufferSize, the async parser can buffer more than the configured limit without throwing.
int offset = _lineOffset + _lineLength;
if (offset == _lineBuffer.Length)
{
int probeBytesRead = await _stream.ReadAsync(new byte[1].AsMemory(), cancellationToken).ConfigureAwait(false);
if (probeBytesRead == 0)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:601
- The configurable max buffer size is no longer enforced for the event data buffer: GrowBuffer used to throw InvalidDataException when
minimumLength > _maxBufferSize, but that guard was removed when making the method static. As a result, a stream with many smalldata:lines can grow_dataBufferbeyond the configured limit (and beyond the previous default limit), defeating the purpose of MaxBufferSize and potentially enabling unbounded memory growth/DoS.
/// <summary>Grows the buffer, returning the existing one to the ArrayPool and renting an ArrayPool replacement.</summary>
private static void GrowBuffer([NotNull] ref byte[]? buffer, int minimumLength)
{
byte[]? toReturn = buffer;
buffer = ArrayPool<byte>.Shared.Rent(Math.Max(minimumLength, DefaultArrayPoolRentSize));
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser.cs:35
- Create(Stream, SseItemParser) now constructs SseParserOptions before validating sseStream. This changes argument-validation order (e.g., when both arguments are null) and also introduces an avoidable allocation on the null-stream error path. Consider validating sseStream first, then creating options.
public static SseParser<T> Create<T>(Stream sseStream, SseItemParser<T> itemParser) =>
Create(sseStream, new SseParserOptions<T>(itemParser));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:527
- The
count == 0path throws immediately when the line buffer is full, without checking whether the stream is actually at EOF. This can incorrectly throw when the buffered line length is exactlyMaxBufferSizeand the stream ends without a terminating newline (the parser would otherwise just discard the incomplete event at EOF). Consider probing the stream for EOF before throwing when no buffer space remains.
int offset = _lineOffset + _lineLength;
int count = _lineBuffer.Length - offset;
if (count == 0)
{
throw new InvalidDataException(SR.InvalidDataException_SseExceededMaxLength);
}
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:559
- Same as the synchronous path: when
count == 0, this throws without verifying EOF. If the stream ends exactly atMaxBufferSizewithout a newline, this can throw even though the parser should just observe EOF and discard the incomplete event.
int offset = _lineOffset + _lineLength;
int count = _lineBuffer.Length - offset;
if (count == 0)
{
throw new InvalidDataException(SR.InvalidDataException_SseExceededMaxLength);
}
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser_1.cs:343
- Typo in comment: "avalaible" → "available".
// Storage avalaible for at least one byte
src/libraries/System.Net.ServerSentEvents/src/System/Net/ServerSentEvents/SseParser.cs:35
Create<T>(Stream, SseItemParser<T>)now allocates aSseParserOptions<T>instance unconditionally just to delegate to the options overload. This adds an avoidable allocation to the common path; consider keeping the direct construction/validation for this overload and delegating the options overload to it (or adding an internal constructor that takesitemParser+maxBufferSizewithout requiring an options object).
public static SseParser<T> Create<T>(Stream sseStream, SseItemParser<T> itemParser) =>
Create(sseStream, new SseParserOptions<T>(itemParser));
SSE parsing now supports a caller-configured buffer limit through
SseParserOptions<T>.API
SseParserOptions<T>withItemParserandMaxBufferSize.MaxBufferSizeto-1by default, preserving the internal default limit.SseParser.Createoverload as the parser creation path.Coverage
Note
This description was generated by GitHub Copilot.