fix use-after-free of the new block in SingleIOBuf::assign#3397
Open
ubeddulla wants to merge 1 commit into
Open
fix use-after-free of the new block in SingleIOBuf::assign#3397ubeddulla wants to merge 1 commit into
ubeddulla wants to merge 1 commit into
Conversation
Signed-off-by: ubeddulla khan <ubed@bugqore.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a use-after-free in butil::SingleIOBuf::assign() when the input IOBuf spans multiple BlockRefs, by avoiding reset() after alloc_block_by_size() has just set _cur_block to the newly allocated destination block.
Changes:
- Update
SingleIOBuf::assign()to drop only the previously assigned_cur_refreference instead of callingreset()(which could release_cur_blockand invalidate the destination block). - Add a regression unit test covering multi-block concatenation with a message larger than the default block size, including re-assigning on an already-assigned
SingleIOBuf.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/butil/single_iobuf.cpp |
Prevents freeing/recycling the freshly allocated destination block during multi-block assign() concatenation. |
test/iobuf_unittest.cpp |
Adds a regression test that reproduces the prior UAF scenario and verifies correctness across repeated assign(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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:
SingleIOBuf::assign()takes the concatenating path when the message spans more than oneBlockRefof the sourceIOBuf, and that path frees the block it is about to write into.alloc_block_by_size(msg_size)stores the block it returns in the member_cur_block, and for a freshly created block that member holds the only reference.reset()then runs on the next line and releases_cur_block. For a message bigger than the default block size it callsdec_ref(),nsharedgoes 1 to 0 and the block is freed; for a smaller one the block goes back into the TLS pool where another owner can take it.msg_sizebytes intob, bumpsb->size, storesbin_cur_refandinc_ref()s it._cur_refkeeps the dangling pointer, so the laterreset()or the destructor dec-refs it a second time.ASAN on a source
IOBufof two 5000/6000 byte blocks withmsg_size = 11000:What is changed and the side effects?
Changed:
Drop only the stale
_cur_refreference instead of callingreset().alloc_block_by_size()already releases the old_cur_blockwhen it cannot be reused, so the reference that had to go here is the one to the previously assigned data. The single-BlockReffast path above andassign_user_data()are unaffected, they already release before acquiring.reallocate_downward()orders itsdec_ref()after the copy and is fine as is.single_iobuf_assign_large_multi_blockiniobuf_unittest.cppcovers it: it reports the trace above on the current tree and passes with the change, and the other 59IOBufTestcases still pass under ASAN.Side effects:
Performance effects: none. Keeping
_cur_blocklets a repeatedassign()reuse the same block, which the old code could not do since it released it every time.Breaking backward compatibility: none.
Check List: