bosswnx opened a new pull request, #67442:
URL: https://github.com/apache/doris/pull/67442
### What problem does this PR solve?
Issue Number: close #67428
Related PR: #66889 (introduced the UAF)
Problem Summary:
ASAN heap-use-after-free in `SharedMemtable::~SharedMemtable()` during
group-commit memtable flush task teardown, introduced by #66889.
`PartOfGroupMemtableFlushTask` holds its `SharedMemtable` by `shared_ptr` but
only a `weak_ptr` to the `FlushToken`. When `run()`'s local
`shared_ptr<FlushToken>` drops the last reference at the end of `run()`,
destruction cascades `FlushToken` -> `RowsetWriter` -> `RowsetWriterContext`;
the thread pool then destroys the task object, and `~SharedMemtable()`
dereferences the dangling raw `RowsetWriterContext* rowset_ctx` in
`remove_segment_allocated_lsns()`. Reproduced by the nonConcurrent pipeline on
two unrelated PRs (#67404, #67402).
### What changed?
- `SharedMemtable` now owns `std::shared_ptr<SegmentAllocatedLsnMap>`
captured from the group writer context at submission; insert/remove go through
the owned map. A null map stands for "no LSN allocation" (equivalent to
`need_allocated_lsn()`, since `GroupRowsetWriter::init()` creates the map
exactly when needed). This keeps the precise cleanup dependency alive without
extending the whole `RowsetWriter` lifetime — the approach recommended in the
#67428 triage.
- Add `SegmentAllocatedLsnMap::contains_segment()` for test assertions.
- Regression tests covering: (a) the last token/writer owner released while
a group flush task finishes, (b) a queued subtask running after its weak token
expired, (c) cancellation, plus LSN-entry cleanup assertions on the flush-error
path.
### Verification
ASAN BE UT, both directions: **without** the fix the new tests abort with
the exact reported UAF (`SUMMARY: AddressSanitizer: heap-use-after-free
rowset_writer_context.h:202:9 in
doris::RowsetWriterContext::remove_segment_allocated_lsns`); **with** the fix
the `MemTableFlushExecutor*` tests pass 8/8 (two runs).
### Release note
None
### Check List (For Author)
- Test
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [x] No.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label
--
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]
