From 6977684f6b24ca9141d34550cfab04b968cb54e7 Mon Sep 17 00:00:00 2001 From: Doug Bunting <6431421+dougbu@users.noreply.github.com> Date: Thu, 16 Feb 2023 17:16:39 -0800 Subject: [PATCH 1/3] Correct `ValidateStreamForReading(...)` - fix #389 - check `Stream` before first read attempt - check `CanRead` (not `CanSeek`) before second or later read attempts - add lots of tests of related scenarios - nit: correct a recent `HttpContentMessageExtensions` comment --- .../HttpContentMessageExtensions.cs | 3 +- .../HttpMessageContent.cs | 11 +- .../HttpMessageContentTests.cs | 298 ++++++++++++++++-- 3 files changed, 283 insertions(+), 29 deletions(-) diff --git a/src/System.Net.Http.Formatting/HttpContentMessageExtensions.cs b/src/System.Net.Http.Formatting/HttpContentMessageExtensions.cs index ca883460b..fdc518c54 100644 --- a/src/System.Net.Http.Formatting/HttpContentMessageExtensions.cs +++ b/src/System.Net.Http.Formatting/HttpContentMessageExtensions.cs @@ -467,7 +467,8 @@ private static HttpContent CreateHeaderFields(HttpHeaders source, HttpHeaders de } } - // If we have content headers then create an HttpContent for this Response + // If we have content headers then create an HttpContent for this request or response. Otherwise, + // provide a HttpContent instance to overwrite the null or EmptyContent value. if (contentHeaders != null) { // Need to rewind the input stream to be at the position right after the HTTP header diff --git a/src/System.Net.Http.Formatting/HttpMessageContent.cs b/src/System.Net.Http.Formatting/HttpMessageContent.cs index 839888a2e..5723a26fd 100644 --- a/src/System.Net.Http.Formatting/HttpMessageContent.cs +++ b/src/System.Net.Http.Formatting/HttpMessageContent.cs @@ -359,12 +359,21 @@ private byte[] SerializeHeader() private void ValidateStreamForReading(Stream stream) { + // Stream is null case should be an extreme, incredibly unlikely corner case. Every HttpContent from + // the framework (see dotnet/runtime or .NET Framework reference source) provides a non-null Stream + // in the ReadAsStringAsync task's return value. Likely need a poorly-designed derived HttpContent + // to hit this. Mostly ignoring the fact this message doesn't make much sense for the case. + if (stream is null || !stream.CanRead) + { + throw Error.NotSupported(Properties.Resources.NotSupported_UnreadableStream); + } + // If the content needs to be written to a target stream a 2nd time, then the stream must support // seeking (e.g. a FileStream), otherwise the stream can't be copied a second time to a target // stream (e.g. a NetworkStream). if (_contentConsumed) { - if (stream != null && stream.CanRead) + if (stream.CanSeek) { stream.Position = 0; } diff --git a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs index 2ef0d3113..4206730b1 100644 --- a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs +++ b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs @@ -46,20 +46,34 @@ private static HttpResponseMessage CreateResponse(bool containsEntity) return httpResponse; } - private static async Task ReadContentAsync(HttpContent content) + private static async Task ReadContentAsync(HttpContent content, bool unBuffered = false) { - await content.LoadIntoBufferAsync(); + if (unBuffered) + { + var stream = new MemoryStream(); + await content.CopyToAsync(stream); + stream.Position = 0L; + + // StreamReader will dispose of the Stream. + using var reader = new StreamReader(stream); + + return await reader.ReadToEndAsync(); + } + else + { + await content.LoadIntoBufferAsync(); - return await content.ReadAsStringAsync(); + return await content.ReadAsStringAsync(); + } } - private static async Task ValidateRequest(HttpContent content, bool containsEntity) + private static async Task ValidateRequest(HttpContent content, bool containsEntity, bool unBuffered = false) { Assert.Equal(ParserData.HttpRequestMediaType, content.Headers.ContentType); long? length = content.Headers.ContentLength; Assert.NotNull(length); - string message = await ReadContentAsync(content); + string message = await ReadContentAsync(content, unBuffered); if (containsEntity) { Assert.Equal(ParserData.HttpRequestWithEntity.Length, length); @@ -72,14 +86,14 @@ private static async Task ValidateRequest(HttpContent content, bool containsEnti } } - private static async Task ValidateResponse(HttpContent content, bool containsEntity) + private static async Task ValidateResponse(HttpContent content, bool containsEntity, bool unBuffered = false) { Assert.Equal(ParserData.HttpResponseMediaType, content.Headers.ContentType); long? length = content.Headers.ContentLength; Assert.NotNull(length); - string message = await ReadContentAsync(content); + string message = await ReadContentAsync(content, unBuffered); if (containsEntity) { Assert.Equal(ParserData.HttpResponseWithEntity.Length, length); @@ -153,14 +167,16 @@ public async Task SerializeRequestWithExistingHostHeader() Assert.Equal(ParserData.HttpRequestWithHost, message); } - [Fact] - public async Task SerializeRequestMultipleTimes() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task SerializeRequestMultipleTimes(bool unBuffered) { - HttpRequestMessage request = CreateRequest(ParserData.HttpRequestUri, false); - HttpMessageContent instance = new HttpMessageContent(request); + HttpRequestMessage request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + HttpMessageContent instance = new(request); for (int cnt = 0; cnt < iterations; cnt++) { - await ValidateRequest(instance, false); + await ValidateRequest(instance, containsEntity: false, unBuffered); } } @@ -175,14 +191,16 @@ public async Task SerializeResponse() } } - [Fact] - public async Task SerializeResponseMultipleTimes() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task SerializeResponseMultipleTimes(bool unBuffered) { - HttpResponseMessage response = CreateResponse(false); - HttpMessageContent instance = new HttpMessageContent(response); + HttpResponseMessage response = CreateResponse(containsEntity: false); + HttpMessageContent instance = new(response); for (int cnt = 0; cnt < iterations; cnt++) { - await ValidateResponse(instance, false); + await ValidateResponse(instance, containsEntity: false, unBuffered); } } @@ -197,14 +215,16 @@ public async Task SerializeRequestWithEntity() } } - [Fact] - public async Task SerializeRequestWithEntityMultipleTimes() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task SerializeRequestWithEntityMultipleTimes(bool unBuffered) { - HttpRequestMessage request = CreateRequest(ParserData.HttpRequestUri, true); - HttpMessageContent instance = new HttpMessageContent(request); + HttpRequestMessage request = CreateRequest(ParserData.HttpRequestUri, containsEntity: true); + HttpMessageContent instance = new(request); for (int cnt = 0; cnt < iterations; cnt++) { - await ValidateRequest(instance, true); + await ValidateRequest(instance, containsEntity: true, unBuffered); } } @@ -219,14 +239,16 @@ public async Task SerializeResponseWithEntity() } } - [Fact] - public async Task SerializeResponseWithEntityMultipleTimes() + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task SerializeResponseWithEntityMultipleTimes(bool unBuffered) { - HttpResponseMessage response = CreateResponse(true); - HttpMessageContent instance = new HttpMessageContent(response); + HttpResponseMessage response = CreateResponse(containsEntity: true); + HttpMessageContent instance = new(response); for (int cnt = 0; cnt < iterations; cnt++) { - await ValidateResponse(instance, true); + await ValidateResponse(instance, containsEntity: true, unBuffered); } } @@ -303,5 +325,227 @@ public void DisposeInnerHttpResponseMessage() instance.Dispose(); Assert.ThrowsObjectDisposed(() => { response.StatusCode = HttpStatusCode.OK; }, typeof(HttpResponseMessage).FullName); } + + [Fact] + public void Request_ContentLengthNull_IfReadOnlyStream() + { + var request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + request.Content = new StreamContent(new ReadOnlyStream()); + var instance = new HttpMessageContent(request); + + var length = instance.Headers.ContentLength; + + Assert.Null(length); + } + + [Fact] + public void Response_ContentLengthNull_IfReadOnlyStream() + { + var response = CreateResponse(containsEntity: false); + response.Content = new StreamContent(new ReadOnlyStream()); + var instance = new HttpMessageContent(response); + + var length = instance.Headers.ContentLength; + + Assert.Null(length); + } + + // Also confirms content can be serialized multiple times if either buffered or involves a seekable Stream. + [Theory] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task Request_NoContentLength_IfNotRequested(bool readOnlyStream, bool unBuffered) + { + var request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + if (readOnlyStream) + { + request.Content = new StreamContent(new ReadOnlyStream()); + } + var instance = new HttpMessageContent(request); + + for (int cnt = 0; cnt < iterations; cnt++) + { + var contentString = await ReadContentAsync(instance, unBuffered); + + Assert.Equal(ParserData.HttpRequest.Replace("Content-Length: 0\r\n", ""), contentString); + } + } + + // Also confirms content can be serialized multiple times if either buffered or involves a seekable Stream. + [Theory] + [InlineData(false, true)] + [InlineData(true, false)] + public async Task Response_NoContentLength_IfNotRequested(bool readOnlyStream, bool unBuffered) + { + var response = CreateResponse(containsEntity: false); + if (readOnlyStream) + { + response.Content = new StreamContent(new ReadOnlyStream()); + } + var instance = new HttpMessageContent(response); + + for (int cnt = 0; cnt < iterations; cnt++) + { + var contentString = await ReadContentAsync(instance, unBuffered); + + Assert.Equal(ParserData.HttpResponse.Replace("Content-Length: 0\r\n", ""), contentString); + } + } + + // Covers the false, false case of Request_NoContentLength_IfNotRequested(...). + [Fact] + public async Task Request_HasContentLength_IfBuffered_EvenIfNotRequested() + { + var request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + var instance = new HttpMessageContent(request); + for (int cnt = 0; cnt < iterations; cnt++) + { + var contentString = await ReadContentAsync(instance, unBuffered: false); + + Assert.Equal(ParserData.HttpRequest, contentString); + } + } + + // Covers the false, false case of Response_NoContentLength_IfNotRequested(...). + [Fact] + public async Task Response_HasContentLength_IfBuffered_EvenIfNotRequested() + { + var response = CreateResponse(containsEntity: false); + var instance = new HttpMessageContent(response); + for (int cnt = 0; cnt < iterations; cnt++) + { + var contentString = await ReadContentAsync(instance, unBuffered: false); + + Assert.Equal(ParserData.HttpResponse, contentString); + } + } + + // Covers the true, true case of Request_NoContentLength_IfNotRequested(...). + [Fact] + public async Task Request_CannotSerializeMultipleTimes_IfNotBufferedAndNotSeekable() + { + var request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + request.Content = new StreamContent(new ReadOnlyStream()); + var instance = new HttpMessageContent(request); + + // Act #1 + var contentString = await ReadContentAsync(instance, unBuffered: true); + + // Assert #1 + Assert.Equal(ParserData.HttpRequest.Replace("Content-Length: 0\r\n", ""), contentString); + + // Act #2 + await Assert.ThrowsAsync( + () => ReadContentAsync(instance, unBuffered: true), + "The 'HttpContent' of the 'HttpRequestMessage' has already been read."); + } + + // Covers the true, true case of Response_NoContentLength_IfNotRequested(...). + [Fact] + public async Task Response_CannotSerializeMultipleTimes_IfNotBufferedAndNotSeekable() + { + var response = CreateResponse(containsEntity: false); + response.Content = new StreamContent(new ReadOnlyStream()); + var instance = new HttpMessageContent(response); + + // Act #1 + var contentString = await ReadContentAsync(instance, unBuffered: true); + + // Assert #1 + Assert.Equal(ParserData.HttpResponse.Replace("Content-Length: 0\r\n", ""), contentString); + + // Act #2 + await Assert.ThrowsAsync( + () => ReadContentAsync(instance, unBuffered: true), + "The 'HttpContent' of the 'HttpResponseMessage' has already been read."); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Request_CannotSerialize_IfWriteOnlyStream(bool unBuffered) + { + var request = CreateRequest(ParserData.HttpRequestUri, containsEntity: false); + request.Content = new StreamContent(new WriteOnlyStream()); + var instance = new HttpMessageContent(request); + + await Assert.ThrowsAsync( + () => ReadContentAsync(instance, unBuffered), + "Stream does not support reading."); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Response_CannotSerialize_IfWriteOnlyStream(bool unBuffered) + { + var response = CreateResponse(containsEntity: false); + response.Content = new StreamContent(new WriteOnlyStream()); + var instance = new HttpMessageContent(response); + + await Assert.ThrowsAsync( + () => ReadContentAsync(instance, unBuffered), + "Stream does not support reading."); + } + + // Unlike Stream.Null, this stream does not support seeking. Bit more like (say) a network stream or + // the EmptyReadStream introduced in .NET 5. Note: EmptyReadStream should never be visible to our code + // because HttpContentMessageExtensions and HttpRequestMessageExtensions overwrite + // HttpResponseMessage.Content (or HttpRequestMessage.Content in one case) when creating an instance. + private class ReadOnlyStream : Stream + { + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotImplementedException(); + public override long Position + { + get => throw new NotImplementedException(); + set => throw new NotImplementedException(); + } + + public override void Flush() + { + // Do nothing. + } + + public override int Read(byte[] buffer, int offset, int count) => 0; + + public override long Seek(long offset, SeekOrigin origin) => throw new NotImplementedException(); + + public override void SetLength(long value) => throw new NotImplementedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotImplementedException(); + } + + // Unlike Stream.Null, this stream does not support seeking. Bit more like (say) a network stream. + private class WriteOnlyStream : Stream + { + public override bool CanRead => false; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotImplementedException(); + public override long Position + { + get => throw new NotImplementedException(); + set => throw new NotImplementedException(); + } + + public override void Flush() + { + // Do nothing. + } + + public override int Read(byte[] buffer, int offset, int count) => throw new NotImplementedException(); + + public override long Seek(long offset, SeekOrigin origin) => throw new NotImplementedException(); + + public override void SetLength(long value) => throw new NotImplementedException(); + + public override void Write(byte[] buffer, int offset, int count) + { + // Ignore all parameters and do nothing. + } + } } } From da6d49a6c48c6a00d308083cc994a378a957de40 Mon Sep 17 00:00:00 2001 From: Doug Bunting <6431421+dougbu@users.noreply.github.com> Date: Thu, 23 Feb 2023 15:47:34 -0800 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Chris Ross --- src/System.Net.Http.Formatting/HttpMessageContent.cs | 2 +- .../HttpMessageContentTests.cs | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/System.Net.Http.Formatting/HttpMessageContent.cs b/src/System.Net.Http.Formatting/HttpMessageContent.cs index 5723a26fd..c182d40c8 100644 --- a/src/System.Net.Http.Formatting/HttpMessageContent.cs +++ b/src/System.Net.Http.Formatting/HttpMessageContent.cs @@ -361,7 +361,7 @@ private void ValidateStreamForReading(Stream stream) { // Stream is null case should be an extreme, incredibly unlikely corner case. Every HttpContent from // the framework (see dotnet/runtime or .NET Framework reference source) provides a non-null Stream - // in the ReadAsStringAsync task's return value. Likely need a poorly-designed derived HttpContent + // in the ReadAsStreamAsync task's return value. Likely need a poorly-designed derived HttpContent // to hit this. Mostly ignoring the fact this message doesn't make much sense for the case. if (stream is null || !stream.CanRead) { diff --git a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs index 4206730b1..0a1d756f6 100644 --- a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs +++ b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs @@ -50,12 +50,7 @@ private static async Task ReadContentAsync(HttpContent content, bool unB { if (unBuffered) { - var stream = new MemoryStream(); - await content.CopyToAsync(stream); - stream.Position = 0L; - - // StreamReader will dispose of the Stream. - using var reader = new StreamReader(stream); + using var reader = new StreamReader(await content.ReadAsStreamAsync()); return await reader.ReadToEndAsync(); } From 0d58c8269ae734e37d9aa0e73f01f6902a86d133 Mon Sep 17 00:00:00 2001 From: Doug Bunting <6431421+dougbu@users.noreply.github.com> Date: Thu, 23 Feb 2023 17:16:41 -0800 Subject: [PATCH 3/3] Revert part of the previous commit - `ReadAsStreamAsync()` introduces unwanted behaviours - returned `Stream` cannot be read multiple times and is not recreated on next `ReadAsStreamAsync` call - method buffers the `HttpContent`, invalidating the intent of `unBuffered` --- .../HttpMessageContentTests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs index 0a1d756f6..4206730b1 100644 --- a/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs +++ b/test/System.Net.Http.Formatting.Test/HttpMessageContentTests.cs @@ -50,7 +50,12 @@ private static async Task ReadContentAsync(HttpContent content, bool unB { if (unBuffered) { - using var reader = new StreamReader(await content.ReadAsStreamAsync()); + var stream = new MemoryStream(); + await content.CopyToAsync(stream); + stream.Position = 0L; + + // StreamReader will dispose of the Stream. + using var reader = new StreamReader(stream); return await reader.ReadToEndAsync(); }