Reject negative data length in ExchangeCodec decode - #16448
Open
L1nq0 wants to merge 1 commit into
Open
Conversation
A negative data length can never become a valid frame, so returning NEED_MORE_INPUT would leave the channel waiting forever. Throw an IOException where the length is read instead. Same change in the deprecated Codec-path copy, which CodecAdapterTest runs through the old interface.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16448 +/- ##
============================================
+ Coverage 60.91% 60.93% +0.01%
- Complexity 15 11767 +11752
============================================
Files 1953 1953
Lines 89271 89273 +2
Branches 13473 13474 +1
============================================
+ Hits 54383 54402 +19
+ Misses 29309 29290 -19
- Partials 5579 5581 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change?
Closes #16447
A frame header that declares a negative data length is not a valid frame, and no additional input can make it one. On the 3.3 branch such a header passes every check in ExchangeCodec.decode: finishRespWhenOverPayload only compares against the payload upper bound, and the completeness check readable < len + 16 holds for every negative len. The negative value then reaches the ChannelBufferInputStream constructor, which rejects it with an IllegalArgumentException that escapes decode.
Stack trace from a 16-byte frame with data length ffffffff:
What this PR does
decode now checks the sign of len right after Bytes.bytes2int(header, 12) and rejects a negative value with an IOException, so an invalid header is handled as a decode-level protocol error at the point the length is read instead of surfacing as an unexpected exception from a stream constructor. On the netty4 transport the connection-level behavior is unchanged: the exception is caught by InternalDecoder, logged, and the connection is closed.
NEED_MORE_INPUT is deliberately not used here. It is only correct when more input can complete the frame, and a negative length can never be completed.
The same guard is added to DeprecatedExchangeCodec in test sources: CodecAdapterTest re-runs the ExchangeCodecTest suite through the old Codec interface, so the copy needs the same behavior to keep the two paths aligned.
Testing
A new test, test_Decode_Negative_Data_Length, feeds the exact 16-byte reproducer from the issue (magic dabb, flag c2, requestId all ff, data length ffffffff) and asserts that decode rejects it with an IOException. Without the fix this test fails with the stack above.
The full dubbo-remoting-api module suite (385 tests) and the dubbo-remoting-netty4 module pass locally.
Checklist