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 #213

Open
JimiSmith opened this issue Aug 23, 2018 · 33 comments
Open

Crash when deserializing a HttpResponseMessage on .NET Core #213

JimiSmith opened this issue Aug 23, 2018 · 33 comments
Assignees

Comments

@JimiSmith
Copy link

On .NET Core (I've tested 2.0 and 2.1) calling DeserializeToResponseAsync on MessageContentHttpMessageSerializer will crash with certain http responses. (google.com being one of them)

I've tested this on 4.7.1 and the crash does not occur. I'm guessing this may be a framework issue, but I'm not 100% sure.

I've attached a project that reproduces the problem

Exception backtrace

System.InvalidOperationException: Error parsing HTTP message header byte 697 of message System.Byte[].
   at System.Net.Http.HttpContentMessageExtensions.ReadAsHttpResponseMessageAsyncCore(HttpContent content, Int32 bufferSize, Int32 maxHeaderSize, CancellationToken cancellationToken)
   at CacheCow.Client.MessageContentHttpMessageSerializer.DeserializeToResponseAsync(Stream stream)
   at CacheCowCrashRepro.TestCacheStore.AddOrUpdateAsync(CacheKey key, HttpResponseMessage response) in C:\Users\james\Documents\Projects\CacheCowCrashRepro\CacheCowCrashRepro\Program.cs:line 45
   at CacheCow.Client.CachingHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts)
   at CacheCowCrashRepro.Program.Main(String[] args) in C:\Users\james\Documents\Projects\CacheCowCrashRepro\CacheCowCrashRepro\Program.cs:line 19}	System.Exception {System.InvalidOperationException

CacheCowCrashRepro.zip

Thanks

@JimiSmith
Copy link
Author

This is the contents of the serialized HttpResponseMessage. It looks normal to me

cache-cow-http-response-crash.txt

@aliostad aliostad self-assigned this Aug 23, 2018
@aliostad
Copy link
Owner

Interesting problem. Thanks for reporting. I will have a look and get back to you soon.

@aliostad
Copy link
Owner

aliostad commented Aug 23, 2018

It is seems to be a bug in .NET Core 2.1. Works with 2.0. Please follow-up this github issue.

https://github.com/dotnet/corefx/issues/31918

@JimiSmith
Copy link
Author

Thanks :) And thanks for the great library

@aliostad aliostad reopened this Sep 5, 2018
@aliostad
Copy link
Owner

aliostad commented Sep 5, 2018

Re-opening to keep track. The issue currently is here:
aspnet/AspNetWebStack#193 (comment)

@aliostad
Copy link
Owner

aliostad commented Nov 16, 2018

There is some breakthrough (finally) with the issue and it is a bug in the .NET Framework from 2.0 to 2.1. Hopefully fixed soon. Thank you for reporting it.

@pietervdvn
Copy link
Contributor

Hey,

While testing and bugfixing, I encountered a similar issue. It turns out that the Server-header is the culprit.

Removing this part of the header with response.Headers.Remove("Server"); is a viable workaround.

I'm gonna report this upstream too.

@aliostad
Copy link
Owner

@pietervdvn thanks for the tip. Issue seems to be in Expires header and waiting for the resolution from Microsoft.

@pietervdvn
Copy link
Contributor

