Skip to content
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.

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

Crash when deserializing a HttpResponseMessage on .NET Core #313

Closed
Daniel-Svensson opened this issue Jan 23, 2022 · 2 comments
Closed

Crash when deserializing a HttpResponseMessage on .NET Core #313

Daniel-Svensson opened this issue Jan 23, 2022 · 2 comments
Labels
Milestone

Comments

@Daniel-Svensson
Copy link

When a response contains "Expires: -1" the System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore method fails.
The method works as expected on .Net Framework, but fails on .Net Core 2.1+ including NET 6.0

This appears to be a break between .NET Core 2.0 and 2.1. w.r.t. how date headers are parsed. In 2.0, Expires: -1 is treated as valid.

My root cause/issue is that this bug breaks the the only good library for client side Http caching, which seems to have given many people headaches.
aliostad/CacheCow#213

The issues has been previously reported: #193 but was closed as external (which was only partly true), there is still code that needs to be fixed here in AspNetWebStack
The dotnet/runtime part is fixed dotnet/runtime#27218 and contains both testcase as well as further details.

Based on discussion in runtime repo it seems like the relevant code is:
https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Net.Http.Formatting/Formatting/Parsers/InternetMessageFormatHeaderParser.cs#L319-L334

And that validation should always be ignored for the Expires header, since it should be able to contain common values such as -1.

I am unsure about how you want to fix this since it can be changed just for the expires header in that specific method, or if the default value for header validation should change since it is possible to control that when creating the InternetMessageFormatHeaderParser

Stacktrace:

System.Net.Http.dll!System.Net.Http.Headers.HttpHeaderParser.ParseValue(string value, object storeValue, ref int index)
System.Net.Http.dll!System.Net.Http.Headers.HttpHeaders.ParseAndAddValue(System.Net.Http.Headers.HeaderDescriptor descriptor, System.Net.Http.Headers.HttpHeaders.HeaderStoreItemInfo info, string value)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.CurrentHeaderFieldStore.CopyTo(System.Net.Http.Headers.HttpHeaders headers, bool ignoreHeaderValidation)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.ParseHeaderFields(byte[] buffer, int bytesReady, ref int bytesConsumed, ref System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.HeaderFieldState requestHeaderState, int maximumHeaderLength, ref int totalBytesConsumed, System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.CurrentHeaderFieldStore currentField, System.Net.Http.Headers.HttpHeaders headers, bool ignoreHeaderValidation)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.InternetMessageFormatHeaderParser.ParseBuffer(byte[] buffer, int bytesReady, ref int bytesConsumed)
System.Net.Http.Formatting.dll!System.Net.Http.Formatting.Parsers.HttpResponseHeaderParser.ParseBuffer(byte[] buffer, int bytesReady, ref int bytesConsumed)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize, System.Threading.CancellationToken cancellationToken)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize, System.Threading.CancellationToken cancellationToken)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize, int maxHeaderSize)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content, int bufferSize)
System.Net.Http.Formatting.dll!System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsync(System.Net.Http.HttpContent content)
CacheCow.Client.dll!CacheCow.Client.MessageContentHttpMessageSerializer.DeserializeToResponseAsync(System.IO.Stream stream)
@pranavkm
Copy link
Contributor

pranavkm commented Feb 9, 2022

We think a fix here might be to update https://github.com/aspnet/AspNetWebStack/blob/main/src/System.Net.Http.Formatting/Formatting/Parsers/HttpResponseHeaderParser.cs#L52 to use the 2nd constructor that turns of HTTP header validation as part of deserialization.

Alternatively a narrower fix might be to special case the Expires header.

@pranavkm
Copy link
Contributor

pranavkm commented Mar 2, 2022

Looks like this was previously addressed as part of 3.2.8: https://github.com/aspnet/AspNetWebStack/blob/release/3.2.8/src/System.Net.Http.Formatting/Formatting/Parsers/InternetMessageFormatHeaderParser.cs#L321-L326. I hasn't noticed this since main doesn't yet have this change. Marking this issue as resolved.

FYI @dougbu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants