Skip to content

Don't send body in HEAD response when using PipeWriter.Advance before headers flushed #59725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 6, 2025
Merged

Don't send body in HEAD response when using PipeWriter.Advance before headers flushed #59725

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a38b96
Don't send body in HEAD response when using PipeWriter.Advance before…
BrennanConroy Jan 6, 2025
9bb37ca
test + fix
BrennanConroy Jan 6, 2025
96c885b
fix state bug
BrennanConroy Jan 14, 2025
000f75d
fb
BrennanConroy Feb 3, 2025
fdf96e3
fb
BrennanConroy Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 31 additions & 28 deletions src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ internal class Http1OutputProducer : IHttpOutputProducer, IDisposable
// Once write or flush is called, we modify the _currentChunkMemory to prepend the size of data written
// and append the end terminator.

private bool _autoChunk;
private ResponseBodyMode _responseBodyMode;

private bool _writeStreamSuffixCalled;

Expand Down Expand Up @@ -121,7 +121,7 @@ public ValueTask<FlushResult> WriteStreamSuffixAsync()
{
if (!_writeStreamSuffixCalled)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
var writer = new BufferWriter<PipeWriter>(_pipeWriter);
result = WriteAsyncInternal(ref writer, EndChunkedResponseBytes);
Expand All @@ -147,7 +147,7 @@ public ValueTask<FlushResult> FlushAsync(CancellationToken cancellationToken = d
return new ValueTask<FlushResult>(new FlushResult(false, true));
}

if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (_advancedBytesForChunk > 0)
{
Expand All @@ -173,7 +173,7 @@ static ValueTask<FlushResult> FlushAsyncChunked(Http1OutputProducer producer, Ca
// Local function so in the common-path the stack space for BufferWriter isn't reserved and cleared when it isn't used.

Debug.Assert(!producer._pipeWriterCompleted);
Debug.Assert(producer._autoChunk && producer._advancedBytesForChunk > 0);
Debug.Assert(producer._responseBodyMode == ResponseBodyMode.Chunked && producer._advancedBytesForChunk > 0);

var writer = new BufferWriter<PipeWriter>(producer._pipeWriter);
producer.WriteCurrentChunkMemoryToPipeWriter(ref writer);
Expand Down Expand Up @@ -203,7 +203,7 @@ public Memory<byte> GetMemory(int sizeHint = 0)
{
return LeasedMemory(sizeHint);
}
else if (_autoChunk)
else if (_responseBodyMode == ResponseBodyMode.Chunked)
{
return GetChunkedMemory(sizeHint);
}
Expand All @@ -228,7 +228,7 @@ public Span<byte> GetSpan(int sizeHint = 0)
{
return LeasedMemory(sizeHint).Span;
}
else if (_autoChunk)
else if (_responseBodyMode == ResponseBodyMode.Chunked)
{
return GetChunkedMemory(sizeHint).Span;
}
Expand Down Expand Up @@ -262,7 +262,7 @@ public void Advance(int bytes)
_position += bytes;
}
}
else if (_autoChunk)
else if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (_advancedBytesForChunk > _currentChunkMemory.Length - _currentMemoryPrefixBytes - EndChunkLength - bytes)
{
Expand Down Expand Up @@ -333,7 +333,7 @@ private void CommitChunkInternal(ref BufferWriter<PipeWriter> writer, ReadOnlySp
writer.Commit();
}

public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appComplete)
public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, bool appComplete)
{
lock (_contextLock)
{
Expand All @@ -346,11 +346,11 @@ public void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpRespo

var buffer = _pipeWriter;
var writer = new BufferWriter<PipeWriter>(buffer);
WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, autoChunk);
WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, responseBodyMode);
}
}

