Skip to content

Chunked request parsing does not properly update examined #8318

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

Merged
merged 2 commits into from
Mar 8, 2019

Conversation

davidfowl
Copy link
Member

Chunked request parsing does not properly update examined

  • The chunked parsing logic didn't properly update the examined position when parsing the chunked prefix. This started to throw because Pipe now throws if examined is set to the position before the previous.

- The chunked parsing logic didn't properly update the examined position when parsing the chunked prefix. This started to throw because Pipe now throws if examined is set to the position before the previous.
@pakrym
Copy link
Contributor

pakrym commented Mar 8, 2019

https://media0.giphy.com/media/xUPGcrdDf64Leq5lOo/giphy.gif

@@ -323,6 +323,9 @@ private void ParseChunkedPrefix(ReadOnlySequence<byte> buffer, out SequencePosit
return;
}

// Assigned this before calculating the chunk size since that can throw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try-finally ?

@@ -360,6 +363,9 @@ private void ParseChunkedPrefix(ReadOnlySequence<byte> buffer, out SequencePosit
ch1 = ch2;
}

// Set examined so that we capture the progress that way made
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We

@davidfowl davidfowl merged commit bd6faa5 into master Mar 8, 2019
@halter73 halter73 deleted the davidfowl/broken-kestrel branch March 8, 2019 19:36
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

Successfully merging this pull request may close these issues.

4 participants