You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by pa...@apache.org on 2026/04/23 09:54:22 UTC
(doris) branch master updated: [Chore](check) add some Sanity-check for avoid npe (#62691)
This is an automated email from the ASF dual-hosted git repository.
BiteTheDDDDt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new e7afcac96d6 [Chore](check) add some Sanity-check for avoid npe (#62691)
e7afcac96d6 is described below
commit e7afcac96d685989201c63205a531821ee97a950
Author: Pxl <xl...@selectdb.com>
AuthorDate: Thu Apr 23 17:54:15 2026 +0800
[Chore](check) add some Sanity-check for avoid npe (#62691)
This pull request adds comprehensive null pointer checks and improved
error reporting to critical memory management and pipeline execution
paths. The main goal is to catch and clearly report misuses or
unexpected nulls in resource and memory tracker objects, especially in
RELEASE builds where previous checks (like DCHECK) were no-ops. This
should make debugging memory management and pipeline execution issues
much easier by providing explicit error messages and context, rather
than opaque crashes or silent failures.
The most important changes are:
**Memory Tracker Safety and Error Reporting:**
* Added explicit null checks and detailed exception throwing in
`ThreadMemTrackerMgr` methods, including `attach_limiter_tracker`,
`detach_limiter_tracker`, and `limiter_mem_tracker`, to prevent
propagation of null memory tracker pointers and provide clear
diagnostics when misuse occurs.
[[1]](diffhunk://#diff-d22d036bc75761be008a06388ac64878a0150314aad313ba63cb1e70d6b98f67R22-R39)
[[2]](diffhunk://#diff-d22d036bc75761be008a06388ac64878a0150314aad313ba63cb1e70d6b98f67R68-R86)
[[3]](diffhunk://#diff-65c8664db7728c5cc60ea7ae8434dc47014b2531e03072333a5989554b747029R105-R113)
* Improved header includes to ensure `common/exception.h` is available
where exceptions are thrown.
**Resource Context Initialization and Validation:**
* Added step-by-step null checks in `AttachTask` and
`SwitchResourceContext` constructors to pinpoint exactly which part of
the resource context chain is null, throwing a `FatalError` with clear
context if any are missing.
[[1]](diffhunk://#diff-2b2f9638f5ba33910b1e2325b34ad4f99d4b9da393a9458858471cd84bf4e9e6R47-R61)
[[2]](diffhunk://#diff-2b2f9638f5ba33910b1e2325b34ad4f99d4b9da393a9458858471cd84bf4e9e6R82-R96)
* Enhanced `ThreadContext::attach_task` to check for null
`ResourceContext`, `MemoryContext`, and `mem_tracker` before attaching,
with detailed error messages to aid debugging of half-initialized or
misused contexts.
**Pipeline Execution Robustness:**
* Added sanity checks for null pointers in the `PipelineTask::execute`
method before dereferencing critical objects, throwing an exception with
query ID and pointer values for easier triage if any are unexpectedly
null.
[[1]](diffhunk://#diff-b1379f2a0e6db93d3aaeec091b10d8a13e9461322f30af2c3f4a54df5bee702fR511-R520)
[[2]](diffhunk://#diff-b1379f2a0e6db93d3aaeec091b10d8a13e9461322f30af2c3f4a54df5bee702fR31)
These changes collectively increase the safety and debuggability of
memory and resource management in the codebase, reducing the risk of
silent failures and making it easier to identify the root cause of any
issues.
---
be/src/exec/operator/hashjoin_build_sink.cpp | 9 +-
.../partitioned_hash_join_sink_operator.cpp | 11 ++-
be/src/exec/operator/set_sink_operator.cpp | 8 +-
be/src/exec/pipeline/pipeline_task.cpp | 1 +
be/src/runtime/memory/thread_mem_tracker_mgr.cpp | 39 ++++++++
be/src/runtime/thread_context.cpp | 101 +++++++++++++++++++++
be/src/runtime/thread_context.h | 5 +
7 files changed, 171 insertions(+), 3 deletions(-)
diff --git a/be/src/exec/operator/hashjoin_build_sink.cpp b/be/src/exec/operator/hashjoin_build_sink.cpp
index ba1ec4236a6..9f1a05876f8 100644
--- a/be/src/exec/operator/hashjoin_build_sink.cpp
+++ b/be/src/exec/operator/hashjoin_build_sink.cpp
@@ -118,7 +118,14 @@ Status HashJoinBuildSinkLocalState::terminate(RuntimeState* state) {
if (_terminated) {
return Status::OK();
}
- RETURN_IF_ERROR(_runtime_filter_producer_helper->skip_process(state));
+ // Defensive null-guard: other paths in this file already gate on
+ // `_runtime_filter_producer_helper` because cancel / early wake-up may
+ // run terminate before the helper is attached. The terminate path used
+ // to be the only one missing the guard, which would NPE inside
+ // skip_process() and surface as a generic crash deep in the allocator.
+ if (_runtime_filter_producer_helper) {
+ RETURN_IF_ERROR(_runtime_filter_producer_helper->skip_process(state));
+ }
return JoinBuildSinkLocalState::terminate(state);
}
diff --git a/be/src/exec/operator/partitioned_hash_join_sink_operator.cpp b/be/src/exec/operator/partitioned_hash_join_sink_operator.cpp
index e59f17c7bcd..4a0d4d0f42e 100644
--- a/be/src/exec/operator/partitioned_hash_join_sink_operator.cpp
+++ b/be/src/exec/operator/partitioned_hash_join_sink_operator.cpp
@@ -235,12 +235,21 @@ Status PartitionedHashJoinSinkLocalState::terminate(RuntimeState* state) {
if (_terminated) {
return Status::OK();
}
+ // Walk the chain `_shared_state -> _inner_runtime_state` defensively.
+ // The inner runtime state is built separately from the outer sink's
+ // setup_local_state path, so its atomicity is weaker than a top-level
+ // local-state init. Any null in this chain on a cancel / early-wake
+ // path would NPE inside terminate.
+ if (_shared_state == nullptr || _shared_state->_inner_runtime_state == nullptr) {
+ return PipelineXSpillSinkLocalState<PartitionedHashJoinSharedState>::terminate(state);
+ }
HashJoinBuildSinkLocalState* inner_sink_state {nullptr};
if (auto* tmp_sink_state = _shared_state->_inner_runtime_state->get_sink_local_state()) {
inner_sink_state = assert_cast<HashJoinBuildSinkLocalState*>(tmp_sink_state);
}
if (inner_sink_state) {
- if (_parent->cast<PartitionedHashJoinSinkOperatorX>()._inner_sink_operator) {
+ if (_parent->cast<PartitionedHashJoinSinkOperatorX>()._inner_sink_operator &&
+ inner_sink_state->_runtime_filter_producer_helper) {
RETURN_IF_ERROR(inner_sink_state->_runtime_filter_producer_helper->skip_process(state));
}
inner_sink_state->_terminated = true;
diff --git a/be/src/exec/operator/set_sink_operator.cpp b/be/src/exec/operator/set_sink_operator.cpp
index ec037697e98..608bd8c1b69 100644
--- a/be/src/exec/operator/set_sink_operator.cpp
+++ b/be/src/exec/operator/set_sink_operator.cpp
@@ -31,7 +31,13 @@ Status SetSinkLocalState<is_intersect>::terminate(RuntimeState* state) {
if (_terminated) {
return Status::OK();
}
- RETURN_IF_ERROR(_runtime_filter_producer_helper->skip_process(state));
+ // Defensive null-guard, mirroring the close()/sink() paths in this
+ // file which already gate on `_runtime_filter_producer_helper`.
+ // terminate() may run on a cancel / early-wake path before the helper
+ // is attached, in which case skip_process() would NPE.
+ if (_runtime_filter_producer_helper) {
+ RETURN_IF_ERROR(_runtime_filter_producer_helper->skip_process(state));
+ }
return Base::terminate(state);
}
diff --git a/be/src/exec/pipeline/pipeline_task.cpp b/be/src/exec/pipeline/pipeline_task.cpp
index 58ccf5dbb72..146d3f028c6 100644
--- a/be/src/exec/pipeline/pipeline_task.cpp
+++ b/be/src/exec/pipeline/pipeline_task.cpp
@@ -27,6 +27,7 @@
#include <ostream>
#include <vector>
+#include "common/exception.h"
#include "common/logging.h"
#include "common/status.h"
#include "core/block/block.h"
diff --git a/be/src/runtime/memory/thread_mem_tracker_mgr.cpp b/be/src/runtime/memory/thread_mem_tracker_mgr.cpp
index 138eb7e14c6..3a0f0c7972f 100644
--- a/be/src/runtime/memory/thread_mem_tracker_mgr.cpp
+++ b/be/src/runtime/memory/thread_mem_tracker_mgr.cpp
@@ -19,6 +19,8 @@
#include <gen_cpp/types.pb.h>
+#include "common/exception.h"
+#include "common/signal_handler.h"
#include "runtime/exec_env.h"
namespace doris {
@@ -26,6 +28,25 @@ namespace doris {
void ThreadMemTrackerMgr::attach_limiter_tracker(
const std::shared_ptr<MemTrackerLimiter>& mem_tracker) {
DCHECK(mem_tracker);
+ if (UNLIKELY(!mem_tracker)) {
+ // Without this guard, _limiter_tracker silently becomes null and any
+ // later allocation on this thread would dereference it inside the
+ // allocator's memory check path, surfacing as a generic NPE far from
+ // the real call site that fed the null tracker. The query id is
+ // read from signal-handler thread-local storage; it does not depend
+ // on any object that may already be torn down. Note: do not call
+ // print_debug_string() here — it itself dereferences _limiter_tracker
+ // (via make_profile_str()), which on a fresh ThreadMemTrackerMgr is
+ // still null and would crash inside the format call before the
+ // intended FatalError is thrown.
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::attach_limiter_tracker called with null mem_tracker. "
+ "previous limiter label={}, snapshot_stack_depth={}, "
+ "query_id={:x}-{:x}",
+ _limiter_tracker ? _limiter_tracker->label() : "<null>",
+ _last_attach_snapshots_stack.size(), doris::signal::query_id_hi,
+ doris::signal::query_id_lo));
+ }
CHECK(init());
flush_untracked_mem();
_last_attach_snapshots_stack.push_back(
@@ -54,7 +75,25 @@ void ThreadMemTrackerMgr::detach_limiter_tracker() {
CHECK(init());
flush_untracked_mem();
shrink_reserved();
+ if (UNLIKELY(_last_attach_snapshots_stack.empty())) {
+ // detach_limiter_tracker() is invoked from RAII destructors that are
+ // noexcept; throwing would call std::terminate without a useful
+ // message. A LOG(FATAL) gives a controlled abort with a flushed
+ // message and a glog-captured stack trace.
+ LOG(FATAL) << "ThreadMemTrackerMgr::detach_limiter_tracker called with empty snapshot "
+ "stack. current limiter label="
+ << (_limiter_tracker ? _limiter_tracker->label() : "<null>")
+ << ", query_id=" << std::hex << doris::signal::query_id_hi << "-"
+ << doris::signal::query_id_lo << std::dec;
+ }
DCHECK(!_last_attach_snapshots_stack.empty());
+ if (UNLIKELY(!_last_attach_snapshots_stack.back().limiter_tracker)) {
+ // Same noexcept-destructor rationale as above.
+ LOG(FATAL) << "ThreadMemTrackerMgr::detach_limiter_tracker restored null limiter from "
+ "snapshot. stack_depth_before_pop="
+ << _last_attach_snapshots_stack.size() << ", query_id=" << std::hex
+ << doris::signal::query_id_hi << "-" << doris::signal::query_id_lo << std::dec;
+ }
_limiter_tracker_sptr = _last_attach_snapshots_stack.back().limiter_tracker;
_limiter_tracker = _limiter_tracker_sptr.get();
_wg_wptr = _last_attach_snapshots_stack.back().wg_wptr;
diff --git a/be/src/runtime/thread_context.cpp b/be/src/runtime/thread_context.cpp
index 7e8354f6ff6..83f38369d6c 100644
--- a/be/src/runtime/thread_context.cpp
+++ b/be/src/runtime/thread_context.cpp
@@ -25,6 +25,35 @@ namespace doris {
class MemTracker;
void AttachTask::init(const std::shared_ptr<ResourceContext>& rc) {
+ // Validate the ResourceContext chain before mutating any thread-local
+ // or signal state. If any link is null we throw immediately, so the
+ // caller's stack-unwind sees a clean state (no thread-local handle is
+ // acquired and no signal task id is set). Without these the previous
+ // code would silently propagate a null mem_tracker into
+ // ThreadMemTrackerMgr and crash much later inside the allocator.
+ if (UNLIKELY(rc == nullptr)) {
+ throw Exception(
+ Status::FatalError("AttachTask::init: rc is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->memory_context() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask::init: rc->memory_context() is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->memory_context()->mem_tracker() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask::init: rc->memory_context()->mem_tracker() is null. "
+ "ResourceContext was created but set_mem_tracker has not been called yet "
+ "(likely a half-initialized QueryContext used before _init_query_mem_tracker). "
+ "signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->task_controller() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask::init: rc->task_controller() is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
ThreadLocalHandle::create_thread_local_if_not_exits();
signal::set_signal_task_id(rc->task_controller()->task_id());
thread_context()->attach_task(rc);
@@ -38,17 +67,52 @@ AttachTask::AttachTask(const std::shared_ptr<MemTrackerLimiter>& mem_tracker) {
// if parameter is `orphan_mem_tracker`, if you do not switch thraed mem tracker afterwards,
// alloc or free memory from Allocator will fail DCHECK. unless you know for sure that
// the thread will not alloc or free memory from Allocator later.
+ //
+ // Validate before constructing the ResourceContext: MemoryContext::set_mem_tracker()
+ // immediately dereferences mem_tracker->limit(), so a null shared_ptr would
+ // crash there before reaching init()'s diagnostics.
+ if (UNLIKELY(mem_tracker == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask(MemTrackerLimiter): mem_tracker is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
std::shared_ptr<ResourceContext> rc = ResourceContext::create_shared();
rc->memory_context()->set_mem_tracker(mem_tracker);
init(rc);
}
AttachTask::AttachTask(RuntimeState* runtime_state) {
+ // Walk the chain `runtime_state -> get_query_ctx() -> resource_ctx()`
+ // step by step so that an unexpected null pinpoints exactly which link
+ // failed instead of crashing with a generic NPE inside attach_task() or
+ // even later inside the allocator.
+ if (UNLIKELY(runtime_state == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask(RuntimeState*): runtime_state is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(runtime_state->get_query_ctx() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask(RuntimeState*): runtime_state->get_query_ctx() is null. "
+ "signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(runtime_state->get_query_ctx()->resource_ctx() == nullptr)) {
+ throw Exception(
+ Status::FatalError("AttachTask(RuntimeState*): query_ctx->resource_ctx() is null. "
+ "signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
signal::set_signal_is_nereids(runtime_state->is_nereids());
init(runtime_state->get_query_ctx()->resource_ctx());
}
AttachTask::AttachTask(QueryContext* query_ctx) {
+ if (UNLIKELY(query_ctx == nullptr)) {
+ throw Exception(Status::FatalError(
+ "AttachTask(QueryContext*): query_ctx is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
init(query_ctx->resource_ctx());
}
@@ -60,6 +124,33 @@ AttachTask::~AttachTask() {
SwitchResourceContext::SwitchResourceContext(const std::shared_ptr<ResourceContext>& rc) {
DCHECK(rc != nullptr);
+ // Validate the chain before mutating any thread-local or signal state,
+ // symmetric to AttachTask::init(). Throwing after the thread-local
+ // handle was acquired or the signal task id was set would skip this
+ // object's destructor (because construction failed) and leak the
+ // handle / leave a stale signal task id behind.
+ if (UNLIKELY(rc == nullptr)) {
+ throw Exception(
+ Status::FatalError("SwitchResourceContext: rc is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->memory_context() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "SwitchResourceContext: rc->memory_context() is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->memory_context()->mem_tracker() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "SwitchResourceContext: rc->memory_context()->mem_tracker() is null. "
+ "ResourceContext was switched in before _init_query_mem_tracker ran. "
+ "signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
+ if (UNLIKELY(rc->task_controller() == nullptr)) {
+ throw Exception(Status::FatalError(
+ "SwitchResourceContext: rc->task_controller() is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
doris::ThreadLocalHandle::create_thread_local_if_not_exits();
DCHECK(thread_context()->is_attach_task());
old_resource_ctx_ = thread_context()->resource_ctx();
@@ -84,6 +175,16 @@ SwitchResourceContext::~SwitchResourceContext() {
SwitchThreadMemTrackerLimiter::SwitchThreadMemTrackerLimiter(
const std::shared_ptr<doris::MemTrackerLimiter>& mem_tracker) {
DCHECK(mem_tracker);
+ // Third entry point that calls attach_limiter_tracker(). Without this
+ // null guard a null mem_tracker silently propagates and the next
+ // allocation on this thread would NPE deep inside the allocator. Throw
+ // before acquiring the thread-local handle / doing any side effect so
+ // the destructor (which is noexcept) never runs in a dirty state.
+ if (UNLIKELY(mem_tracker == nullptr)) {
+ throw Exception(Status::FatalError(
+ "SwitchThreadMemTrackerLimiter: mem_tracker is null. signal_query_id={:x}-{:x}",
+ signal::query_id_hi, signal::query_id_lo));
+ }
doris::ThreadLocalHandle::create_thread_local_if_not_exits();
if (mem_tracker != thread_context()->thread_mem_tracker_mgr->limiter_mem_tracker_sptr()) {
thread_context()->thread_mem_tracker_mgr->attach_limiter_tracker(mem_tracker);
diff --git a/be/src/runtime/thread_context.h b/be/src/runtime/thread_context.h
index 54e565cc424..332cd0f6545 100644
--- a/be/src/runtime/thread_context.h
+++ b/be/src/runtime/thread_context.h
@@ -168,6 +168,11 @@ public:
void attach_task(const std::shared_ptr<ResourceContext>& rc) {
// will only attach_task at the beginning of the thread function, there should be no duplicate attach_task.
DCHECK(resource_ctx_ == nullptr);
+ // Validation of `rc` and its sub-objects is performed by the
+ // AttachTask::init() / SwitchResourceContext constructor entry
+ // points before any thread-local or signal mutation, so a thrown
+ // FatalError does not leak thread-local handle counts or leave a
+ // stale signal task id behind.
resource_ctx_ = rc;
thread_mem_tracker_mgr->attach_limiter_tracker(rc->memory_context()->mem_tracker(),
rc->workload_group());
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org