guard underflowed value size in couchbase collection responses#3402
Open
ubeddulla wants to merge 1 commit into
Open
guard underflowed value size in couchbase collection responses#3402ubeddulla wants to merge 1 commit into
ubeddulla wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Guards against unsigned underflow when computing the value section size in Couchbase collection-related response parsers, preventing a malformed header from draining the pipelined buffer and desynchronizing the stream.
Changes:
- Compute
value_sizeusing signed arithmetic and reject negative sizes before consuming any bytes inpopCollectionId()andpopManifest(). - Add a regression test covering inconsistent
total_body_lengthvsextras_length + key_lengthfor both parsers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/brpc/couchbase.cpp |
Adds negative-size guards to prevent underflow-driven buffer draining in collection response parsing. |
test/brpc_couchbase_unittest.cpp |
Adds regression coverage ensuring malformed headers don’t consume buffered bytes or mutate outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1510
to
+1515
| const int value_size = (int)header.total_body_length - | ||
| (int)header.extras_length - (int)header.key_length; | ||
| if (value_size < 0) { | ||
| butil::string_printf(&_err, "value_size=%d is negative", value_size); | ||
| return false; | ||
| } |
| const int value_size = (int)header.total_body_length - | ||
| (int)header.extras_length - (int)header.key_length; | ||
| if (value_size < 0) { | ||
| butil::string_printf(&_err, "value_size=%d is negative", value_size); |
Comment on lines
+107
to
+111
| brpc::policy::CouchbaseResponseHeader header = {}; | ||
| header.magic = brpc::policy::CB_MAGIC_RESPONSE; | ||
| header.command = brpc::policy::CB_GET_COLLECTIONS_MANIFEST; | ||
| header.extras_length = 1; | ||
| header.total_body_length = 0; |
Comment on lines
+126
to
+128
| uint8_t collection_id = 0; | ||
| EXPECT_FALSE(res2.popCollectionId(&collection_id)); | ||
| EXPECT_EQ(sizeof(header) + next_response.size(), res2.rawBuffer().size()); |
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: resolve
Problem Summary:
popCollectionId()andpopManifest()insrc/brpc/couchbase.cppsize the value section with unsigned arithmetic:header.total_body_length - header.extras_length - header.key_length. All three fields come straight from the server's response header and nothing cross-checks them, so a reply whereextras_length + key_lengthexceedstotal_body_lengthwraps the subtraction close toSIZE_MAX.IOBuf::cutn()clamps to what is available, so the whole remaining pipelined buffer gets swallowed:popManifest()hands those bytes back to the caller as the collection manifest,popCollectionId()folds them into the error string, and either way the response stream on that connection is out of sync from then on. The sibling parsers in the same file (popGet,popStore,popCounter,popVersion) already compute this as a signedintand bail out when it is negative; only these two collection parsers were written withsize_tand no check.Reproduced against a 24-byte header with
total_body_length=0,extras_length=1followed by 64 bytes of the next response:popManifest()returns true with a computed size of 18446744073709551615, copies 63 bytes of the following response into the manifest string and leaves the buffer empty. With this change it returns false and the buffer is untouched.What is changed and the side effects?
Changed:
Both functions now use the same signed computation and
< 0rejection as their siblings, done beforepop_front()so a malformed reply does not consume anything. Added a regression test totest/brpc_couchbase_unittest.cpp.Side effects:
Performance effects: none.
Breaking backward compatibility: no, well-formed responses parse exactly as before.
Check List: