Doris-Breakwater commented on issue #67428: URL: https://github.com/apache/doris/issues/67428#issuecomment-5504708555
Breakwater-GitHub-Analysis-Slot: slot_64930d36f4b6 ## Initial triage **Verdict: confirmed code-level lifetime bug (high confidence), with master-blocking CI impact.** The issue currently has no labels. Suggested project labels are `kind/fix`, `bug/asan`, `kind/stability`, `area/load`, and `area/storage`. ### Verified facts - In base `952bfcbb40fa756cb66a7d8e70d559496eb344c8`, `SharedMemtable` owns a raw `RowsetWriterContext*` plus `has_allocated_lsns`. Its destructor dereferences that pointer to remove the segment's LSN entry. - Commit `e925e6ae1b2` introduced that raw pointer, the LSN insertion in `_memtable2block()`, and the destructor cleanup. - `RowsetWriter` embeds `RowsetWriterContext` by value, so destroying the last writer owner destroys the context. The `GroupRowsetWriter` context creates the `SegmentAllocatedLsnMap` as a `shared_ptr` and shares that same map with the data and row-binlog writer contexts. - `PartOfGroupMemtableFlushTask` owns `SharedMemtable` but only a `weak_ptr<FlushToken>`. `run()` temporarily locks the token; after `run()` returns, `ThreadPool::dispatch_thread()` explicitly resets the task at line 628. Therefore the local token can release the last writer/context owner before task destruction releases the last `SharedMemtable` owner. - The reported ASAN free stack at the end of `PartOfGroupMemtableFlushTask::run()` and invalid read from `SharedMemtable::~SharedMemtable()` exactly matches this ownership order. This is not merely inferred from the later cascade failures. - The relevant memtable-flush lifetime code is unchanged between the two reported bases (`5fbe3600024...` and its child `952bfcbb40fa...`). No fix PR is linked to this issue at the time of this triage. ### Important attribution boundary The root cause is the non-owning `rowset_ctx` added by #66889. The timeline shows that #66191/#66899 made the failure observable, but it does not yet prove that either changed this lifetime. #66899 adds row-binlog regression coverage and #66191 changes `GroupRowsetBuilder::init()`/transaction attachment; neither changes the raw-pointer cleanup. Treat them as coverage/timing exposure candidates unless a focused rerun or commit bisection establishes a stronger causal link. ### Recommended fix Prefer making `SharedMemtable` own the precise cleanup dependency: 1. Replace `RowsetWriterContext* rowset_ctx` with `std::shared_ptr<segment_v2::SegmentAllocatedLsnMap>` captured from `group_rowset_writer->context().allocated_lsn_map` at submission. 2. Insert and remove entries directly through that map; retain an insertion flag (or equivalent invariant) so cleanup runs only when an entry was created. 3. Use a non-null map to represent the existing `need_allocated_lsn()` condition, since `GroupRowsetWriter::init()` creates the map exactly when LSN allocation is required. This is narrower than retaining the whole `RowsetWriter`, preserves the intended shared-map design, and uses the map's existing mutex. Retaining `std::shared_ptr<RowsetWriter>` would also make the dereference safe, but extends a much larger object's lifetime. I would not use destruction reordering inside `run()` as the sole fix. It addresses the captured last-local-token ordering, but the task intentionally keeps only a weak token; a queued sibling task can observe an expired token. Cleanup performed by `SharedMemtable` should therefore own the state it dereferences rather than depend on a token still being lockable. ### Validation requested for the fix - Add a deterministic ASAN/BE unit test covering both (a) the last external token/writer owner being released while a group flush task finishes and (b) a queued group subtask running after its weak token has expired. - Assert that the per-segment LSN entry is removed on success, cancellation, and flush-error paths without retaining stale entries. - Run the relevant memtable-flush/group-rowset-writer BE tests under ASAN, the row-binlog/group-commit regression suites, and the full `nonConcurrent` pipeline. ### Missing evidence No additional information is required to establish the UAF root cause. For a deterministic regression test and for attributing the exposure window, please attach the exact first failing suite/load statement, the complete ASAN allocation/free stacks (not only selected frames), and the BE log around that load's cancellation/close/destruction. A focused rerun immediately before and after #66899 and #66191 would distinguish added coverage from a timing/lifetime change. -- 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]
