fix popCollectionId over-consuming the couchbase response body by sahvx655-wq · Pull Request #3614 · apache/brpc · GitHub
Skip to content

fix popCollectionId over-consuming the couchbase response body - #3614

Open
sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:couchbase-collection-id-overconsume
Open

sahvx655-wq wants to merge 1 commit into
apache:masterfrom
sahvx655-wq:couchbase-collection-id-overconsume

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

CouchbaseOperations::CouchbaseResponse::popCollectionId in src/brpc/couchbase.cpp parses a CB_COLLECTIONS_GET_CID response whose total_body_length already counts the 12-byte extras (8-byte manifest id plus 4-byte collection id). On the success path it pops the 24-byte header, then pops 8 bytes for the manifest id, 4 bytes for the collection id, and then pops total_body_length again. Those 12 extras bytes are part of total_body_length, so the handler consumes 12 bytes too many and reads into the next pipelined response, desyncing every following response on the connection. The header fields come straight from the (untrusted) server peer, and a response with extras_length >= 12 but total_body_length < 12 reaches the fixed 8+4-byte reads with no lower bound.

What is changed and the side effects?

Changed:

Reject the response when total_body_length < 12 next to the existing extras_length < 12 check, and pop only total_body_length - 12 remaining body bytes after the extras so the handler consumes exactly one message. The sibling pop* handlers already size their value against the body, so only this one was affected.

CouchbaseUnitTest.CollectionIdSuccessConsumesExactBody feeds a valid GET_CID response followed by a second response and checks the second one is left intact: it fails on master (12 bytes of the next response are eaten, leaving 52 instead of 64) and passes with the fix.

Side effects:

  • Performance effects: none

  • Breaking backward compatibility: no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant