From 030c445ac967054937a0a7c3b17c6b01a38e767c Mon Sep 17 00:00:00 2001 From: Thomas Tauber-Marshall Date: Wed, 13 Mar 2019 20:38:48 +0000 Subject: [PATCH] IMPALA-8299: Fix crash in GroupingAggregator on uninited hash table If HashTable::Init() fails, in some cases we may try to call HashTable::Close() on the table in GroupingAggregator::Close(), which can cause a crash. The solution is to set the hash table to a nullptr if Init() fails. I also verified that the other use of HashTable, PhjBuilder, handles this case correctly. Testing: - Manually verified that the crash no longer occurs if HashTable::Init fails. Change-Id: I381657c43f93eb4871b93d54532cb886198fd0b2 Reviewed-on: http://gerrit.cloudera.org:8080/12746 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- be/src/exec/grouping-aggregator-partition.cc | 7 ++++++- be/src/exec/grouping-aggregator.h | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/be/src/exec/grouping-aggregator-partition.cc b/be/src/exec/grouping-aggregator-partition.cc index 8fe08f4e8..8be932d70 100644 --- a/be/src/exec/grouping-aggregator-partition.cc +++ b/be/src/exec/grouping-aggregator-partition.cc @@ -89,7 +89,12 @@ Status GroupingAggregator::Partition::InitHashTable(bool* got_memory) { 1L << (32 - NUM_PARTITIONING_BITS), PAGG_DEFAULT_HASH_TABLE_SZ)); // Please update the error message in CreateHashPartitions() if initial size of // hash table changes. - return hash_tbl->Init(got_memory); + Status status = hash_tbl->Init(got_memory); + if (!status.ok() || !(*got_memory)) { + hash_tbl->Close(); + hash_tbl.reset(); + } + return status; } Status GroupingAggregator::Partition::SerializeStreamForSpilling() { diff --git a/be/src/exec/grouping-aggregator.h b/be/src/exec/grouping-aggregator.h index 405d4ab26..258ed6ed0 100644 --- a/be/src/exec/grouping-aggregator.h +++ b/be/src/exec/grouping-aggregator.h @@ -346,6 +346,8 @@ class GroupingAggregator : public Aggregator { /// Initializes the hash table. 'aggregated_row_stream' must be non-NULL. /// Sets 'got_memory' to true if the hash table was initialised or false on OOM. + /// After returning, 'hash_tbl' will be non-null iff 'got_memory' is true and the + /// returned status is OK. Status InitHashTable(bool* got_memory) WARN_UNUSED_RESULT; /// Called in case we need to serialize aggregated rows. This step effectively does