[impala] 02/05: IMPALA-5031: method calls on NULL are not UBSAN-clean

[lv...@apache.org]

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 <jb...@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 <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@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_);