[impala] 16/27: IMPALA-11365: Dereferencing null pointer in TopNNode

[st...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

stigahuang pushed a commit to branch branch-4.1.1
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 822fcf32744d323f101ebf9b03ac7d869785ba62
Author: Daniel Becker <da...@cloudera.com>
AuthorDate: Thu Jun 16 16:44:08 2022 +0200

    IMPALA-11365: Dereferencing null pointer in TopNNode
    
    In the constructor of TopNNode, if 'pnode.partition_comparator_config_'
    is NULL, we initialise 'partition_cmp_' with a NULL pointer. However,
    when initialising 'partition_heaps_', we dereference 'partition_cmp_'
    because 'ComparatorWrapper' expects a reference.
    
    This has so far not lead to a crash because in this case the comparator
    of 'partition_heaps_' is not used, but assigning a NULL pointer to a
    reference is undefined behaviour.
    
    After this change, instead of assigning a NULL pointer to
    'partition_cmp_', we use a dummy comparator, and no undefined behaviour
    is invoked.
    
    Change-Id: I0b15b06f608b4d17fdf8a24e05967aaa16ebb79c
    Reviewed-on: http://gerrit.cloudera.org:8080/18629
    Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/exec/topn-node.cc        | 29 ++++++++++++++++++++++++++++-
 be/src/util/tuple-row-compare.h |  6 ++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/be/src/exec/topn-node.cc b/be/src/exec/topn-node.cc
index fc4318637..f65af7df3 100644
--- a/be/src/exec/topn-node.cc
+++ b/be/src/exec/topn-node.cc
@@ -121,6 +121,33 @@ Status TopNPlanNode::CreateExecNode(RuntimeState* state, ExecNode** node) const
   return Status::OK();
 }
 
+/// In the TopNNode constructor if 'pnode.partition_comparator_config_' is NULL, we use
+/// this dummy comparator to avoid 'partition_cmp_' becoming a null pointer. This is
+/// needed because 'partition_cmp_' is wrapped in a
+/// 'ComparatorWrapper<TupleRowComparator>' that takes a reference to the underlying
+/// comparator, so we cannot pass in a null pointer or dereference it.
+class DummyTupleRowComparator: public TupleRowComparator {
+ public:
+  DummyTupleRowComparator()
+   : TupleRowComparator(dummy_scalar_exprs_, dummy_codegend_compare_fn_) {
+  }
+ private:
+  static const std::vector<ScalarExpr*> dummy_scalar_exprs_;
+  static const CodegenFnPtr<TupleRowComparatorConfig::CompareFn>
+      dummy_codegend_compare_fn_;
+
+  int CompareInterpreted(const TupleRow* lhs, const TupleRow* rhs) const override {
+    // This function should never be called as this is a dummy comparator.
+    DCHECK(false);
+    return std::less<const TupleRow*>{}(lhs, rhs);
+  }
+};
+
+/// Initialise vector length to 0 so no buffer needs to be allocated.
+const std::vector<ScalarExpr*> DummyTupleRowComparator::dummy_scalar_exprs_{0};
+const CodegenFnPtr<TupleRowComparatorConfig::CompareFn>
+DummyTupleRowComparator::dummy_codegend_compare_fn_{};
+
 TopNNode::TopNNode(
     ObjectPool* pool, const TopNPlanNode& pnode, const DescriptorTbl& descs)
   : ExecNode(pool, pnode, descs),
@@ -129,7 +156,7 @@ TopNNode::TopNNode(
     output_tuple_desc_(pnode.output_tuple_desc_),
     order_cmp_(new TupleRowLexicalComparator(*pnode.ordering_comparator_config_)),
     partition_cmp_(pnode.partition_comparator_config_ == nullptr ?
-            nullptr :
+            static_cast<TupleRowComparator*>(new DummyTupleRowComparator()) :
             new TupleRowLexicalComparator(*pnode.partition_comparator_config_)),
     intra_partition_order_cmp_(pnode.intra_partition_comparator_config_ == nullptr ?
             nullptr :
diff --git a/be/src/util/tuple-row-compare.h b/be/src/util/tuple-row-compare.h
index 2a2be7d82..c17253cc3 100644
--- a/be/src/util/tuple-row-compare.h
+++ b/be/src/util/tuple-row-compare.h
@@ -179,6 +179,12 @@ class TupleRowComparator {
   static const char* LLVM_CLASS_NAME;
 
  protected:
+  TupleRowComparator(const std::vector<ScalarExpr*>& ordering_exprs,
+      const CodegenFnPtr<TupleRowComparatorConfig::CompareFn>& codegend_compare_fn)
+    : ordering_exprs_(ordering_exprs),
+      codegend_compare_fn_(codegend_compare_fn) {
+  }
+
   /// References to ordering expressions owned by the plan node which owns the
   /// TupleRowComparatorConfig used to create this instance.
   const std::vector<ScalarExpr*>& ordering_exprs_;