You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "BiteTheDDDDt (via GitHub)" <gi...@apache.org> on 2026/04/22 03:12:15 UTC
[PR] [Chore](check) add some Sanity-check for avoid npe [doris]
BiteTheDDDDt opened a new pull request, #62691:
URL: https://github.com/apache/doris/pull/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.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on code in PR #62691:
URL: https://github.com/apache/doris/pull/62691#discussion_r3123622279
##########
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)) {
Review Comment:
不是问题，本来npe也会挂，现在这样报错信息更明确
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4295823781
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4294256237
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62691:
URL: https://github.com/apache/doris/pull/62691#discussion_r3121433945
##########
be/src/runtime/thread_context.h:
##########
@@ -168,6 +168,25 @@ class ThreadContext {
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);
+ // Disambiguate which sub-object is null when attach silently propagates a null
+ // mem_tracker (see attach_limiter_tracker). These checks tell us at the source
+ // whether `rc` itself is empty, the embedded MemoryContext is null, or the
+ // MemoryContext exists but its mem_tracker_ was never set
+ // (i.e. _init_query_mem_tracker hasn't run yet for this ResourceContext).
+ if (UNLIKELY(rc == nullptr)) {
Review Comment:
Blocking: these throws happen only after `AttachTask::init()` has already called `ThreadLocalHandle::create_thread_local_if_not_exits()` and `signal::set_signal_task_id(rc->task_controller()->task_id())` in `thread_context.cpp`. If one of the new checks fires, construction aborts before `~AttachTask()` runs, so the worker leaks `thread_local_handle_count` and keeps a stale signal task id. Also, the `rc == nullptr` case still dereferences `rc` earlier in `AttachTask::init()`, so this branch does not actually diagnose that path. The validation needs to happen before the TLS / signal mutation, or `init()` needs rollback on exception.
##########
be/src/exec/pipeline/pipeline_task.cpp:
##########
@@ -507,6 +508,16 @@ Status PipelineTask::execute(bool* done) {
// called. This relies on _wake_up_early and _always_ready both being std::atomic with the
// default seq_cst ordering — do not weaken them to relaxed or acq/rel.
if (_wake_up_early) {
+ // Sanity-check the pointers we are about to dereference so the failure
+ // mode tells us *which* pointer is null instead of an opaque NPE inside
+ // the operator's terminate() call. Also dump the query_id for triage.
Review Comment:
Blocking: this does not actually give a stable error for the null cases. `_do_work()` catches the exception from `execute()`, but its cleanup path always runs `close_task(task.get(), status, ...)`. That immediately does `SCOPED_ATTACH_TASK(task->runtime_state())` and then `task->close()`. So if `_state` is null we now throw again from `close_task()` with no catch in the worker thread, and if `_sink` is null `PipelineTask::close()` still dereferences it. The scheduler / close path has to be made null-safe before this branch can safely replace the original NPE.
##########
be/src/runtime/memory/thread_mem_tracker_mgr.cpp:
##########
@@ -54,7 +65,25 @@ void ThreadMemTrackerMgr::detach_limiter_tracker() {
CHECK(init());
flush_untracked_mem();
shrink_reserved();
Review Comment:
Blocking: `detach_limiter_tracker()` is the teardown path for `~AttachTask()`, `~SwitchResourceContext()`, and `~SwitchThreadMemTrackerLimiter()`. Those destructors are implicitly `noexcept`, so these new `throw Exception(...)` branches will call `std::terminate` before any outer `ASSIGN_STATUS_IF_CATCH_EXCEPTION` / RPC handler can translate the error. Before this patch the release build only had the existing `DCHECK`; after this patch an unbalanced attach/detach becomes an unconditional process abort from RAII cleanup. If this invariant is meant to be fatal, it needs a non-throwing fatal path here, not an exception from destructor-backed cleanup.
##########
be/src/runtime/memory/thread_mem_tracker_mgr.h:
##########
@@ -101,6 +102,15 @@ class ThreadMemTrackerMgr {
MemTrackerLimiter* limiter_mem_tracker() {
CHECK(init());
Review Comment:
Blocking: `limiter_mem_tracker()` is called from `core/allocator.cpp` on the allocator / memory-hook path (`sys_memory_exceed()`, `memory_tracker_exceed()`, ASAN helpers, etc.). Building `Status::FatalError` + `Exception` here allocates strings, gathers a stack trace, and logs exactly when `_limiter_tracker` is already poisoned. Those allocations re-enter the Doris allocator / memory hook, so this diagnostic path can recurse or crash before it ever returns a clean message. This needs an allocation-free failure path (or earlier validation before we reach allocator code), not a normal exception object.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62691:
URL: https://github.com/apache/doris/pull/62691#discussion_r3122774894
##########
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)) {
Review Comment:
These new `throw Exception(Status::FatalError(...))` checks change `SCOPED_ATTACH_TASK(...)` from a local crash into an exception, but several existing attach sites run at plain async callback entry points that do not have a catch-and-convert boundary above them. For example, `SchemaScanner::get_next_block_async()` and `AsyncResultWriter::start_writer()` call `SCOPED_ATTACH_TASK(state)` inside lambdas submitted to `FragmentMgr`'s `ThreadPool`, and `ThreadPool::dispatch_thread()` just executes `task.runnable->run()` without catching `doris::Exception`. If one of these new checks fires, the exception escapes the worker thread and `std::terminate`s the whole BE instead of failing only that query. Could we either add the required exception boundary around every async attach entry or keep these diagnostics in a non-throwing form at those entry points?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4293273584
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt merged PR #62691:
URL: https://github.com/apache/doris/pull/62691
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4302498332
PR approved by at least one committer and no changes requested.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4293284299
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62691:
URL: https://github.com/apache/doris/pull/62691#discussion_r3122238504
##########
be/src/runtime/memory/thread_mem_tracker_mgr.cpp:
##########
@@ -19,13 +19,32 @@
#include <gen_cpp/types.pb.h>
+#include "common/exception.h"
+#include "common/signal_handler.h"
#include "runtime/exec_env.h"
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. Embed the full
+ // ThreadMemTrackerMgr state (consumer stack, untracked_mem,
+ // reserved_mem) so the FATAL message alone is enough to triage. The
+ // query id is read from signal-handler thread-local storage; it
+ // does not depend on any object that may already be torn down.
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::attach_limiter_tracker called with null mem_tracker. "
+ "previous limiter label={}, snapshot_stack_depth={}, "
+ "query_id={:x}-{:x}, mgr_state={{{}}}",
+ _limiter_tracker ? _limiter_tracker->label() : "<null>",
+ _last_attach_snapshots_stack.size(), doris::signal::query_id_hi,
+ doris::signal::query_id_lo, print_debug_string()));
Review Comment:
Calling `print_debug_string()` here defeats the new diagnostic on the exact `nullptr` case this branch is trying to catch. `attach_limiter_tracker()` does this check before `CHECK(init())`, so on a fresh `ThreadMemTrackerMgr` `_limiter_tracker` is still null. `print_debug_string()` unconditionally dereferences `_limiter_tracker->make_profile_str()`, which means `attach_limiter_tracker(nullptr)` will segfault while formatting the error message instead of throwing the intended `FatalError`.
##########
be/src/runtime/thread_context.cpp:
##########
@@ -84,6 +161,16 @@ SwitchResourceContext::~SwitchResourceContext() {
SwitchThreadMemTrackerLimiter::SwitchThreadMemTrackerLimiter(
Review Comment:
`SwitchThreadMemTrackerLimiter` is not the only public entry point that can feed a tracker into `attach_limiter_tracker()`. `AttachTask(const std::shared_ptr<MemTrackerLimiter>&)` still does `rc->memory_context()->set_mem_tracker(mem_tracker)` with no null check, and `MemoryContext::set_mem_tracker()` dereferences `mem_tracker_->limit()` immediately. So every existing `SCOPED_ATTACH_TASK(mem_tracker)` call site still crashes before any of the new diagnostics run if a null tracker slips through.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "Thearas (via GitHub)" <gi...@apache.org>.
Thearas commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4293273362
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4293273370
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4294312037
# BE Regression && UT Coverage Report
Increment line coverage `63.81% (67/105)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62691_c93c7e65c08d3e48c75202e4333cb14fd77dec11_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62691_c93c7e65c08d3e48c75202e4333cb14fd77dec11_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.51% (27498/37406) |
| Line Coverage | 57.28% (297284/518992) |
| Region Coverage | 54.42% (247141/454108) |
| Branch Coverage | 56.04% (107022/190987) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #62691:
URL: https://github.com/apache/doris/pull/62691#discussion_r3121377903
##########
be/src/runtime/memory/thread_mem_tracker_mgr.cpp:
##########
@@ -19,13 +19,24 @@
#include <gen_cpp/types.pb.h>
+#include "common/exception.h"
#include "runtime/exec_env.h"
namespace doris {
void ThreadMemTrackerMgr::attach_limiter_tracker(
const std::shared_ptr<MemTrackerLimiter>& mem_tracker) {
DCHECK(mem_tracker);
+ if (UNLIKELY(!mem_tracker)) {
+ // Catch the null in RELEASE. Without this, _limiter_tracker silently
+ // becomes null and any later allocation on this thread NPEs deep inside
+ // Allocator::memory_tracker_exceed (allocator.cpp:211) with no clue.
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::attach_limiter_tracker called with null mem_tracker. "
+ "previous limiter label={}, snapshot_stack_depth={}",
+ _limiter_tracker ? _limiter_tracker->label() : "<null>",
+ _last_attach_snapshots_stack.size()));
+ }
Review Comment:
New RELEASE-mode misuse checks were added (null `mem_tracker` on attach, empty snapshot stack / null snapshot limiter on detach), but there are existing unit tests for `ThreadMemTrackerMgr` (e.g. `be/test/runtime/memory/thread_mem_tracker_mgr_test.cpp`). Consider adding negative tests that assert these misuse cases fail in the expected way (Exception/FatalError), to prevent regressions and document the intended contract.
##########
be/src/runtime/memory/thread_mem_tracker_mgr.cpp:
##########
@@ -54,7 +65,25 @@ void ThreadMemTrackerMgr::detach_limiter_tracker() {
CHECK(init());
flush_untracked_mem();
shrink_reserved();
+ if (UNLIKELY(_last_attach_snapshots_stack.empty())) {
+ // attach/detach must be balanced. Detaching from an empty stack would let
+ // _limiter_tracker_sptr become a default-constructed null shared_ptr below,
+ // poisoning the thread tracker for any future allocation.
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::detach_limiter_tracker called with empty snapshot stack. "
+ "current limiter label={}",
+ _limiter_tracker ? _limiter_tracker->label() : "<null>"));
+ }
DCHECK(!_last_attach_snapshots_stack.empty());
+ if (UNLIKELY(!_last_attach_snapshots_stack.back().limiter_tracker)) {
+ // The snapshot saved a null limiter (means a prior attach with null mem_tracker
+ // already poisoned the chain — earlier patch in attach_limiter_tracker should
+ // have caught it; this is a second-line defense).
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::detach_limiter_tracker restored null limiter from snapshot. "
+ "stack_depth_before_pop={}",
+ _last_attach_snapshots_stack.size()));
+ }
Review Comment:
Same concern as above: throwing from `detach_limiter_tracker()` (restoring a null limiter from snapshot) can unwind through destructors that are `noexcept(true)` and trigger `std::terminate`. If you want this to be a hard-fail, prefer `CHECK`/`LOG(FATAL)`; otherwise make the RAII destructors `noexcept(false)` and rely on the existing exception-to-Status conversion paths.
```suggestion
CHECK(!_last_attach_snapshots_stack.empty())
<< "ThreadMemTrackerMgr::detach_limiter_tracker called with empty snapshot stack. "
<< "current limiter label="
<< (_limiter_tracker ? _limiter_tracker->label() : "<null>");
DCHECK(!_last_attach_snapshots_stack.empty());
CHECK(_last_attach_snapshots_stack.back().limiter_tracker)
<< "ThreadMemTrackerMgr::detach_limiter_tracker restored null limiter from snapshot. "
<< "stack_depth_before_pop=" << _last_attach_snapshots_stack.size();
```
##########
be/src/runtime/memory/thread_mem_tracker_mgr.cpp:
##########
@@ -54,7 +65,25 @@ void ThreadMemTrackerMgr::detach_limiter_tracker() {
CHECK(init());
flush_untracked_mem();
shrink_reserved();
+ if (UNLIKELY(_last_attach_snapshots_stack.empty())) {
+ // attach/detach must be balanced. Detaching from an empty stack would let
+ // _limiter_tracker_sptr become a default-constructed null shared_ptr below,
+ // poisoning the thread tracker for any future allocation.
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::detach_limiter_tracker called with empty snapshot stack. "
+ "current limiter label={}",
+ _limiter_tracker ? _limiter_tracker->label() : "<null>"));
+ }
DCHECK(!_last_attach_snapshots_stack.empty());
+ if (UNLIKELY(!_last_attach_snapshots_stack.back().limiter_tracker)) {
+ // The snapshot saved a null limiter (means a prior attach with null mem_tracker
+ // already poisoned the chain — earlier patch in attach_limiter_tracker should
+ // have caught it; this is a second-line defense).
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::detach_limiter_tracker restored null limiter from snapshot. "
+ "stack_depth_before_pop={}",
+ _last_attach_snapshots_stack.size()));
+ }
Review Comment:
`detach_limiter_tracker()` throws `Exception` on misuse, but it is called from several RAII destructors (e.g. `ThreadContext::detach_task()`, `SwitchResourceContext::~SwitchResourceContext()`, `SwitchThreadMemTrackerLimiter::~SwitchThreadMemTrackerLimiter()`). Those destructors are implicitly `noexcept(true)`, so if this path is hit it will call `std::terminate` and may lose the intended diagnostic. Prefer using `CHECK`/`LOG(FATAL)` here, or change the RAII destructors to `noexcept(false)` and ensure call sites catch and convert the exception to `Status`.
```suggestion
CHECK(!_last_attach_snapshots_stack.empty())
<< "ThreadMemTrackerMgr::detach_limiter_tracker called with empty snapshot stack. "
<< "current limiter label="
<< (_limiter_tracker ? _limiter_tracker->label() : "<null>");
DCHECK(!_last_attach_snapshots_stack.empty());
CHECK(_last_attach_snapshots_stack.back().limiter_tracker != nullptr)
<< "ThreadMemTrackerMgr::detach_limiter_tracker restored null limiter from "
"snapshot. stack_depth_before_pop="
<< _last_attach_snapshots_stack.size();
```
##########
be/src/runtime/thread_context.cpp:
##########
@@ -64,6 +79,21 @@ SwitchResourceContext::SwitchResourceContext(const std::shared_ptr<ResourceConte
DCHECK(thread_context()->is_attach_task());
old_resource_ctx_ = thread_context()->resource_ctx();
if (rc != old_resource_ctx_) {
+ // Symmetric to the checks in ThreadContext::attach_task — SwitchResourceContext
+ // also reaches attach_limiter_tracker() and can silently propagate a null
+ // mem_tracker if any link in the chain is empty in RELEASE.
Review Comment:
This comment mentions null `mem_tracker` being "silently" propagated in RELEASE, but `attach_limiter_tracker()` now throws on null input. Consider updating the wording to avoid implying the failure would be silent with the current code.
```suggestion
// also reaches attach_limiter_tracker(), so validate each link explicitly in
// RELEASE and throw a precise FatalError before calling it if any link is null.
```
##########
be/src/runtime/memory/thread_mem_tracker_mgr.h:
##########
@@ -101,6 +102,15 @@ class ThreadMemTrackerMgr {
MemTrackerLimiter* limiter_mem_tracker() {
CHECK(init());
+ if (UNLIKELY(_limiter_tracker == nullptr)) {
+ throw Exception(Status::FatalError(
+ "ThreadMemTrackerMgr::limiter_mem_tracker() returns nullptr. "
+ "This means a previous attach_limiter_tracker() received a null "
+ "shared_ptr (silent in RELEASE because DCHECK is no-op). "
Review Comment:
The FatalError text here says a null `shared_ptr` passed to `attach_limiter_tracker()` would be "silent in RELEASE because DCHECK is no-op", but this PR also adds an explicit RELEASE-mode throw in `attach_limiter_tracker()` for null input. Consider updating the message to reflect the new behavior (e.g., indicate this implies a prior poisoning/ABI mismatch/memory corruption, rather than a simple DCHECK being compiled out).
```suggestion
"attach_limiter_tracker() should reject null shared_ptr in both "
"DEBUG and RELEASE builds, so this implies an earlier invariant "
"violation such as poisoning, ABI mismatch, or memory corruption. "
```
##########
be/src/runtime/thread_context.h:
##########
@@ -168,6 +168,25 @@ class ThreadContext {
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);
+ // Disambiguate which sub-object is null when attach silently propagates a null
+ // mem_tracker (see attach_limiter_tracker). These checks tell us at the source
+ // whether `rc` itself is empty, the embedded MemoryContext is null, or the
+ // MemoryContext exists but its mem_tracker_ was never set
+ // (i.e. _init_query_mem_tracker hasn't run yet for this ResourceContext).
Review Comment:
The comment says attach can "silently propagate a null mem_tracker" in RELEASE, but with this PR `ThreadMemTrackerMgr::attach_limiter_tracker()` now throws on null input. Consider rewording to reflect that this used to be silent (pre-change) and that these checks are meant to pinpoint the source earlier.
```suggestion
// Historically, attach could reach attach_limiter_tracker with a null
// mem_tracker and the failure was harder to attribute. These checks now
// fail earlier and make it clear whether `rc` itself is empty, the
// embedded MemoryContext is null, or the MemoryContext exists but its
// mem_tracker_ was never set (i.e. _init_query_mem_tracker hasn't run
// yet for this ResourceContext).
```
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "BiteTheDDDDt (via GitHub)" <gi...@apache.org>.
BiteTheDDDDt commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4294449465
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [Chore](check) add some Sanity-check for avoid npe [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62691:
URL: https://github.com/apache/doris/pull/62691#issuecomment-4297632509
# BE Regression && UT Coverage Report
Increment line coverage `18.35% (20/109)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62691_971510195d0fdac62b9a52da6ee198d4be779f97_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62691_971510195d0fdac62b9a52da6ee198d4be779f97_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.50% (27494/37407) |
| Line Coverage | 57.26% (297205/519052) |
| Region Coverage | 54.38% (247006/454221) |
| Branch Coverage | 56.01% (106986/191020) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org