CacheCow icon indicating copy to clipboard operation
CacheCow copied to clipboard

Crash when deserializing a HttpResponseMessage on .NET Core

Open JimiSmith opened this issue 6 years ago • 33 comments

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 avatar Aug 23 '18 11:08 JimiSmith

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

cache-cow-http-response-crash.txt

JimiSmith avatar Aug 23 '18 11:08 JimiSmith

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

aliostad avatar Aug 23 '18 15:08 aliostad

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

aliostad avatar Aug 23 '18 18:08 aliostad

Thanks :) And thanks for the great library

JimiSmith avatar Aug 23 '18 19:08 JimiSmith

Re-opening to keep track. The issue currently is here: https://github.com/aspnet/AspNetWebStack/issues/193#issuecomment-418529386

aliostad avatar Sep 05 '18 19:09 aliostad

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.

aliostad avatar Nov 16 '18 13:11 aliostad

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.

pietervdvn avatar Nov 23 '18 20:11 pietervdvn

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

aliostad avatar Nov 24 '18 18:11 aliostad

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

pietervdvn avatar Nov 26 '18 10:11 pietervdvn

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

aliostad avatar Dec 21 '18 10:12 aliostad

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.<DeserializeToResponseAsync>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.<SendAsync>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.<SendAsync>d__24.MoveNext()" }`

alibaghernejad avatar Apr 15 '19 07:04 alibaghernejad

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

aliostad avatar Apr 15 '19 08:04 aliostad

@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?

alibaghernejad avatar Apr 29 '19 12:04 alibaghernejad

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 avatar Apr 29 '19 14:04 aliostad

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

aliostad avatar Oct 17 '19 17:10 aliostad

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.

wocasella avatar Jun 05 '20 18:06 wocasella

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.

aliostad avatar Jun 08 '20 10:06 aliostad

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 avatar Aug 03 '21 08:08 foresightyj

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!

foresightyj avatar Aug 03 '21 08:08 foresightyj

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

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

Daniel-Svensson avatar Jan 23 '22 11:01 Daniel-Svensson

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

Daniel-Svensson avatar Mar 02 '22 19:03 Daniel-Svensson

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.

EamonHetherton avatar Jan 23 '23 02:01 EamonHetherton

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

ricardoreais avatar Mar 15 '23 11:03 ricardoreais

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

esskar avatar Feb 24 '24 16:02 esskar

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??

aliostad avatar Feb 25 '24 08:02 aliostad

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

esskar avatar Feb 25 '24 12:02 esskar

@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?

aliostad avatar Feb 25 '24 13:02 aliostad

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.

esskar avatar Feb 25 '24 20:02 esskar

@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.

aliostad avatar Feb 26 '24 08:02 aliostad

... 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.

loraderon avatar Feb 26 '24 08:02 loraderon