HPACK decoder fixes #39907

scalablecory · 2019-07-31T00:23:09Z

Fix HPACK decoder failing inconsistently with 0-length header names. Fail fast with same exception message that HTTP1 uses. Resolves #39873.
Fix DynamicTable returning incorrect values or throwing exceptions when it wraps its ring buffer. Resolves #39656.
Moves HPACK encoder methods from Http2LoopbackConnection into their own class.
Implement all forms of header encodings in HPACK encoder.
Move a number of files where they belong into the ProductionCode folder in unit test project.

…tests. Add support for headers beyond "literal+literal without indexing".

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs

stephentoub · 2019-07-31T00:32:51Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs

@@ -265,6 +270,7 @@ public void Decode(ReadOnlySpan<byte> data, bool endHeaders, HeaderCallback onHe
                    case State.HeaderNameLengthContinue:
                        if (_integerDecoder.Decode(b))
                        {
+                            Debug.Assert(_integerDecoder.Value != 0, "HPACK integer decoder failed to stop an overlong 0.");


What does "an overlong 0" mean?

over long encoding. the integer 0 should take only a single byte, any more and IntegerDecoder will throw an exception.

I'm still having trouble parsing this. By "over long", do you mean "too long" / "too many bytes"?

Not trying to nitpick, I just wouldn't know how to react if this assert failed.

It's terminology that came to me via UTF-8 security issues and I'm realizing now that it may not be common knowledge. I will update the assert message with something else.

Overlong encoding means an integer was encoded in more bytes than was required. IntegerDecoder does not allow this. The assert is there mostly as documentation that we don't need to check the 0 case on continuation bytes because it is prevented elsewhere.

I see. Wasn't familiar with the term. Thanks for the explanation.

scalablecory · 2019-07-31T17:00:15Z

/azp run

azure-pipelines · 2019-07-31T17:00:39Z

Azure Pipelines successfully started running 4 pipeline(s).

scalablecory · 2019-07-31T21:06:49Z

/azp run

azure-pipelines · 2019-07-31T21:07:11Z

Azure Pipelines successfully started running 4 pipeline(s).

scalablecory · 2019-08-01T00:22:15Z

Can't seem to get a reliable CI run today. Guess I'll try later once everyone is asleep :).

JamesNK · 2019-08-01T00:25:27Z

Are these changes to code from ASP.NET Core HPACK? Should they be mirrored in aspnetcore? // @Tratcher

scalablecory · 2019-08-01T03:44:09Z

@JamesNK yes it looks like both of these fixes willy apply to the ASP.NET version.

Tratcher · 2019-08-01T05:26:10Z

Fix DynamicTable returning incorrect values or throwing exceptions when it wraps its ring buffer. Resolves #39656.

Ug, I spent all day today debugging that. Amusingly our fixes look identical. dotnet/aspnetcore#12782

scalablecory · 2019-08-01T09:00:01Z

/azp run

azure-pipelines · 2019-08-01T09:00:27Z

Azure Pipelines successfully started running 4 pipeline(s).

scalablecory · 2019-08-01T17:53:54Z

@stephentoub @dotnet/ncl any further review?

CI still failing in unrelated places. This is not platform-specific code so I think we're okay there as one of the outerloop runs got through successfully. @dotnet/dnceng @ViktorHofer @safern can anything be done about this?

wfurt

LGTM.

MattGal · 2019-08-01T18:42:09Z

@scalablecory - from a DncEng perspective, the only suspicious bit I see is the OSX legs where it seems the machines may not be properly interactively logged in with unlocked keychains; The other failures look like flaky tests (the system.net one hitting an HTTP Client timeout is classic flaky test behavior) or just a plain bug causing fact failure (FileSystemWatcher ones)

I'm checking out some of the macs and will discuss with folks about this; I believe many of them were recently reimaged and configuration might be subtly affected.

MattGal · 2019-08-01T18:52:55Z

@scalablecory one more update; I don't see anything specifically wrong about the machines your OSX work ran on w.r.t. interactive login or keychains, but @ilyas1974 is going to take a peek as well later.

Thanks for bringing this to our attention!

scalablecory · 2019-08-01T19:05:25Z

Thanks @MattGal

ilyas1974 · 2019-08-01T21:19:19Z

It doing some investigation, it appears that they way Apple addresses the Key Chain functionality changed between 10.11/12 and 10.13/14. The configuration directory within /Library/Keychain no longer exists (where the permissions were originally changed in previous versions). As there is nothing to modify permissions on, the changes that we previous used do not apply.

stephentoub · 2019-08-01T22:24:19Z

src/Common/tests/System/Net/Http/HPackEncoder.cs

+        /// </summary>
+        NeverIndexed = 8
+    }
+}


Is this a second implementation of hpack encoding, separate from the product implementation?

Hmm, I see, you consolidated existing test code, rather than writing it from scratch as part of this PR. Ok.

