Decreasing the window size below the amount of data sent can prematurely abort the connection

[BrennanConroy]

The following error can occur in specific scenarios if the server has sent some data on a stream and the client updates the window size to some value below the amount already sent

Microsoft.AspNetCore.Server.Kestrel.Http2 Critical: The event loop in connection 0HMP8EQDO1NJO failed unexpectedly.
System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'length')
    at System.ThrowHelper.ThrowStartOrEndArgumentValidationException(Int64 start)
    at System.Buffers.ReadOnlySequence`1.Slice(Int64 start, Int64 length)
    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.WriteToOutputPipe() in /_/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs:line 139

Method 1

For example, if the server sends 2 bytes with an initial window size of 3, then the client updates the window to be 1. The server will successfully have scheduled a new item to the output loop for processing:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs

Lines 312 to 314 in 7aca303

	if (_streamWindow > 0)
	{
	Schedule();

But before it starts processing the item in

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs

Lines 111 to 117 in c93d0d0

	private async Task WriteToOutputPipe()
	{
	while (await _channel.Reader.WaitToReadAsync())
	{
	// We need to handle the case where aborts can be scheduled while this loop is running and might be on the way to complete
	// the reader.
	while (_channel.Reader.TryRead(out var producer) && !producer.CompletedResponse)

The updated Window can be processed:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

Lines 891 to 897 in ff6c5e7

	var windowSizeDifference = (int)_clientSettings.InitialWindowSize - previousInitialWindowSize;
	if (windowSizeDifference != 0)
	{
	foreach (var stream in _streams.Values)
	{
	if (!stream.TryUpdateOutputWindow(windowSizeDifference))

Which will decrease the _streamWindow to a negative value, which will then fail the buffer slice at

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2FrameWriter.cs

Lines 131 to 139 in c93d0d0

	var actual = producer.CheckStreamWindow(buffer.Length);
	// Now check the connection window
	actual = CheckConnectionWindow(actual);
	// Write what we can
	if (actual < buffer.Length)
	{
	buffer = buffer.Slice(0, actual);

---

Method 2

Another way this can be hit is if Stop is called once the _streamWindow is negative, even if nothing has been scheduled yet, because it will call Schedule itself without any checks for _streamWindow

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs

Lines 566 to 583 in ff6c5e7

	public void Stop()
	{
	lock (_dataWriterLock)
	{
	_waitingForWindowUpdates = false;
	if (_completeScheduled && _completedResponse)
	{
	// We can overschedule as long as we haven't yet completed the response. This is important because
	// we may need to abort the stream if it's waiting for a window update.
	return;
	}
	_completeScheduled = true;
	EnqueueStateUpdate(State.Aborted);
	Schedule();

---

Method 3

Doing multiple writes can also cause this, if the window update happens after the first write, but before the second one (or if it happens during the race between starting to process the second write and reading the settings as described in method 1).
This path also calls Schedule without a check for _streamWindow.

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs

Lines 388 to 407 in ff6c5e7

	lock (_dataWriterLock)
	{
	ThrowIfSuffixSentOrCompleted();
	// This length check is important because we don't want to set _startedWritingDataFrames unless a data
	// frame will actually be written causing the headers to be flushed.
	if (_completeScheduled || data.Length == 0)
	{
	return Task.CompletedTask;
	}
	_startedWritingDataFrames = true;
	_pipeWriter.Write(data);
	EnqueueDataWrite(data.Length);
	var task = _flusher.FlushAsync(this, cancellationToken).GetAsTask();
	Schedule();

---

This following test will hit the issue in either the Stop path described in method 2 or the multi write path described in method 3.

[Fact]
public async Task DecreaseWindowBelowCurrentSentAmount()
{
    const int windowSize = 3;
    _clientSettings.InitialWindowSize = windowSize;

    var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);

    await InitializeConnectionAsync(async context =>
    {
        var bodyControlFeature = context.Features.Get<IHttpBodyControlFeature>();
        bodyControlFeature.AllowSynchronousIO = true;
        await context.Response.Body.WriteAsync(new byte[windowSize - 1], 0, windowSize - 1);
        await tcs.Task;
        context.Response.Body.Write(new byte[windowSize], 0, windowSize);
    });

    await StartStreamAsync(1, _browserRequestHeaders, endStream: true);

    await ExpectAsync(Http2FrameType.HEADERS,
        withLength: 32,
        withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS,
        withStreamId: 1);

    // Decrease window size after server has already sent the current window - 1 size of data
    _clientSettings.InitialWindowSize = windowSize - 2;
    await SendSettingsAsync();

    await ExpectAsync(Http2FrameType.DATA,
        withLength: windowSize - 1,
        withFlags: (byte)Http2DataFrameFlags.NONE,
        withStreamId: 1);

    await ExpectAsync(Http2FrameType.SETTINGS,
        withLength: 0,
        withFlags: (byte)Http2DataFrameFlags.END_STREAM,
        withStreamId: 0);

    tcs.SetResult();

    await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false);
}

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[BrennanConroy]

Moving to backlog. This is a low-pri bug, only affects an edge case in the HTTP/2 spec and only sometimes since it's a race.