Consume chunked request fully by benaadams · Pull Request #619 · aspnet/KestrelHttpServer

benaadams · 2016-02-08T09:28:37Z

Chunked requests have an extra \r\n at end

5\r\n
Hello\r\n
6\r\n
World\r\n
0\r\n
... Trailer headers ...
\r\n

Resolves #617

@Tratcher is this correct?

/cc @CesarBS @halter73

Please note changes in tests

cesarblum · 2016-02-08T15:58:05Z

+                    else
+                    {
+                        // Post request headers
+                        throw new NotImplementedException("INVALID REQUEST FORMAT");


I don't think NotImplementedException is the right exception to throw here. InvalidOperationException seems more appropriate.

This though could be valid http (trailing headers)

Oh, if you're not going to process incoming trailer headers you should drain them and file a bug to deal with them later.

benaadams · 2016-02-08T18:40:07Z

Resolved issues. @CesarBS question on whether to go NotImplementedException vs InvalidOperationException for unsupported but valid features (e.g. Chunked-extensions & Trailing headers)

halter73 · 2016-02-08T19:47:00Z

+                    }
+                    else if (ch1 == '\r' && ch2 == '\n')
+                    {
+                        input.ConsumingComplete(scan, scan);


ConsumingComplete should be called in a finally block. We're not consistent about doing that in this class (I'll fix that soon), but that is the recommended calling pattern.

I think the latest commit fixes this - but I can't compile it due to

Unable to resolve Microsoft.DotNet.CoreHost (>= 0.0.1-beta-00001) for DNXCore,Version=v5.0

cesarblum · 2016-02-08T21:27:40Z


+            private static void ThrowInvalidFormat()
+            {
+                throw new InvalidOperationException("INVALID REQUEST FORMAT");


Can you switch to lowercase? Not sure why it was all uppercase to begin with, but since you're changing this it would be nice to be consistent (and better looking 😄) with the rest.

All caps is LogLevel.CRITICAL! ;-)

benaadams · 2016-02-09T03:25:16Z

Capturing comment hidden in outdated diff

Oh, if you're not going to process incoming trailer headers you should drain them and file a bug to deal with them later.

benaadams · 2016-02-10T04:30:13Z

Now parses the trailing headers

benaadams · 2016-02-10T06:41:19Z

Test for parsing headers

halter73 · 2016-02-10T18:35:25Z

