(impala) branch master updated: IMPALA-12575: test_executor_group_num_queries_executing_metric fails in UBSAN build

[stigahuang]

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

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


The following commit(s) were added to refs/heads/master by this push:
     new 6e90dafcf IMPALA-12575: 
test_executor_group_num_queries_executing_metric fails in UBSAN build
6e90dafcf is described below

commit 6e90dafcf4b8528061c5ae20933e112c355da834
Author: Zoltan Borok-Nagy <borokna...@cloudera.com>
AuthorDate: Thu Nov 23 21:02:58 2023 +0100

    IMPALA-12575: test_executor_group_num_queries_executing_metric fails in 
UBSAN build
    
    test_executor_group_num_queries_executing_metric fails in UBSAN builds
    with the Small String Optimization. It fails because it is possible to
    invoke Smallify() on a StringValue where ptr is nullptr and len = 0. In
    this case we call 'memcpy(rep.small_rep.buf, nullptr, 0)' internally
    which is undefined behavior according to the specs. The memcpy we use
    doesn't do any harm though.
    
    This patch substitutes mempcy() to Ubsan::MemCpy() which does null
    checking and only invokes memcpy() if neither of its arguments are
    nulls.
    
    There are other memcpy() invocations in SmallableString, but these
    only make sense if neither of their arguments are nulls, so it's
    good to have UBSAN-validation for them.
    
    Testing:
     * tested in UBSAN build
     * added backend test for this case
    
    Change-Id: Id937b9a975ac657e63ddb848e8c61eee75cfe17d
    Reviewed-on: http://gerrit.cloudera.org:8080/20726
    Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
    Reviewed-by: Csaba Ringhofer <csringho...@cloudera.com>
---
 be/src/runtime/smallable-string.h   | 4 +++-
 be/src/runtime/string-value-test.cc | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/be/src/runtime/smallable-string.h 
b/be/src/runtime/smallable-string.h
index 79705d6cd..5b8da44b5 100644
--- a/be/src/runtime/smallable-string.h
+++ b/be/src/runtime/smallable-string.h
@@ -19,6 +19,8 @@
 
 #include <string>
 
+#include "util/ubsan.h"
+
 #include "common/logging.h"
 
 namespace impala {
@@ -119,7 +121,7 @@ class __attribute__((__packed__)) SmallableString {
     // Let's zero out the object so compression algorithms will be more 
efficient
     // on small strings (there will be no garbage between string data and len).
     memset(this, 0, sizeof(*this));
-    memcpy(rep.small_rep.buf, s, len);
+    Ubsan::MemCpy(rep.small_rep.buf, s, len);
     SetSmallLen(len);
     return true;
   }
diff --git a/be/src/runtime/string-value-test.cc 
b/be/src/runtime/string-value-test.cc
index 0f9e83e36..a68dc2052 100644
--- a/be/src/runtime/string-value-test.cc
+++ b/be/src/runtime/string-value-test.cc
@@ -263,21 +263,27 @@ TEST(StringValueTest, TestConstructors) {
 }
 
 TEST(StringValueTest, TestSmallify) {
+  StringValue nullstr;
   StringValue empty(const_cast<char*>(""), 0);
   StringValue one_char(const_cast<char*>("a"), 1);
   StringValue limit(const_cast<char*>("0123456789A"), 11);
   StringValue over_the_limit(const_cast<char*>("0123456789AB"), 12);
 
+  StringValue nullstr_clone(nullstr);
   StringValue empty_clone(empty);
   StringValue one_char_clone(one_char);
   StringValue limit_clone(limit);
   StringValue over_the_limit_clone(over_the_limit);
 
+  EXPECT_TRUE(nullstr.Smallify());
   EXPECT_TRUE(empty.Smallify());
   EXPECT_TRUE(one_char.Smallify());
   EXPECT_TRUE(limit.Smallify());
   EXPECT_FALSE(over_the_limit.Smallify());
 
+  EXPECT_EQ(nullstr, nullstr_clone);
+  EXPECT_NE(nullstr.Ptr(), nullstr_clone.Ptr());
+
   EXPECT_EQ(empty, empty_clone);
   EXPECT_NE(empty.Ptr(), empty_clone.Ptr());