private void WriteResponseHeadersInternal(ref BufferWriter<PipeWriter> writer, int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk)
private void WriteResponseHeadersInternal(ref BufferWriter<PipeWriter> writer, int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode)
{
writer.Write(HttpVersion11Bytes);
var statusBytes = ReasonPhrases.ToStatusBytes(statusCode, reasonPhrase);
Expand All @@ -360,7 +360,7 @@ private void WriteResponseHeadersInternal(ref BufferWriter<PipeWriter> writer, i

writer.Commit();

_autoChunk = autoChunk;
_responseBodyMode = responseBodyMode;
WriteDataWrittenBeforeHeaders(ref writer);
_unflushedBytes += writer.BytesCommitted;

Expand All @@ -373,11 +373,11 @@ private void WriteDataWrittenBeforeHeaders(ref BufferWriter<PipeWriter> writer)
{
foreach (var segment in _completedSegments)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
else if (_responseBodyMode == ResponseBodyMode.ContentLength)
{
writer.Write(segment.Span);
writer.Commit();
Expand All @@ -391,16 +391,19 @@ private void WriteDataWrittenBeforeHeaders(ref BufferWriter<PipeWriter> writer)

if (!_currentSegment.IsEmpty)
{
var segment = _currentSegment.Slice(0, _position);

if (_autoChunk)
if (_responseBodyMode != ResponseBodyMode.Disabled)
{
CommitChunkInternal(ref writer, segment.Span);
}
else
{
writer.Write(segment.Span);
writer.Commit();
var segment = _currentSegment.Slice(0, _position);

if (_responseBodyMode == ResponseBodyMode.Chunked)
{
CommitChunkInternal(ref writer, segment.Span);
}
else if (_responseBodyMode == ResponseBodyMode.ContentLength)
{
writer.Write(segment.Span);
writer.Commit();
}
}

_position = 0;
Expand Down Expand Up @@ -491,7 +494,7 @@ public ValueTask<FlushResult> Write100ContinueAsync()
return WriteAsync(ContinueBytes);
}

public ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, ReadOnlySpan<byte> buffer, CancellationToken cancellationToken)
public ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, ReadOnlySpan<byte> buffer, CancellationToken cancellationToken)
{
lock (_contextLock)
{
Expand All @@ -505,13 +508,13 @@ public ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhra
// Uses same BufferWriter to write response headers and response
var writer = new BufferWriter<PipeWriter>(_pipeWriter);

WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, autoChunk);
WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, responseBodyMode);

return WriteAsyncInternal(ref writer, buffer, cancellationToken);
}
}

