From f129dfd2022d268a5658bac9da279df79dd6c857 Mon Sep 17 00:00:00 2001 From: Michael Ho Date: Fri, 8 Jul 2016 14:39:44 -0700 Subject: [PATCH] IMPALA-3018: Don't return NULL on zero length allocations. FunctionContext::Allocate() and FunctionContext::AllocateLocal() used to return NULL for zero length allocations. This makes it hard to distinguish between allocation failures and zero length allocations. Such confusion may lead to DCHECK failure in the macro RETURN_IF_NULL() in debug builds or access to NULL pointers in non-debug builds. This change fixes the problem above by returning NULL only if there is allocation failure. Zero-length allocations will always return a dummy non-NULL pointer. Change-Id: Id8c3211f4d9417f44b8018ccc58ae182682693da Reviewed-on: http://gerrit.cloudera.org:8080/3601 Reviewed-by: Michael Ho Tested-by: Internal Jenkins --- be/src/exprs/aggregate-functions-ir.cc | 1 + be/src/runtime/free-pool.h | 8 +++---- be/src/udf/udf-test.cc | 5 ++-- be/src/udf/udf.cc | 2 -- .../queries/QueryTest/aggregation.test | 24 ++++++++++++++++--- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/be/src/exprs/aggregate-functions-ir.cc b/be/src/exprs/aggregate-functions-ir.cc index 79d7ac791..853661c4f 100644 --- a/be/src/exprs/aggregate-functions-ir.cc +++ b/be/src/exprs/aggregate-functions-ir.cc @@ -1388,6 +1388,7 @@ void AggregateFunctions::LastValUpdate(FunctionContext* ctx, const StringVal& sr } else { new_ptr = ctx->Reallocate(dst->ptr, src.len); } + // Note that a zero-length string is not the same as StringVal::null(). RETURN_IF_NULL(ctx, new_ptr); dst->ptr = new_ptr; memcpy(dst->ptr, src.ptr, src.len); diff --git a/be/src/runtime/free-pool.h b/be/src/runtime/free-pool.h index 90df74992..8762f1df2 100644 --- a/be/src/runtime/free-pool.h +++ b/be/src/runtime/free-pool.h @@ -62,12 +62,10 @@ class FreePool { return NULL; } #endif - ++net_allocations_; - if (FLAGS_disable_mem_pools) return reinterpret_cast(malloc(size)); - /// Return a non-NULL dummy pointer. NULL is reserved for failures. if (UNLIKELY(size == 0)) return mem_pool_->EmptyAllocPtr(); - + ++net_allocations_; + if (FLAGS_disable_mem_pools) return reinterpret_cast(malloc(size)); int free_list_idx = Bits::Log2Ceiling64(size); DCHECK_LT(free_list_idx, NUM_LISTS); FreeListNode* allocation = lists_[free_list_idx].next; @@ -92,12 +90,12 @@ class FreePool { } void Free(uint8_t* ptr) { + if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return; --net_allocations_; if (FLAGS_disable_mem_pools) { free(ptr); return; } - if (UNLIKELY(ptr == NULL || ptr == mem_pool_->EmptyAllocPtr())) return; FreeListNode* node = reinterpret_cast(ptr - sizeof(FreeListNode)); FreeListNode* list = node->list; #ifndef NDEBUG diff --git a/be/src/udf/udf-test.cc b/be/src/udf/udf-test.cc index 4857a0630..86f02c2cb 100644 --- a/be/src/udf/udf-test.cc +++ b/be/src/udf/udf-test.cc @@ -123,8 +123,9 @@ IntVal ValidateFail(FunctionContext* context) { } IntVal ValidateMem(FunctionContext* context) { - EXPECT_TRUE(context->Allocate(0) == NULL); - uint8_t* buffer = context->Allocate(10); + uint8_t* buffer = context->Allocate(0); + EXPECT_TRUE(buffer != NULL); + buffer = context->Reallocate(buffer, 10); EXPECT_TRUE(buffer != NULL); memset(buffer, 0, 10); context->Free(buffer); diff --git a/be/src/udf/udf.cc b/be/src/udf/udf.cc index ff81c9c04..c1fb5be42 100644 --- a/be/src/udf/udf.cc +++ b/be/src/udf/udf.cc @@ -286,7 +286,6 @@ inline bool FunctionContextImpl::CheckAllocResult(const char* fn_name, uint8_t* FunctionContext::Allocate(int byte_size) noexcept { assert(!impl_->closed_); - if (byte_size == 0) return NULL; uint8_t* buffer = impl_->pool_->Allocate(byte_size); if (UNLIKELY(!impl_->CheckAllocResult("FunctionContext::Allocate", buffer, byte_size))) { @@ -418,7 +417,6 @@ void FunctionContext::SetFunctionState(FunctionStateScope scope, void* ptr) { uint8_t* FunctionContextImpl::AllocateLocal(int64_t byte_size) noexcept { assert(!closed_); - if (byte_size == 0) return NULL; uint8_t* buffer = pool_->Allocate(byte_size); if (UNLIKELY(!CheckAllocResult("FunctionContextImpl::AllocateLocal", buffer, byte_size))) { diff --git a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test index 5389b04f3..740f777a6 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test +++ b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test @@ -154,13 +154,31 @@ from alltypesagg where day is not null bigint, string, string, string, string ==== ---- QUERY -# Test for IMPALA-3018. Verify update() function of min() handles +# Test for IMPALA-3018. Verify update() functions of min() and max() handle # zero-length string correctly. -select min(str) from (values ('aaa' as str), ('')) as tmp +select max(str), min(str) from (values ('aaa' as str), (''), ('123')) as tmp +---- RESULTS +'aaa','' +---- TYPES +string,string +==== +---- QUERY +# Test for IMPALA-3018. Verify update() function of last_value() handles +# zero-length string correctly. +select last_value(b) over (partition by a order by d) from functional.nulltable; ---- RESULTS '' ---- TYPES -String +string +==== +---- QUERY +# Test for IMPALA-3018. Verify update() function of first_value() handles +# zero-length string correctly. +select first_value(b) over (partition by a order by d) from functional.nulltable; +---- RESULTS +'' +---- TYPES +string ==== ---- QUERY # grouping by different data types, with NULLs