Repository: incubator-impala Updated Branches: refs/heads/master 3307acfef -> 3a630a5d6
IMPALA-1619, IMPALA-3018: Address various small memory allocation related bugs This patch addresses a potential overflow in calculation FreePool::Rellocate() and its handling of zero-length allocations. This patch also adds code to gracefully handle malloc() failures when initializing/resizing hash tables. Change-Id: I6eb9a4472a65cf68edb0323b13d745277ead2e1d Reviewed-on: http://gerrit.cloudera.org:8080/3807 Reviewed-by: Tim Armstrong <[email protected]> Reviewed-by: Dan Hecht <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/5a55ba76 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/5a55ba76 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/5a55ba76 Branch: refs/heads/master Commit: 5a55ba76108443fb7a136d6a48fb95840ab7e5e6 Parents: 3307acf Author: Michael Ho <[email protected]> Authored: Wed Jul 27 12:22:43 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Tue Aug 16 04:27:11 2016 +0000 ---------------------------------------------------------------------- be/src/exec/hash-table.cc | 10 +++++++++- be/src/runtime/free-pool-test.cc | 5 +++++ be/src/runtime/free-pool.h | 4 ++-- 3 files changed, 16 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/exec/hash-table.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/hash-table.cc b/be/src/exec/hash-table.cc index 7c2eab1..f8df641 100644 --- a/be/src/exec/hash-table.cc +++ b/be/src/exec/hash-table.cc @@ -386,6 +386,11 @@ bool HashTable::Init() { return false; } buckets_ = reinterpret_cast<Bucket*>(malloc(buckets_byte_size)); + if (buckets_ == NULL) { + state_->block_mgr()->ReleaseMemory(block_mgr_client_, buckets_byte_size); + num_buckets_ = 0; + return false; + } memset(buckets_, 0, buckets_byte_size); return true; } @@ -438,7 +443,10 @@ bool HashTable::ResizeBuckets(int64_t num_buckets, const HashTableCtx* ht_ctx) { int64_t new_size = num_buckets * sizeof(Bucket); if (!state_->block_mgr()->ConsumeMemory(block_mgr_client_, new_size)) return false; Bucket* new_buckets = reinterpret_cast<Bucket*>(malloc(new_size)); - DCHECK(new_buckets != NULL); + if (new_buckets == NULL) { + state_->block_mgr()->ReleaseMemory(block_mgr_client_, new_size); + return false; + } memset(new_buckets, 0, new_size); // Walk the old table and copy all the filled buckets to the new (resized) table. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/runtime/free-pool-test.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/free-pool-test.cc b/be/src/runtime/free-pool-test.cc index d5eec25..1558683 100644 --- a/be/src/runtime/free-pool-test.cc +++ b/be/src/runtime/free-pool-test.cc @@ -187,6 +187,11 @@ TEST(FreePoolTest, ReAlloc) { EXPECT_TRUE(ptr3 != ptr4); EXPECT_EQ(mem_pool.total_allocated_bytes(), 1024 + 8 + 2048 + 8 + (1LL << 32) + 8); + // Shrink the allocation. + uint8_t* ptr5 = pool.Reallocate(ptr4, 1024); + EXPECT_TRUE(ptr4 == ptr5); + EXPECT_EQ(mem_pool.total_allocated_bytes(), 1024 + 8 + 2048 + 8 + (1LL << 32) + 8); + mem_pool.FreeAll(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5a55ba76/be/src/runtime/free-pool.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/free-pool.h b/be/src/runtime/free-pool.h index a5fe8e6..f37f433 100644 --- a/be/src/runtime/free-pool.h +++ b/be/src/runtime/free-pool.h @@ -123,10 +123,10 @@ class FreePool { return NULL; } #endif + if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return Allocate(size); if (FLAGS_disable_mem_pools) { return reinterpret_cast<uint8_t*>(realloc(reinterpret_cast<void*>(ptr), size)); } - if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return Allocate(size); FreeListNode* node = reinterpret_cast<FreeListNode*>(ptr - sizeof(FreeListNode)); FreeListNode* list = node->list; #ifndef NDEBUG @@ -135,7 +135,7 @@ class FreePool { int bucket_idx = (list - &lists_[0]); DCHECK_LT(bucket_idx, NUM_LISTS); // This is the actual size of ptr. - int allocation_size = 1 << bucket_idx; + int64_t allocation_size = 1LL << bucket_idx; // If it's already big enough, just return the ptr. if (allocation_size >= size) return ptr;
