Back-port packet assert from v26.1.x #251
Open
Conversation
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
Posix stack's data_sink implementation stores a net packet locally and passes it by reference to the file descriptor. If put is called concurrently, UB can occur. With two invocations, one faster and one slower the faster invocation will reset the packet on completion of its write_all invocation, and the slower may dereference a nullptr in using the referenced packet This commit changes three things: 1. it initializes the net::packet in posix_data_sink_impl to a null packet 2. reset of posix_data_sink_impl's net::packet will now occur in a finally s.t. the contained packet will also be reset on excpetions 3. invocation of posix_data_sink_impl::put will now assert that the contained packet is null before overwriting. Any concurrent usages should now fail on an assert. Closes scylladb#3147
There was a problem hiding this comment.
Pull request overview
This PR back-ports a packet assertion and error handling improvement from v26.1.x to detect and prevent potential double-write scenarios in the POSIX network stack implementation.
Changes:
- Added an assertion to catch double-write attempts before they occur
- Changed error handling from
thentofinallyto ensure cleanup happens in all cases - Initialized
_pmember variable to a null packet state
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/net/posix-stack.cc | Added assertion to detect concurrent writes and improved cleanup reliability by switching to finally |
| include/seastar/net/posix-stack.hh | Initialized _p member to null packet state to support the new assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Backport of a74d64f
With the modifications for the future api version cut out.
This assert has baked long enough to be reasonable to backport