+
+                if (requestsReceived < requestCount)
+                {
+                    Assert.Equal(new string('a', requestsReceived), request.Headers["X-Trailer-Header"]);


Cool. Now I'm interested to see if this feature gets used even once in the wild. You should find a way to take advantage of this in one of your games 😉

Might be a stretch even for me 😉

Linked to discussion from "Web Hypertext Application Technology Working Group" (whatwg) in https://github.com/aspnet/KestrelHttpServer/issues/622 - think it needs a custom client rather than browser.

halter73 · 2016-02-23T08:50:37Z

Do you see these errors? If not, I can investigate tomorrow.

Microsoft.AspNetCore.Server.KestrelTests.ChunkedRequestTests.InvalidLengthResultsIn500(testContext: TestServiceContext { App = null, AppLifetime = LifetimeNotImplemented { ApplicationStarted = (throws NotImplementedException), ApplicationStopped = (throws NotImplementedException), ApplicationStopping = (throws NotImplementedException) }, DateHeaderValueManager = TestDateHeaderValueManager { }, FrameFactory = null, Log = TestKestrelTrace { }, ... }) [FAIL]
  System.IO.IOException : Unable to write data to the transport connection: An established connection was aborted by the software in your host machine.
  ---- System.Net.Sockets.SocketException : An established connection was aborted by the software in your host machine
  Stack Trace:
       at System.Net.Sockets.NetworkStream.BeginWrite(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state)
       at System.IO.Stream.<>c.<BeginEndWriteAsync>b__53_0(Stream stream, ReadWriteParameters args, AsyncCallback callback, Object state)
       at System.Threading.Tasks.TaskFactory`1.FromAsyncTrim[TInstance,TArgs](TInstance thisRef, TArgs args, Func`5 beginMethod, Func`3 endMethod)
       at System.IO.Stream.BeginEndWriteAsync(Byte[] buffer, Int32 offset, Int32 count)
       at System.IO.Stream.WriteAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken)
       at System.IO.Stream.WriteAsync(Byte[] buffer, Int32 offset, Int32 count)
       at System.IO.StreamWriter.<FlushAsyncInternal>d__68.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
    C:\Users\shalter\dev\Universe\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\TestConnection.cs(51,0): at Microsoft.AspNetCore.Server.KestrelTests.TestConnection.<Send>d__6.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
    C:\Users\shalter\dev\Universe\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\ChunkedRequestTests.cs(366,0): at Microsoft.AspNetCore.Server.KestrelTests.ChunkedRequestTests.<InvalidLengthResultsIn500>d__9.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    ----- Inner Stack Trace -----
       at System.Net.Sockets.Socket.BeginSend(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags, AsyncCallback callback, Object state)
       at System.Net.Sockets.NetworkStream.BeginWrite(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state)
Microsoft.AspNetCore.Server.KestrelTests.ChunkedRequestTests.InvalidSizedDataResultsIn500(testContext: TestServiceContext { App = null, AppLifetime = LifetimeNotImplemented { ApplicationStarted = (throws NotImplementedException), ApplicationStopped = (throws NotImplementedException), ApplicationStopping = (throws NotImplementedException) }, DateHeaderValueManager = TestDateHeaderValueManager { }, FrameFactory = null, Log = TestKestrelTrace { }, ... }) [FAIL]
  System.IO.IOException : Unable to write data to the transport connection: An established connection was aborted by the software in your host machine.
  ---- System.Net.Sockets.SocketException : An established connection was aborted by the software in your host machine
  Stack Trace:
       at System.Net.Sockets.NetworkStream.BeginWrite(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state)
       at System.IO.Stream.<>c.<BeginEndWriteAsync>b__53_0(Stream stream, ReadWriteParameters args, AsyncCallback callback, Object state)
       at System.Threading.Tasks.TaskFactory`1.FromAsyncTrim[TInstance,TArgs](TInstance thisRef, TArgs args, Func`5 beginMethod, Func`3 endMethod)
       at System.IO.Stream.BeginEndWriteAsync(Byte[] buffer, Int32 offset, Int32 count)
       at System.IO.Stream.WriteAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken)
       at System.IO.Stream.WriteAsync(Byte[] buffer, Int32 offset, Int32 count)
       at System.IO.StreamWriter.<FlushAsyncInternal>d__68.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
    C:\Users\shalter\dev\Universe\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\TestConnection.cs(51,0): at Microsoft.AspNetCore.Server.KestrelTests.TestConnection.<Send>d__6.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
    C:\Users\shalter\dev\Universe\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\ChunkedRequestTests.cs(413,0): at Microsoft.AspNetCore.Server.KestrelTests.ChunkedRequestTests.<InvalidSizedDataResultsIn500>d__10.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
    ----- Inner Stack Trace -----
       at System.Net.Sockets.Socket.BeginSend(Byte[] buffer, Int32 offset, Int32 size, SocketFlags socketFlags, AsyncCallback callback, Object state)
       at System.Net.Sockets.NetworkStream.BeginWrite(Byte[] buffer, Int32 offset, Int32 size, AsyncCallback callback, Object state)
Finished:    Microsoft.AspNetCore.Server.KestrelTests
=== TEST EXECUTION SUMMARY ===
Microsoft.AspNetCore.Server.KestrelTests  Total: 359, Errors: 0, Failed: 2, Skipped: 2, Time: 10.361s

benaadams · 2016-02-23T09:24:07Z

On and off - get it with the other closing tests also (e.g. Http10ContentLength). Normally seems to happen if I run the tests repeatedly rather than in clean cmd - perhaps lingering ports?

Can wrap in a IOException catch; but that obv misses the point a little. Think it must be either sending the error post start; or the SendEnd chops the connection prior to flushing sometimes?

Couldn't get to the bottom of it though :(

benaadams · 2016-02-23T09:36:08Z