NP. I could parse expire headers just fine (although I'm on dotnetcore 2.0).

@aliostad
Copy link
Owner

Now it is being followed here
https://github.com/dotnet/corefx/issues/31918

@alibaghernejad
Copy link

Have a similar problem in Asp.Net Web API project.
Version: 2.4.3
{ "Message": "An error has occurred.", "ExceptionMessage": "Error parsing HTTP message header byte 13 of message System.Byte[].", "ExceptionType": "System.InvalidOperationException", "StackTrace": " at System.Net.Http.HttpContentMessageExtensions.<ReadAsHttpResponseMessageAsyncCore>d__19.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult()\r\n at CacheCow.Client.MessageContentHttpMessageSerializer.d__5.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n
at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult()\r\n at CacheCow.Client.InMemoryCacheStore.<GetValueAsync>d__8.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n at System.Runtime.CompilerServices.ConfiguredTaskAwaitable1.ConfiguredTaskAwaiter.GetResult()\r\n
at CacheCow.Client.CachingHandler.d__42.MoveNext()\r\n--- End of stack trace from previous location where exception was thrown ---\r\n
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)\r\n
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)\r\n
at System.Web.Http.HttpServer.d__24.MoveNext()"
}`

@aliostad
Copy link
Owner

@alibaghernejad do you mind posting this to Microsoft thread? It is a frustrating situation.
https://github.com/dotnet/corefx/issues/31918#issuecomment-483162502

@alibaghernejad
Copy link

@alibaghernejad do you mind posting this to Microsoft thread? It is a frustrating situation.
dotnet/corefx#31918 (comment)

I had a try to do that today but the state of the issue is closed now.
what's the current state of the Issue?

@aliostad
Copy link
Owner

OK, it is fixed apparently. It will be part of the 3.0 milestone. So let's wait until testing if it is fixed.

@aliostad
Copy link
Owner

Well even .NET Core 3.0 does not seem to fix it so I have got back to them now :(

@wocasella
Copy link

Hi, I'm just hit the same issue in my app on asp.net core 3.1.
@aliostad, is there any update on this?

Thanks.

@aliostad
Copy link
Owner

aliostad commented Jun 8, 2020

The problem is you guys need to go to Microsoft open issue and push them to solve the problem. When I go to them, they say it is a low priority, we do not have enough +1 on it to act quicker.

@foresightyj
Copy link

foresightyj commented Aug 3, 2021

Hi @aliostad, I am using version 2.8.2 of this library with asp.net core 3.1 and encountered the same problem almost 1 year later.

I went to the issue you posted in corefx repo but that issue was closed. What am I going to do to circumvent this problem?

Actually, my project previously were using cachecow 2.8.2 with asp.net core 2.1 and they worked well before. I am migrating my project to asp.net core 3.1 today and encountered this problem.

@foresightyj
Copy link

foresightyj commented Aug 3, 2021

It might help for the rest who have a similar issue. I found a way to bypass this Expires header issue, provided that in your case, it is totally ok to ignore that header.

    public class RemoveExpiresHeaderCacheStoreProxy : ICacheStore
    {
        private readonly ICacheStore _proxied;
        public RemoveExpiresHeaderCacheStoreProxy(ICacheStore proxied)
        {
            _proxied = proxied;
        }

        public Task AddOrUpdateAsync(CacheKey key, HttpResponseMessage response)
        {
            const string EXPIRES_HEADER = "Expires";
            if (response.Content.Headers.Contains(EXPIRES_HEADER))
            {
                response.Content.Headers.Remove(EXPIRES_HEADER);
            }
            return _proxied.AddOrUpdateAsync(key, response);
        }

        public Task ClearAsync()
        {
            return _proxied.ClearAsync();
        }

        public void Dispose()
        {
            _proxied.Dispose();
        }

        public Task<HttpResponseMessage> GetValueAsync(CacheKey key)
        {
            return _proxied.GetValueAsync(key);
        }

        public Task<bool> TryRemoveAsync(CacheKey key)
        {
            return _proxied.TryRemoveAsync(key);
        }
    }

And then you can wrap your original FileStore or memoryStore with:

image

That worked for me. Thanks for the great library!

@Daniel-Svensson
Copy link

I ran into this bug when trying to run our application on .NET 6, so I have tried to open an issue in aspnet/AspNetWebStack#313 to get it fixed there.

If anybody else has more information/details feel free to comment on to the issue

@Daniel-Svensson
Copy link

Daniel-Svensson commented Mar 2, 2022

Seems like they issued a fix in 3.2.8
I don't have the time to verify it at the moment, but it looks promising that it should be solvable by just updating the nuget package, fyi @aliostad

@EamonHetherton
Copy link

EamonHetherton commented Jan 23, 2023

I've run into a similar issue when the ETag is not well formed.

The Http spec says that the ETag header should be quoted and in my case I have a resource that is returning an ETag without quotes.

Putting aside the fact that the server should be returning a well-formed ETag, the root problem here as I see it is that the serialisation and deserialisation are not being done by the same library/code and essentially the deserialisation is less tolerant of malformed headers than the normal code path that creates a HttpResponse from the network stream. When reading from the network stream, the headers are added using "response.Content!.Headers.TryAddWithoutValidation(descriptor, headerValue);", but when deserialised it uses "headers.Add(text, value)".

There are likely to be other ways to trigger this failure with other malformed headers too.

Ironically, the internal class "InternetMessageFormatHeaderParser" that is doing the deserialisation currently has an overload that takes a parameter in the constructor to "ignoreHeaderValidation", but there is nowhere to set that further up the call chain.

I think to fix this, the library needs to take control of the deserialisation rather than delegating to the extension method in the "System.Net.Http.Formatting" library (which I think is legacy anyway?). I'm going to have a look at implementing my own FileStore that uses a new "MessageContentHttpMessageSerializer" that does not depend on the "System.Net.Http.Formatting" library. If I get somewhere I am happy with I'll create a pull request.

@ricardoreais
Copy link

@foresightyj answer worked for me. It was an issue with the cache handler

@esskar
Copy link

esskar commented Feb 24, 2024

Same issue happens in .NET8 ... i am using native-AOT, not clear if it is related. But the RemoveExpiresHeaderCacheStoreProxy does not help here.

@aliostad
Copy link
Owner

Thanks @esskar .

The solution is really to go shout on Microsoft on why it has taken them 6 years and not yet fixed this. Last time I did it, they said it is on you [me] talking about it.

Anyway, the solution is to change the -1 to 0 ion the header. Are you saying this solution does not work??

@esskar
Copy link

esskar commented Feb 25, 2024

I just did some debuging; in my case, it was the ETag does was unable to deserialize.

@aliostad
Copy link
Owner

@esskar value of ETag has been described by HTTP Spec. So I would add some code to change it before-hand.

Can you share whar is the value of ETag?

@esskar
Copy link

esskar commented Feb 25, 2024

Screenshot 2024-02-25 at 21 32 37

That is the internal c# representation; i thing the serialization will not write it with the required quotes.
I agree that the HttpMessageContent is buggy, but it makes it hard to trust cashcow to be integrated when consumers need to work around it.

@aliostad
Copy link
Owner

aliostad commented Feb 26, 2024

@esskar

Dude, you are using wrong ETag value blaming CacheCow??
Up to you to trust or not trust CacheCow, I am not trying to sell you anything. But I expect the courtesy of accepting your own fault here... that format of ETag is invalid. ETag can take two kinds:

  • W/"ETag value"
  • "ETag value"

The double-quotes are required. So you just need to use the correct value. If a system sets bad value outside HTTP spec it is not gonna work - after all this is a library for HTTP Caching which is highly dependent on the spec.

@loraderon
Copy link

loraderon commented Feb 26, 2024

... that format of ETag is invalid. ETag can take two kinds:

  • W/"ETag value"

  • "ETag value"

The double-quotes are required. So you just need to use the correct value. If a system sets bad value outside HTTP spec it is not gonna work - after all this is a library for HTTP Caching which is highly dependent on the spec.

I also ran in to this issue in #293 when trying to cache external resources. But the wrong value is being emitted by someone else so it's impossible to fix it.
I tried to fix it in Microsoft's repo but they just closed the PR saying that the library is in maintenance mode only which is really a bummer.
No shade at all on CacheCow here since the problem is generated on the wild web and MS is not accepting a fix for it.

@aliostad
Copy link
Owner

OK, I will look into an alternate serialisation. I cannot give an ETA now but expect this to be done within the next few weeks.

#298

@esskar
Copy link

esskar commented Feb 26, 2024

@esskar

Dude, you are using wrong ETag value blaming CacheCow?? Up to you to trust or not trust CacheCow, I am not trying to sell you anything. But I expect the courtesy of accepting your own fault here... that format of ETag is invalid. ETag can take two kinds:

  • W/"ETag value"
  • "ETag value"

The double-quotes are required. So you just need to use the correct value. If a system sets bad value outside HTTP spec it is not gonna work - after all this is a library for HTTP Caching which is highly dependent on the spec.

This how C# keeps the value in the HttpResponse. The upstream server correctly sends "etag". The framework stores it without the quotes in the response.Headers. It does not keep the value as "etag" but only etag. Then, when serializing, only the value without the quote gets serialized; and this fails when deserializing.

@aliostad
Copy link
Owner

The framework stores it without the quotes in the response.Headers

@esskar, really?? By framework do you mean .NET? That would be a horrible mess if true but I doubt it is the case. Can you please send a reproduction?

This is an example of a public resource returning strong ETag:

https://developer.mozilla.org/static/media/webview.dc22c0f5614a5ff2d93a.svg

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

No branches or pull requests