stephentoub

The product fix LGTM.

…. Fail fast with same exception message that HTTP1 uses. Resolves #39873. Partial port of dotnet#39907 from master.

ViktorHofer · 2019-08-07T09:56:53Z

@ViktorHofer @safern can anything be done about this?

We have been actively working on disabling/fixing flaky tests for quite some time. Our goal is to stabilize CI and rolling builds by end of the year latest. We have measurements in place that will help us achieve that goal.

…0-length header names (#40042) Fix HPACK decoder failing inconsistently with 0-length header names. Fail fast with same exception message that HTTP1 uses. Resolves #39873. Partial port of #39907 from master.

- Fix HPACK decoder failing inconsistently with 0-length header names. Fail fast with same exception message that HTTP1 uses. Resolves dotnet/corefx#39873. - Fix `DynamicTable` returning incorrect values or throwing exceptions when it wraps its ring buffer. Resolves dotnet/corefx#39656. - Moves HPACK encoder methods from `Http2LoopbackConnection` into their own class. - Implement all forms of header encodings in HPACK encoder. - Move a number of files where they belong into the ProductionCode folder in unit test project. Commit migrated from dotnet/corefx@b7e60c9

Cory Nelson added 2 commits July 30, 2019 16:48

Move test HPACK encoder into a new class so it can be reused in unit …

d45697a

…tests. Add support for headers beyond "literal+literal without indexing".

Fix wrapping in DynamicTable and add a unit test.

23d206e

scalablecory added bug Product bug (most likely) area-System.Net.Http.SocketsHttpHandler labels Jul 31, 2019

scalablecory added this to the 5.0 milestone Jul 31, 2019

scalablecory requested review from stephentoub and a team July 31, 2019 00:23

scalablecory self-assigned this Jul 31, 2019

stephentoub reviewed Jul 31, 2019

View reviewed changes

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs Show resolved Hide resolved

stephentoub reviewed Jul 31, 2019

View reviewed changes

Comment ring buffer.

ae538a4

Improve overlong assert

bdeb776

karelz removed the bug Product bug (most likely) label Aug 1, 2019

wfurt approved these changes Aug 1, 2019

View reviewed changes

stephentoub reviewed Aug 1, 2019

View reviewed changes

stephentoub approved these changes Aug 1, 2019

View reviewed changes

scalablecory merged commit b7e60c9 into dotnet:master Aug 2, 2019

scalablecory mentioned this pull request Aug 5, 2019

[release/3.0] HTTP/2 - fix HPACK decoder failing inconsistently with 0-length header names #40042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPACK decoder fixes #39907

HPACK decoder fixes #39907

scalablecory commented Jul 31, 2019

stephentoub Jul 31, 2019

scalablecory Jul 31, 2019

stephentoub Jul 31, 2019 •

edited

Loading

scalablecory Jul 31, 2019

stephentoub Jul 31, 2019

scalablecory commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

scalablecory commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

scalablecory commented Aug 1, 2019

JamesNK commented Aug 1, 2019

scalablecory commented Aug 1, 2019

Tratcher commented Aug 1, 2019

scalablecory commented Aug 1, 2019

azure-pipelines bot commented Aug 1, 2019

scalablecory commented Aug 1, 2019

wfurt left a comment

MattGal commented Aug 1, 2019

MattGal commented Aug 1, 2019

scalablecory commented Aug 1, 2019

ilyas1974 commented Aug 1, 2019

stephentoub Aug 1, 2019

stephentoub Aug 1, 2019

stephentoub left a comment

ViktorHofer commented Aug 7, 2019

HPACK decoder fixes #39907

HPACK decoder fixes #39907

Conversation

scalablecory commented Jul 31, 2019

stephentoub Jul 31, 2019

Choose a reason for hiding this comment

scalablecory Jul 31, 2019

Choose a reason for hiding this comment

stephentoub Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

scalablecory Jul 31, 2019

Choose a reason for hiding this comment

stephentoub Jul 31, 2019

Choose a reason for hiding this comment

scalablecory commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

scalablecory commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

scalablecory commented Aug 1, 2019

JamesNK commented Aug 1, 2019

scalablecory commented Aug 1, 2019

Tratcher commented Aug 1, 2019

scalablecory commented Aug 1, 2019

azure-pipelines bot commented Aug 1, 2019

scalablecory commented Aug 1, 2019

wfurt left a comment

Choose a reason for hiding this comment

MattGal commented Aug 1, 2019

MattGal commented Aug 1, 2019

scalablecory commented Aug 1, 2019

ilyas1974 commented Aug 1, 2019

stephentoub Aug 1, 2019

Choose a reason for hiding this comment

stephentoub Aug 1, 2019

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

ViktorHofer commented Aug 7, 2019

stephentoub Jul 31, 2019 •

edited

Loading