Skip to content

fix(ubshm): reject truncated shared memory names - #3728

Open
songgaoye wants to merge 3 commits into
apache:masterfrom
songgaoye:fix/ubshm-name-truncation
Open

songgaoye wants to merge 3 commits into
apache:masterfrom
songgaoye:fix/ubshm-name-truncation

Conversation

@songgaoye

@songgaoye songgaoye commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

UB shared-memory names are generated with snprintf, but the previous code
only handled negative return values. An overlong endpoint name could therefore
silently truncate the generated shared-memory name, causing collisions or use
of an unintended shared-memory object.

What is changed and the side effects?

Changed:

  • Treat snprintf truncation as an error when generating client and server
    shared-memory names.
  • Reject UBS setup rather than use a truncated name.
  • Release the acquired transaction slot when client local-name construction
    fails.
  • Add regression coverage that limits the transaction manager to one slot and
    verifies the slot can be acquired again after the rejected setup.

Side effects:

  • Performance effects:

    None on successful setup paths.

  • Breaking backward compatibility:

    Connections whose generated shared-memory names exceed the fixed limit now
    fall back instead of silently using a truncated name.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The regression test does not cover the independently changed server-handshake path for overlong peer names, leaving its collision-prone fallback behavior unverified.

1 open issue
What changed in this PR

This PR hardens UB shared-memory setup by rejecting names that would be truncated by snprintf, preventing collisions or unintended shared-memory object usage, and releases the client transaction slot when local-name construction fails.

Changes:

  • Detect truncation when constructing client, server, and remote shared-memory names.
  • Fall back from UB when generated names cannot be represented safely.
  • Add regression coverage for transaction-slot reuse after rejected setup.
File Description
test/​brpc_ubring_unittest.cpp Adds regression coverage verifying that a rejected overlong local shared-memory name releases the transaction slot for reuse.
src/​brpc/​ubshm/​ub_ring.cpp Rejects truncated client and remote shared-memory names and releases the client transaction when local-name construction fails.
src/​brpc/​ubshm/​ub_endpoint.cpp Rejects truncated server shared-memory names during handshake and falls back from UB.

Review effort: Lite

Comment thread src/brpc/ubshm/ub_endpoint.cpp Resolved
@wwbmmm
wwbmmm requested a lite review from Copilot November 11, 2026 13:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

3 open issues
1 resolved since last review

Review effort: Lite

Comment thread src/brpc/ubshm/ub_ring.cpp Resolved
Comment thread src/brpc/ubshm/ub_ring.cpp Resolved
Comment thread src/brpc/ubshm/ub_endpoint.h Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The review found no active issues, with regression coverage added for truncation rejection and transaction-slot recovery.

No open issues 🎉

3 resolved since last review

Review effort: Lite

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.

2 participants