This is an automated email from the ASF dual-hosted git repository. lv pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit a7a80d1241a09cfa1f73a80900b74a012de52641 Author: Jim Apple <jbap...@apache.org> AuthorDate: Sat Jun 29 22:00:52 2019 -0700 IMPALA-5031: method calls on NULL are not UBSAN-clean According to [expr.post] in the C++14 standard, a call to a member function like a->b() is interpreted as (a->b)(). In other words, the dereferencing is done separately from the call. This makes calling member functions on nullptr undefined behavior, since the dereference invokes undefined behavior. This fixes such an error in exec-node.cc in the end-to-end tests. The interesting part of the backtrace is: exec/exec-node.cc:396:27: runtime error: member call on null pointer of type 'MemTracker' #0 in ExecNode::ExecDebugActionImpl(TExecNodePhase::type, RuntimeState*) exec/exec-node.cc:396:27 #1 in ExecNode::ExecDebugAction(TExecNodePhase::type, RuntimeState*) exec/exec-node.h:379:12 #2 in ExecNode::Prepare(RuntimeState*) exec/exec-node.cc:106:43 #3 in TopNNode::Prepare(RuntimeState*) exec/topn-node.cc:75:53 Change-Id: Id62d1c504a273451dc1be6831a473f6c7115b403 Reviewed-on: http://gerrit.cloudera.org:8080/13769 Reviewed-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> --- be/src/exec/exec-node.cc | 3 ++- be/src/runtime/mem-tracker.cc | 25 +++++++++++++++---------- be/src/runtime/mem-tracker.h | 8 +++++++- 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/be/src/exec/exec-node.cc b/be/src/exec/exec-node.cc index 833bd86..c046d83 100644 --- a/be/src/exec/exec-node.cc +++ b/be/src/exec/exec-node.cc @@ -393,7 +393,8 @@ Status ExecNode::ExecDebugActionImpl(TExecNodePhase::type phase, RuntimeState* s ErrorMsg(TErrorCode::INTERNAL_ERROR, "Debug Action: INJECT_ERROR_LOG")); return Status::OK(); } else if (debug_options_.action == TDebugAction::MEM_LIMIT_EXCEEDED) { - return mem_tracker()->MemLimitExceeded(state, "Debug Action: MEM_LIMIT_EXCEEDED"); + return MemTracker::MemLimitExceeded( + mem_tracker(), state, "Debug Action: MEM_LIMIT_EXCEEDED"); } else if (debug_options_.action == TDebugAction::SET_DENY_RESERVATION_PROBABILITY) { // We can only enable the debug action if the buffer pool client is registered. // If the buffer client is not registered at this point (e.g. if phase is PREPARE or diff --git a/be/src/runtime/mem-tracker.cc b/be/src/runtime/mem-tracker.cc index f9ba8fe..6db0dc8 100644 --- a/be/src/runtime/mem-tracker.cc +++ b/be/src/runtime/mem-tracker.cc @@ -412,13 +412,14 @@ MemTracker* MemTracker::GetQueryMemTracker() { return tracker; } -Status MemTracker::MemLimitExceeded(RuntimeState* state, const std::string& details, - int64_t failed_allocation_size) { +Status MemTracker::MemLimitExceeded(MemTracker* mtracker, RuntimeState* state, + const std::string& details, int64_t failed_allocation_size) { DCHECK_GE(failed_allocation_size, 0); stringstream ss; if (details.size() != 0) ss << details << endl; if (failed_allocation_size != 0) { - ss << label() << " could not allocate " + if (mtracker != nullptr) ss << mtracker->label(); + ss << " could not allocate " << PrettyPrinter::Print(failed_allocation_size, TUnit::BYTES) << " without exceeding limit." << endl; } @@ -432,14 +433,18 @@ Status MemTracker::MemLimitExceeded(RuntimeState* state, const std::string& deta << PrettyPrinter::Print(process_capacity, TUnit::BYTES) << endl; // Always log the query tracker (if available). - MemTracker* query_tracker = GetQueryMemTracker(); - if (query_tracker != nullptr) { - if (query_tracker->has_limit()) { - const int64_t query_capacity = query_tracker->limit() - query_tracker->consumption(); - ss << "Memory left in query limit: " - << PrettyPrinter::Print(query_capacity, TUnit::BYTES) << endl; + MemTracker* query_tracker = nullptr; + if (mtracker != nullptr) { + query_tracker = mtracker->GetQueryMemTracker(); + if (query_tracker != nullptr) { + if (query_tracker->has_limit()) { + const int64_t query_capacity = + query_tracker->limit() - query_tracker->consumption(); + ss << "Memory left in query limit: " + << PrettyPrinter::Print(query_capacity, TUnit::BYTES) << endl; + } + ss << query_tracker->LogUsage(UNLIMITED_DEPTH); } - ss << query_tracker->LogUsage(UNLIMITED_DEPTH); } // Log the process level if the process tracker is close to the limit or diff --git a/be/src/runtime/mem-tracker.h b/be/src/runtime/mem-tracker.h index a0c1df7..52d15cc 100644 --- a/be/src/runtime/mem-tracker.h +++ b/be/src/runtime/mem-tracker.h @@ -351,7 +351,13 @@ class MemTracker { /// 'failed_allocation_size' is zero, nothing about the allocation size is logged. /// If 'state' is non-NULL, logs the error to 'state'. Status MemLimitExceeded(RuntimeState* state, const std::string& details, - int64_t failed_allocation = 0) WARN_UNUSED_RESULT; + int64_t failed_allocation = 0) WARN_UNUSED_RESULT { + return MemLimitExceeded(this, state, details, failed_allocation); + } + + /// Makes MemLimitExceeded callable for nullptr MemTrackers. + static Status MemLimitExceeded(MemTracker* mtracker, RuntimeState* state, + const std::string& details, int64_t failed_allocation = 0) WARN_UNUSED_RESULT; void set_query_exec_finished() { DCHECK(is_query_mem_tracker_);