[Bug](rec-cte) change _send_rpc use ExchangeSinkBuffer's weakptr to instead TaskExecutionContext#60734
[Bug](rec-cte) change _send_rpc use ExchangeSinkBuffer's weakptr to instead TaskExecutionContext#60734BiteTheDDDDt wants to merge 3 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix an ASAN-reported heap-use-after-free in ExchangeSinkBuffer::_send_rpc by changing how async BRPC callbacks decide whether it’s still safe to access ExchangeSinkBuffer state.
Changes:
- Switch BRPC callback liveness checks from
TaskExecutionContextweak locking to usingExchangeSinkBuffer’sweak_from_this(). - Remove
HasTaskExecutionCtxinheritance fromExchangeSinkBuffer. - Simplify
_turn_off_channel()to always notify_parentswithout a prior liveness check.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
be/src/pipeline/exec/exchange_sink_buffer.h |
Changes ExchangeSinkBuffer inheritance to use enable_shared_from_this and removes HasTaskExecutionCtx. |
be/src/pipeline/exec/exchange_sink_buffer.cpp |
Updates async callback handlers to lock weak_from_this() and adjusts _turn_off_channel() behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parent->on_channel_finished(ins.id); | ||
| } | ||
| for (auto& parent : _parents) { | ||
| parent->on_channel_finished(ins.id); |
There was a problem hiding this comment.
_parents stores raw ExchangeSinkLocalState*, and this method can be reached from async RPC callbacks. Removing the previous liveness guard and unconditionally calling parent->on_channel_finished(...) can lead to use-after-free if the local state has already been destroyed (even if the sink buffer is still alive). Please reintroduce a safe lifetime check (e.g., lock a TaskExecutionContext/other owning context before iterating) or change _parents to a lifetime-safe reference (or clear/unregister parents under lock during close) before calling into them.
| parent->on_channel_finished(ins.id); | |
| if (parent != nullptr) { | |
| parent->on_channel_finished(ins.id); | |
| } |
| closure); | ||
| #endif | ||
| class ExchangeSinkBuffer : public HasTaskExecutionCtx { | ||
| class ExchangeSinkBuffer : std::enable_shared_from_this<ExchangeSinkBuffer> { |
There was a problem hiding this comment.
std::enable_shared_from_this should be inherited publicly. With the current declaration class ExchangeSinkBuffer : std::enable_shared_from_this<...>, the inheritance is private, which prevents std::shared_ptr from properly wiring up the internal weak pointer and can make weak_from_this() always return empty. Since this PR relies on weak_from_this() to guard async callbacks, please change it to public std::enable_shared_from_this<ExchangeSinkBuffer> so the lifetime check works as intended (and to avoid future bad_weak_ptr issues if shared_from_this() is used).
| class ExchangeSinkBuffer : std::enable_shared_from_this<ExchangeSinkBuffer> { | |
| class ExchangeSinkBuffer : public std::enable_shared_from_this<ExchangeSinkBuffer> { |
|
run buildall |
|
run buildall |
|
Using rebuild total fragment ctx object to simplify the logic. |
3 participants
What problem does this PR solve?
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)