However... Kestrel should probably also abort the connection for those tests; which cause the issue also. (As they will have poisoned the connection)

benaadams · 2016-02-23T10:23:50Z

K should now be returning 400 and chopping the connection; not getting the earlier exception either - might have been overflow junk into the next request causing the connection to chop?

benaadams · 2016-02-23T10:42:48Z

Hmm, not perfect; if user code caught the exception and threw it away it would still go wrong...

benaadams · 2016-02-23T11:12:16Z

Should still close connection even if user code handles it now.

benaadams · 2016-02-23T11:33:36Z

@halter73 that error is caused by Consume throwing the exception on read; then chopping the connection and the sender tying to continue to send the rest of the request.

Issues resolved.

benaadams · 2016-02-26T22:25:40Z

Rebased, better bad request handling for headers and invalid content lengths

benaadams · 2016-02-26T23:53:44Z

Re-rebased for newest; resolving errors for newer tests.

benaadams · 2016-02-27T12:59:01Z

I'm getting an failure in the cctor for MessageBody.ForChunkedEncoding of Invalid Program exception on full framework.

If I move private static Vector<byte> _vectorCRs = new Vector<byte>((byte)'\r'); into MessageBody then every frame test fails with same error. Bit odd...

benaadams · 2016-02-27T13:31:15Z

AppVeyor:

System.Diagnostics.Debug+DebugAssertException: Block being garbage collected instead of returned to pool 
   at System.Environment.GetStackTrace(Exception e, Boolean needFileInfo)

dnfclas added the cla-already-signed label Feb 8, 2016

cesarblum reviewed Feb 8, 2016
View reviewed changes

halter73 reviewed Feb 8, 2016
View reviewed changes

muratg added this to the 1.0.0-rc2 milestone Feb 8, 2016

muratg assigned ajcvickers and cesarblum and unassigned ajcvickers Feb 8, 2016

cesarblum reviewed Feb 8, 2016
View reviewed changes

benaadams force-pushed the chunked-request branch from bc3cfff to 6e616c9 Compare February 10, 2016 04:29

benaadams force-pushed the chunked-request branch from 3347162 to 7b029a2 Compare February 10, 2016 05:29

benaadams force-pushed the chunked-request branch 2 times, most recently from f5731fd to 4d2a886 Compare February 10, 2016 12:50

halter73 reviewed Feb 10, 2016
View reviewed changes

benaadams changed the title ~~Consume chunked request fully~~ [Needs Tests] Consume chunked request fully Feb 12, 2016

benaadams mentioned this pull request Feb 18, 2016

System.IO.InvalidDataException in Kestrel.Http.MessageBody when posting large file #520

Closed

benaadams force-pushed the chunked-request branch from f7baf73 to 889e6ab Compare February 23, 2016 10:21

This was referenced Feb 23, 2016

Connection not terminated for invalid/ill-formed requests #181

Closed

Return different status codes depending on condition #653

Closed

benaadams force-pushed the chunked-request branch 4 times, most recently from a7c27d0 to b82a251 Compare February 26, 2016 22:23

benaadams added 5 commits February 26, 2016 23:48

Consume chunked request fully

8d8176f

Parse chunked trailing headers

e0f7bb0

Move chunked request tests

331d4a8

Drain chunked extensions + refactor

4bfcd7b

TestHelper classes for shared types

48d3c63

benaadams force-pushed the chunked-request branch from b82a251 to 766803c Compare February 27, 2016 12:33

Return 400 for bad requests and close conn

766803c

bug dodge: Invalid Program Excep on Clr

dab1a1f

halter73 merged commit dab1a1f into aspnet:dev Feb 29, 2016

cesarblum assigned halter73 and unassigned cesarblum Mar 2, 2016

benaadams deleted the chunked-request branch May 10, 2016 10:59

Conversation

benaadams commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Feb 9, 2016

Uh oh!

benaadams commented Feb 10, 2016

Uh oh!

benaadams commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 23, 2016

Uh oh!

benaadams commented Feb 26, 2016

Uh oh!

benaadams commented Feb 26, 2016

Uh oh!

benaadams commented Feb 27, 2016

Uh oh!

benaadams commented Feb 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants