[impala] 02/03: IMPALA-8850: is_percent not initialized in TmpFileMgr parser

[ta...@apache.org]

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

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

commit f34a243a956dacd52f8afe79a122410ccf45ae26
Author: Tim Armstrong <ta...@cloudera.com>
AuthorDate: Thu Aug 8 17:34:03 2019 -0700

    IMPALA-8850: is_percent not initialized in TmpFileMgr parser
    
    This bug was only detected on ASAN. The is_percent variable
    was not initialised and is not set in ParseMemSpec() when it
    does an early exit for an empty string. This caused
    TmpFileMgr to consider the empty memory spec after the colon
    to be invalid.
    
    Testing:
    This reliably reproduced in TmpFileMgrTest under ASAN.
    Initialising is_percent fixed the issue.
    
    Added more detailed tests for ParseMemSpec() that initialise
    is_percent to avoid similar bugs.
    
    Change-Id: I4465944c7043c1fc6b33db9693ec290c24e3ed19
    Reviewed-on: http://gerrit.cloudera.org:8080/14040
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/util/parse-util-test.cc | 23 +++++++++++++++++++++++
 be/src/util/parse-util.cc      |  2 +-
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/be/src/util/parse-util-test.cc b/be/src/util/parse-util-test.cc
index c8fb4a0..4d2a5ac 100644
--- a/be/src/util/parse-util-test.cc
+++ b/be/src/util/parse-util-test.cc
@@ -37,54 +37,69 @@ TEST(ParseMemSpecs, Basic) {
   int64_t gigabytes = 1024 * megabytes;
   int64_t terabytes = 1024 * gigabytes;
 
+  // Initialize 'is_percent' to the opposite of the expected value, to confirm that
+  // ParseMemSpec() is actually setting the output parameter.
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("1", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(1, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("100b", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(100, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("100kb", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(100 * 1024, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("5KB", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(5 * 1024, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("4MB", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(4 * megabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("4m", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(4 * megabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("8gb", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(8 * gigabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("8G", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(8 * gigabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("12Gb", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(12 * gigabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("8T", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(8 * terabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("12tb", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(12 * terabytes, bytes);
   ASSERT_FALSE(is_percent);
 
+  is_percent = false;
   bytes = ParseUtil::ParseMemSpec("13%", &is_percent, MemInfo::physical_mem());
   ASSERT_GT(bytes, 0);
   ASSERT_TRUE(is_percent);
 
+  is_percent = false;
   ASSERT_GT(ParseUtil::ParseMemSpec("17%", &is_percent, MemInfo::physical_mem()), bytes);
   ASSERT_EQ(ParseUtil::ParseMemSpec("17%", &is_percent, 100), 17);
   ASSERT_TRUE(is_percent);
@@ -111,17 +126,25 @@ TEST(ParseMemSpecs, Basic) {
     ASSERT_EQ(-1, bytes);
   }
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(0, bytes);
+  ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("-1", &is_percent, MemInfo::physical_mem());
   ASSERT_EQ(0, bytes);
+  ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("-2", &is_percent, MemInfo::physical_mem());
   ASSERT_LT(bytes, 0);
+  ASSERT_FALSE(is_percent);
 
+  is_percent = true;
   bytes = ParseUtil::ParseMemSpec("-2%", &is_percent, MemInfo::physical_mem());
   ASSERT_LT(bytes, 0);
+  ASSERT_TRUE(is_percent);
 }
 
 }
diff --git a/be/src/util/parse-util.cc b/be/src/util/parse-util.cc
index 75253d7..a70db99 100644
--- a/be/src/util/parse-util.cc
+++ b/be/src/util/parse-util.cc
@@ -25,9 +25,9 @@ namespace impala {
 
 int64_t ParseUtil::ParseMemSpec(const string& mem_spec_str, bool* is_percent,
     int64_t relative_reference) {
+  *is_percent = false;
   if (mem_spec_str.empty()) return 0;
 
-  *is_percent = false;
   int64_t multiplier = -1;
   int32_t number_str_len = mem_spec_str.size();