C#: fix IndexOutOfRangeException in ReadRawByte on truncated messages

[pawlos]

Summary
A near-int.MaxValue length varint overflows PushLimit, corrupting bufferSize to a negative value. ReadRawByte's == guard then never triggers RefillBuffer, causing an out-of-bounds read instead of InvalidProtocolBufferException.TruncatedMessage().

Fix
change == to >= in ReadRawByte. Regression tests added for all four affected slow-path variants.

Tests
Added TruncatedMessageWithLargeInnerLengthThrowsInvalidProtocolBufferException with 4 test cases.

Fixes #26856

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[jskeet]

I'm not on the protobuf team and don't have time to review this right now. I'll get to it at some point if I can, but it won't be imminently.

[jskeet]

I've had a quick look at this now, and I'm not yet convinced the fix is right - if the bufferPos is greater than bufferSize, we've already done something wrong. Will try to reproduce locally with the new tests, and see if I can understand it better.

[pawlos]

I've had a quick look at this now, and I'm not yet convinced the fix is right - if the bufferPos is greater than bufferSize, we've already done something wrong. Will try to reproduce locally with the new tests, and see if I can understand it better.

You're right. It probably just masks the real issue. I'll have another look too.

[jskeet]

SegmentedBufferHelper.PushLimit calls SegmentedBufferHelper.RecomputeBufferSizeAfterLimit which is changing state.bufferSize to a negative value, because we "think" the length is so huge (so it overflows). Changing that to a checked block triggers an OverflowException instead - which suggests this is indeed the right place to look.

Stack of where I think things are going wrong:

• SegmentedBufferHelper.RecomputeBufferSizeAfterLimit
• SegmentedBufferHelper.PushLimit
• ParsingPrimitivesMessages.ReadMessage
• ParseContext.ReadMessage

I wasn't involved in writing SegmentedBufferHelper or ParserInternalState, so I'm less confident in working out what we should do here. Will need to spend some time understanding it better - but I'm pretty sure the fix should be in one of the first two methods.

[pawlos]

I wasn't involved in writing SegmentedBufferHelper or ParserInternalState, so I'm less confident in working out what we should do here. Will need to spend some time understanding it better - but I'm pretty sure the fix should be in one of the first two methods.

Thanks for the pointer @jskeet! I've pushed a new commit that does the PushLimit arithmetic in long so state.currentLimit can't go negative, and reverted the >= guard in ReadRawByte. Went with widening over checked {} to keep the existing TruncatedMessage() exception path - hope this addresses the underlying issue.

[jskeet]

Possibly add a comment to explain why we're doing this? (It looks right to me though.)

[pawlos]

@jskeet added

[jskeet]

That's great, thanks. I'll add another reviewer from the protobuf team.

[jskeet]

@mkruskal-google This looks good to me, and has appropriate tests.