Repository navigation
null-terminate peer shm_name in HelloMessage::Deserialize - #3605
Merged
chenBright merged 1 commit intoOct 18, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Warning
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.
Copilot review overview
2 open findings
What changed in this PR
This PR fixes a handshake safety issue where HelloMessage::Deserialize could leave shm_name unterminated, allowing downstream code that treats it as a C-string to read past the buffer.
Changes:
- Force
shm_nameto be NUL-terminated after deserialization. - Update
HelloMessageTest.full_shm_nameto assert proper termination and length behavior.
| File | Description |
|---|---|
| test/brpc_ubring_unittest.cpp | Tightens the unit test to validate NUL-termination and bounded length of decoded shm_name. |
| src/brpc/ubshm/ub_endpoint.cpp | Ensures deserialized shm_name is always a valid C-string by forcing a terminator. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+147
to
+150
| // shm_name arrives verbatim from the peer and is used downstream as a | ||
| // C-string (strncpy/strrchr/snprintf("%s")/shm allocate+map). A peer that | ||
| // fills all SHM_MAX_NAME_BUFF_LEN bytes without a terminator would make | ||
| // those scans run past the buffer, so force termination here. |
| // A peer that fills every byte of shm_name leaves no terminator on the | ||
| // wire. Deserialize must still hand back a valid C-string, because the | ||
| // handshake feeds the decoded name straight into strncpy/strrchr/ | ||
| // snprintf("%s") and the shm allocate/map calls; without a terminator |
Copilot stopped reviewing on behalf of
chenBright due to an error
October 14, 2026 03:30
Contributor
|
LGTM |
Sherlock0203
added a commit
to Sherlock0203/brpc
that referenced
this pull request
Oct 19, 2026
Bring the apache master line up to f0239d7 into this branch in a single merge, replacing the two stacked merges with master@36c2d12e and the macOS fix only. Everything the branch needs from that line is included: the data-format negotiation (apache#3507), the IPC_V2 data path (apache#3573), the macOS OpenSSL 4.0 build fix (apache#3616: X509 accessors return const pointers in the current macos-26-arm64 runner image, fixed with auto*), and the newer fixes, notably the null-termination of the peer shm_name in HelloMessage::Deserialize (apache#3605), the MPSC queue node release ordering (apache#3620) and the MultiFlatMap bucket-head erasure fix (apache#3617). Those master changes were built on the timerfd-era close path, which this branch had replaced with the bthread timer facade, generation-checked pool teardown and the cleanup worker, so both sides evolved the same ubshm core after 9334389. Resolution: - Close/clear core (UbrTrxClose, UbrTrxCloseCallback, UbrClearResourceCheck, ClearTrxResource, UbrPassiveClearTrx, UbrTrxCloseCheck) keeps this branch's versions. Master's extra start_time/op parameters were unused, and UbrCloseType only chose between DeleteTimerSafe and DeleteTimer, which the facade subsumes: UbrTimerDelAndWait degrades to the non-blocking delete inside a callback. The unused UbrCloseType enum is therefore dropped. - Master's format-aware initialization is kept: UbrServerTrxInit keeps the UbrDataFormat parameter and the Initialize*LegacyFormat split, UbrAllocateServerShm/UbrMapRemoteShm(UbrMapRemoteShmAddTimer) keep passing it, and ApplyAndMapLocalShm no longer presets the rx capacity (InitializeClientLocalLegacyFormat owns that now). All IPC_V2 ring code and the negotiation handshake are master's, unchanged, with master-side UNLIKELY/LIKELY converted to the module's BAIDU_* macros. - This branch's fixes that master lacked merge on top: the poller re-ADD refresh, butil::atomic for the poller-group flag, the lazy bthread timers replacing TimerInit, and the cleanup-worker changes (leaked queue, start retry instead of the terminal latch). - test/brpc_ubring_unittest.cpp: master's IPC_V2 assertions on the removed timer_fd/hb_timer_fd fields check the unarmed close_timer/hb_timer slots instead, and the UbrCleanupWorkerTest that PR LinQuickDev#74's squash dropped is restored alongside master's apache#3605 hardening of the full_shm_name test. Both sides' suites run. Tested: the close/cleanup/IPC_V2 resolution was verified on the equivalent tree before apache#3605 came in -- cmake WITH_UBRING=ON build, 31/31 unit tests, the same under ASAN (detect_leaks=0), and an example/ubring_performance e2e over the IPC backend (negotiated IPC_V2 path at ~148k QPS, clean teardowns, server kill -9 handled). The new master commits are additive to that resolution; cmake --build build --target brpc_ubring_unittest and ./build/test/brpc_ubring_unittest pass on this exact tree as well.
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:
Problem Summary:
On the UBRing handshake,
HelloMessage::Deserializecopies the 48-byteshm_namestraight from the peer's HELLO message and never terminates it. A peer that fills every byte ofSHM_MAX_NAME_BUFF_LENwith non-zero data leaves the field without a'\0', and the server side then treats it as a C-string:strncpyinto same-size buffers,strrchr,snprintf("%s", client_name), and the shm allocate/map/log calls inshm_ubs.cppall walk past the 48-byte buffer. Under ASAN this is a stack-buffer-overflow READ instrrchronclient_name.SHM_MAX_NAME_LENis defined asSHM_MAX_NAME_BUFF_LEN - 1, so a conforming name already leaves room for the terminator; only a malformed or hostile name reaches this.What is changed and the side effects?
Changed:
HelloMessage::Deserializesetsshm_name[SHM_MAX_NAME_BUFF_LEN - 1] = '\0'after the copy so the decoded name is always a valid C-string within the buffer. Names of 47 bytes or fewer are unchanged.HelloMessageTest.full_shm_namenow asserts the decoded name is terminated; it fails on the current tree and passes with this change.Side effects:
Performance effects: none.
Breaking backward compatibility: none; a conforming peer already sends a terminated name.
Check List: