ubeddulla opened a new pull request, #3397:
URL: https://github.com/apache/brpc/pull/3397
### What problem does this PR solve?
Issue Number:
Problem Summary:
`SingleIOBuf::assign()` takes the concatenating path when the message spans
more than one `BlockRef` of the source `IOBuf`, and that path frees the block
it is about to write into.
1. `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.
2. `reset()` then runs on the next line and releases `_cur_block`. For a
message bigger than the default block size it calls `dec_ref()`, `nshared` goes
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.
3. The rest of the function still writes `msg_size` bytes into `b`, bumps
`b->size`, stores `b` in `_cur_ref` and `inc_ref()`s it. `_cur_ref` keeps the
dangling pointer, so the later `reset()` or the destructor dec-refs it a second
time.
ASAN on a source `IOBuf` of two 5000/6000 byte blocks with `msg_size =
11000`:
```
ERROR: AddressSanitizer: heap-use-after-free on address 0x626000000118
READ of size 8 at 0x626000000118 thread T0
#0 butil::SingleIOBuf::assign(butil::IOBuf const&, unsigned int)
single_iobuf.cpp:230
freed by thread T0 here:
#1 butil::IOBuf::Block::dec_ref() iobuf_inl.h:544
#2 butil::SingleIOBuf::reset() single_iobuf.cpp:193
#3 butil::SingleIOBuf::assign(butil::IOBuf const&, unsigned int)
single_iobuf.cpp:229
previously allocated by thread T0 here:
#2 butil::SingleIOBuf::alloc_block_by_size(unsigned int)
single_iobuf.cpp:126
```
### What is changed and the side effects?
Changed:
Drop only the stale `_cur_ref` reference instead of calling `reset()`.
`alloc_block_by_size()` already releases the old `_cur_block` when it cannot be
reused, so the reference that had to go here is the one to the previously
assigned data. The single-`BlockRef` fast path above and `assign_user_data()`
are unaffected, they already release before acquiring. `reallocate_downward()`
orders its `dec_ref()` after the copy and is fine as is.
`single_iobuf_assign_large_multi_block` in `iobuf_unittest.cpp` covers it:
it reports the trace above on the current tree and passes with the change, and
the other 59 `IOBufTest` cases still pass under ASAN.
Side effects:
- Performance effects: none. Keeping `_cur_block` lets a repeated `assign()`
reuse the same block, which the old code could not do since it released it
every time.
- Breaking backward compatibility: none.
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of
Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