public ValueTask<FlushResult> FirstWriteChunkedAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, ReadOnlySpan<byte> buffer, CancellationToken cancellationToken)
public ValueTask<FlushResult> FirstWriteChunkedAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, ReadOnlySpan<byte> buffer, CancellationToken cancellationToken)
{
lock (_contextLock)
{
Expand All @@ -525,7 +528,7 @@ public ValueTask<FlushResult> FirstWriteChunkedAsync(int statusCode, string? rea
// Uses same BufferWriter to write response headers and chunk
var writer = new BufferWriter<PipeWriter>(_pipeWriter);

WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, autoChunk);
WriteResponseHeadersInternal(ref writer, statusCode, reasonPhrase, responseHeaders, responseBodyMode);

CommitChunkInternal(ref writer, buffer);

Expand All @@ -541,7 +544,7 @@ public void Reset()
Debug.Assert(_completedSegments == null || _completedSegments.Count == 0);
// Cleared in sequential address ascending order
_currentMemoryPrefixBytes = 0;
_autoChunk = false;
_responseBodyMode = ResponseBodyMode.ContentLength;
_writeStreamSuffixCalled = false;
_currentChunkMemoryUpdated = false;
_startCalled = false;
Expand Down Expand Up @@ -570,7 +573,7 @@ private ValueTask<FlushResult> WriteAsyncInternal(
ReadOnlySpan<byte> buffer,
CancellationToken cancellationToken = default)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (_advancedBytesForChunk > 0)
{
Expand Down
47 changes: 24 additions & 23 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,11 @@ internal abstract partial class HttpProtocol : IHttpResponseControl
// Keep-alive is default for HTTP/1.1 and HTTP/2; parsing and errors will change its value
// volatile, see: https://msdn.microsoft.com/en-us/library/x13ttww7.aspx
protected volatile bool _keepAlive = true;
// _canWriteResponseBody is set in CreateResponseHeaders.
// _responseBodyMode is set in CreateResponseHeaders.
// If we are writing with GetMemory/Advance before calling StartAsync, assume we can write and throw away contents if we can't.
private bool _canWriteResponseBody = true;
private ResponseBodyMode _responseBodyMode = ResponseBodyMode.ContentLength;
private bool _hasAdvanced;
private bool _isLeasedMemoryInvalid = true;
private bool _autoChunk;
protected Exception? _applicationException;
private BadHttpRequestException? _requestRejectedException;

Expand Down Expand Up @@ -347,7 +346,7 @@ public void Reset()
_routeValues?.Clear();

_requestProcessingStatus = RequestProcessingStatus.RequestPending;
_autoChunk = false;
_responseBodyMode = ResponseBodyMode.ContentLength;
_applicationException = null;
_requestRejectedException = null;

Expand Down Expand Up @@ -397,7 +396,6 @@ public void Reset()

_isLeasedMemoryInvalid = true;
_hasAdvanced = false;
_canWriteResponseBody = true;

if (_scheme == null)
{
Expand Down Expand Up @@ -999,7 +997,7 @@ private void ProduceStart(bool appCompleted)

var responseHeaders = CreateResponseHeaders(appCompleted);

Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _responseBodyMode, appCompleted);
}

private void VerifyInitializeState(int firstWriteByteCount)
Expand Down Expand Up @@ -1067,7 +1065,7 @@ protected Task ProduceEnd()

private Task WriteSuffix()
{
if (_autoChunk || _httpVersion >= Http.HttpVersion.Http2)
if (_responseBodyMode == ResponseBodyMode.Chunked || _httpVersion >= Http.HttpVersion.Http2)
{
// For the same reason we call CheckLastWrite() in Content-Length responses.
PreventRequestAbortedCancellation();
Expand Down Expand Up @@ -1161,9 +1159,9 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
}

// Set whether response can have body
_canWriteResponseBody = CanWriteResponseBody();
_responseBodyMode = CanWriteResponseBody() ? ResponseBodyMode.ContentLength : ResponseBodyMode.Disabled;
Copy link
Member

@halter73 halter73 Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I suggested an Unitialized state. It seems wrong to have the mode be ContentLength both here and after calling Reset() when we could ultimately end up auto-chunking.


if (!_canWriteResponseBody && hasTransferEncoding)
if (_responseBodyMode == ResponseBodyMode.Disabled && hasTransferEncoding)
{
RejectInvalidHeaderForNonBodyResponse(appCompleted, HeaderNames.TransferEncoding);
}
Expand Down Expand Up @@ -1197,7 +1195,7 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
}
else if (!hasTransferEncoding && !responseHeaders.ContentLength.HasValue)
{
if ((appCompleted || !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders
if ((appCompleted || _responseBodyMode == ResponseBodyMode.Disabled) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders
{
if (CanAutoSetContentLengthZeroResponseHeader())
{
Expand All @@ -1218,8 +1216,11 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
// The chunked transfer encoding defined in Section 4.1 of [RFC7230] MUST NOT be used in HTTP/2.
else if (_httpVersion == Http.HttpVersion.Http11)
{
_autoChunk = true;
responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
if (_responseBodyMode == ResponseBodyMode.ContentLength)
{
_responseBodyMode = ResponseBodyMode.Chunked;
responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
}
}
else
{
Expand Down Expand Up @@ -1470,7 +1471,7 @@ public void Advance(int bytes)
throw new InvalidOperationException("Invalid ordering of calling StartAsync or CompleteAsync and Advance.");
}

if (_canWriteResponseBody)
if (_responseBodyMode != ResponseBodyMode.Disabled)
{
VerifyAndUpdateWrite(bytes);
Output.Advance(bytes);
Expand Down Expand Up @@ -1591,9 +1592,9 @@ public ValueTask<FlushResult> WritePipeAsync(ReadOnlyMemory<byte> data, Cancella
VerifyAndUpdateWrite(data.Length);
}

if (_canWriteResponseBody)
if (_responseBodyMode != ResponseBodyMode.Disabled)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (data.Length == 0)
{
Expand Down Expand Up @@ -1639,27 +1640,27 @@ private ValueTask<FlushResult> FirstWriteAsyncInternal(ReadOnlyMemory<byte> data
{
var responseHeaders = InitializeResponseFirstWrite(data.Length);

if (_canWriteResponseBody)
if (_responseBodyMode != ResponseBodyMode.Disabled)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (data.Length == 0)
{
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _responseBodyMode, appCompleted: false);
return Output.FlushAsync(cancellationToken);
}

return Output.FirstWriteChunkedAsync(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, data.Span, cancellationToken);
return Output.FirstWriteChunkedAsync(StatusCode, ReasonPhrase, responseHeaders, _responseBodyMode, data.Span, cancellationToken);
}
else
{
CheckLastWrite();
return Output.FirstWriteAsync(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, data.Span, cancellationToken);
return Output.FirstWriteAsync(StatusCode, ReasonPhrase, responseHeaders, _responseBodyMode, data.Span, cancellationToken);
}
}
else
{
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _autoChunk, appCompleted: false);
Output.WriteResponseHeaders(StatusCode, ReasonPhrase, responseHeaders, _responseBodyMode, appCompleted: false);
HandleNonBodyResponseWrite();
return Output.FlushAsync(cancellationToken);
}
Expand Down Expand Up @@ -1688,9 +1689,9 @@ public async ValueTask<FlushResult> WriteAsyncAwaited(Task initializeTask, ReadO

// WriteAsyncAwaited is only called for the first write to the body.
// Ensure headers are flushed if Write(Chunked)Async isn't called.
if (_canWriteResponseBody)
if (_responseBodyMode != ResponseBodyMode.Disabled)
{
if (_autoChunk)
if (_responseBodyMode == ResponseBodyMode.Chunked)
{
if (data.Length == 0)
{
Expand Down
13 changes: 10 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ internal interface IHttpOutputProducer
ValueTask<FlushResult> WriteChunkAsync(ReadOnlySpan<byte> data, CancellationToken cancellationToken);
ValueTask<FlushResult> FlushAsync(CancellationToken cancellationToken);
ValueTask<FlushResult> Write100ContinueAsync();
void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, bool appCompleted);
void WriteResponseHeaders(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, bool appCompleted);
// This takes ReadOnlySpan instead of ReadOnlyMemory because it always synchronously copies data before flushing.
ValueTask<FlushResult> WriteDataToPipeAsync(ReadOnlySpan<byte> data, CancellationToken cancellationToken);
// Test hook
Expand All @@ -25,7 +25,14 @@ internal interface IHttpOutputProducer
Memory<byte> GetMemory(int sizeHint = 0);
void CancelPendingFlush();
void Stop();
ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, ReadOnlySpan<byte> data, CancellationToken cancellationToken);
ValueTask<FlushResult> FirstWriteChunkedAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, bool autoChunk, ReadOnlySpan<byte> data, CancellationToken cancellationToken);
ValueTask<FlushResult> FirstWriteAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, ReadOnlySpan<byte> data, CancellationToken cancellationToken);
ValueTask<FlushResult> FirstWriteChunkedAsync(int statusCode, string? reasonPhrase, HttpResponseHeaders responseHeaders, ResponseBodyMode responseBodyMode, ReadOnlySpan<byte> data, CancellationToken cancellationToken);
void Reset();
}

internal enum ResponseBodyMode
{
Disabled,
Chunked,
ContentLength
}
Loading
Loading