Repository navigation
reject out-of-range AMF numbers in ReadAMFUint32 - #3593
Merged
Merged
Conversation
The AMF0 number is a peer-controlled double; casting NaN or values outside [0, UINT32_MAX] to uint32_t is undefined behavior (UBSan float-cast-overflow) and decodes to different ids per platform. Validate the range before the cast.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The range check rejects fractional boundary values whose conversions remain defined, introducing an unintended compatibility change.
1 open finding
What changed in this PR
Adds validation to prevent undefined AMF number-to-uint32_t conversions and regression coverage.
Changes:
- Rejects NaN and unsafe numeric values before casting.
- Adds boundary and non-finite AMF tests.
| File | Description |
|---|---|
src/brpc/amf.cpp |
Validates numbers before conversion. |
test/brpc_rtmp_unittest.cpp |
Tests accepted and rejected values. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+395
to
+397
| // The number comes from the peer and may be NaN or out of [0, UINT32_MAX], | ||
| // for which the conversion below is undefined. The check is false for NaN. | ||
| if (!(d >= 0.0 && d <= 4294967295.0)) { |
Contributor
|
LGTM |
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 problem does this PR solve?
Issue Number: none
Problem Summary: ReadAMFUint32 in src/brpc/amf.cpp casts a peer-controlled AMF0 number straight to uint32_t. For NaN or any value outside [0, UINT32_MAX] that conversion is undefined (UBSan float-cast-overflow), and in practice the same wire bytes decode to different ids on x86-64 (truncating cvttsd2si) and aarch64 (saturating fcvtzu). The function reads the TransactionId of every RTMP command message (connect, createStream, play, ...) and the stream id in createStream results, so any RTMP peer controls the double.
What is changed and the side effects?
Changed: ReadAMFUint32 rejects non-finite and out-of-range numbers before the cast, returning false like the existing malformed-stream paths. Added a regression test in test/brpc_rtmp_unittest.cpp that fails on the unpatched tree.
Side effects:
Performance effects: none, one range check per uint32 read.
Breaking backward compatibility: none, every number that has a uint32 representation decodes as before.
Check List: