Fix a race in buffer usage of Http2Connection.ProcessPingFrame by scalablecory · Pull Request #39242 · dotnet/corefx

scalablecory · 2019-07-05T23:06:51Z

Incoming buffer might be reused before we've sent it. Resolve #38788.

…ing buffer might be reused before we've sent it. Resolve #38788.

scalablecory · 2019-07-05T23:07:02Z

/azp run

azure-pipelines · 2019-07-05T23:07:21Z

Azure Pipelines successfully started running 4 pipeline(s).

geoffkizer · 2019-07-05T23:12:25Z

            // Send PING ACK
            // Don't wait for completion, which could happen asynchronously.
-            LogExceptions(SendPingAckAsync(_incomingBuffer.ActiveMemory.Slice(0, FrameHeader.PingLength)));
+            byte[] pingContent = _incomingBuffer.ActiveMemory.Slice(0, FrameHeader.PingLength).ToArray();


I think we could just do BinaryPrimitives.ReadInt64BigEndian or whatever it is, and avoid an alloc here.

I considered it, but the rarity of PING makes me feel that this is not a worthwhile micro-optimization given that we'd then need to document endianness.

we'd then need to document endianness.

Why? Wouldn't this just be roundtripping the data? As long as you used routines for converting to and from a long that employed the same endianness, it wouldn't be visible. You can also use BitConverter.ToInt64/TryWriteBytes, which just uses the machine's endianness and won't force any reversals.

When it doesn't matter, like in this case, I'd rather reduce complexities and have obvious code than need to comment something.

As long as you used routines for converting to and from a long that employed the same endianness, it wouldn't be visible. You can also use BitConverter.

Either way, this is going to need comments. If you know HTTP2 and know these are just opaque bytes and not an integer, you'll find it suspicious to see an endian-specific read. Even if you didn't know HTTP2, if you see BitConverter being used to read an integer in networking code you'd immediately flag it as a possible bug.

Either way, this is going to need comments

It needs comments even with ToArray. Someone reviewing the code in a year needs to know that this wasn't an oversight and what race conditions it's working around. Otherwise it's very likely to be reverted by someone trying to remove allocations.

When it doesn't matter, like in this case, I'd rather reduce complexities and have obvious code

It shouldn't be any more, or any more complicated, code. It's ToInt64 instead of ToArray, and TryWriteBytes instead of CopyTo. If anything it's arguably clearer code, because it makes the payload size crystal clear in the method contract, whereas today it appears to accept arbitrarily-sized data.

I assume pings are super uncommon, so from a perf perspective it probably doesn't matter. But at least from my perspective the code would be cleaner with a long, and so may as well get the perf benefit, too.

Up to you. If you choose not to switch, please do add a detailed comment about why ToArray is necessary.

We already read a bunch of stuff out of frames as big endian, so I don't see that adding one more here is much different.

karelz · 2019-07-10T21:17:48Z

What's blocking this PR from merge? Can we get it done by EOW?

scalablecory · 2019-07-10T22:46:04Z

What's blocking this PR from merge? Can we get it done by EOW?

It'll be ready today.

geoffkizer · 2019-07-11T03:01:43Z

            // Send PING ACK
            // Don't wait for completion, which could happen asynchronously.
-            LogExceptions(SendPingAckAsync(_incomingBuffer.ActiveMemory.Slice(0, FrameHeader.PingLength)));
+            // We discard the the frame from the incoming buffer, possibly before


Not sure what this comment means?

Generally speaking, all these routines read the frame completely before returning. So I don't think you need to comment specifically on that.

geoffkizer

Aside from clarifying the comment, LGTM

geoffkizer · 2019-07-11T03:04:49Z

+            // We discard the the frame from the incoming buffer, possibly before
+            // SendPingAckAsync is completed, so we need to save our own copy of the data.
+            // The data is not an integer in the RFC, but this will avoid an array allocation.
+            long pingContent = BinaryPrimitives.ReadInt64BigEndian(_incomingBuffer.ActiveSpan.Slice(0, FrameHeader.PingLength));


If anything, move this line to above the "Send PING ACK" comment above, since that comment is specifically commenting on the call to SendPingAckAsync

geoffkizer · 2019-07-11T23:34:31Z

            // Don't wait for completion, which could happen asynchronously.
            LogExceptions(SendPingAckAsync(_incomingBuffer.ActiveMemory.Slice(0, FrameHeader.PingLength)));

+            // SendPingAckAsync copies the buffer for us, so it is safe to not wait for completion.


I think I read this code wrong when I initially reviewed it...

It seems better to just read the pingContent into a long here, in ProcessPingFrame, and then pass the resulting long into SendPingAckAsync. That's generally how these Process*Frame routines work, they read whatever data they need out of the frame. And it's more obviously correct.

By reading the integer in SendPingAckAsync, it avoids depending on ProcessPingFrame (to have the same endianness scheme).

I agree that consistency with other Process*Frame is better here, so I've moved the read into ProcessPingFrame and added comments to note the dependency.

geoffkizer · 2019-07-11T23:36:10Z


+            // Copy pingContent before we go async so the caller can
+            // discard their buffer without waiting for us to complete.
+            long pingContentLong = BitConverter.ToInt64(pingContent.Span);


Is there a particular reason to use BitConverter here, as opposed to BinaryPrimitives.ReadUInt64BigEndian? We use BinaryPrimitives elsewhere in the code. Seems like we should be consistent.

I actually haven't found any instances of BinaryPrimitives being used to shuttle opaque bytes, so this isn't a consistency issue. It made sense to avoid byte shuffling when endianness concerns could be kept to a single small method.

geoffkizer · 2019-07-12T08:57:51Z

LGTM, thanks.

Fix a race in buffer usage of Http2Connection.ProcessPingFrame Commit migrated from dotnet/corefx@1d03a91

Fix a race in buffer usage of Http2Connection.ProcessPingFrame: incom…

1a2c478

…ing buffer might be reused before we've sent it. Resolve #38788.

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

scalablecory added this to the 3.0 milestone Jul 5, 2019

scalablecory requested a review from a team July 5, 2019 23:06

scalablecory self-assigned this Jul 5, 2019

geoffkizer reviewed Jul 5, 2019

View reviewed changes

geoffkizer reviewed Jul 11, 2019

View reviewed changes

geoffkizer approved these changes Jul 11, 2019

View reviewed changes

geoffkizer reviewed Jul 11, 2019

View reviewed changes

Optimize out an array allocation

69b5793

scalablecory force-pushed the 38788-ping-race branch from 3117da3 to 69b5793 Compare July 11, 2019 03:07

scalablecory and others added 2 commits July 10, 2019 20:19

Add comment for Discard.

fd83c71

Capitalize comment.

484aa64

geoffkizer reviewed Jul 11, 2019

View reviewed changes

Move integer read into ProcessPingFrame.

790f537

geoffkizer merged commit 1d03a91 into dotnet:master Jul 12, 2019

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#39242 from scalablecory/38788-ping-race

2fbfaa8

Fix a race in buffer usage of Http2Connection.ProcessPingFrame Commit migrated from dotnet/corefx@1d03a91

Conversation

scalablecory commented Jul 5, 2019

Uh oh!

scalablecory commented Jul 5, 2019

Uh oh!

azure-pipelines Bot commented Jul 5, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karelz commented Jul 10, 2019

Uh oh!

scalablecory commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Jul 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants