Repository: incubator-impala Updated Branches: refs/heads/master 50e3abdc3 -> bdad90e69
IMPALA-5246: MemTestClose() should handle Expr's preparation failure UDF may fail to initialize due exceeding memory limit or other reasons. In which case, its Prepare() function may not have been called and its thread local state may not be initialized. MemTestClose() in test-udf.cc made the wrong assumption that the thread local states are always initialized. This may lead to de-referencing null pointer in Close(). This change fixes this issue by checking the thread local state is not null and returns early if so. Also sets the fragment or thread local states in FunctionContext to nullptr after freeing them in various built-in's Close() functions. Change-Id: Id2c689246ed4f8dd38f104fa35904f3926a7039c Reviewed-on: http://gerrit.cloudera.org:8080/6757 Reviewed-by: Dan Hecht <[email protected]> Reviewed-by: Attila Jeges <[email protected]> Tested-by: Impala Public 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/c26a485a Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/c26a485a Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/c26a485a Branch: refs/heads/master Commit: c26a485afeebb74477f4f34303411614e3cb6921 Parents: 50e3abd Author: Michael Ho <[email protected]> Authored: Thu Apr 27 18:32:34 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu May 4 22:20:11 2017 +0000 ---------------------------------------------------------------------- be/src/exprs/case-expr.cc | 1 + be/src/exprs/hive-udf-call.cc | 3 ++- be/src/exprs/in-predicate.h | 1 + be/src/exprs/like-predicate.cc | 6 ++++-- be/src/exprs/math-functions-ir.cc | 1 + be/src/exprs/string-functions-ir.cc | 9 ++++++--- be/src/exprs/timestamp-functions.cc | 1 + be/src/exprs/udf-builtins-ir.cc | 12 ++++-------- be/src/exprs/utility-functions.cc | 2 +- be/src/testutil/test-udfs.cc | 10 ++++++---- be/src/udf/udf-test.cc | 2 +- 11 files changed, 28 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/case-expr.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/case-expr.cc b/be/src/exprs/case-expr.cc index 3c0db79..6847e82 100644 --- a/be/src/exprs/case-expr.cc +++ b/be/src/exprs/case-expr.cc @@ -80,6 +80,7 @@ void CaseExpr::Close(RuntimeState* state, ExprContext* ctx, FunctionContext* fn_ctx = ctx->fn_context(fn_context_index_); void* case_state = fn_ctx->GetFunctionState(FunctionContext::THREAD_LOCAL); fn_ctx->Free(reinterpret_cast<uint8_t*>(case_state)); + fn_ctx->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } Expr::Close(state, ctx, scope); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/hive-udf-call.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/hive-udf-call.cc b/be/src/exprs/hive-udf-call.cc index 169b603..d39e1bd 100644 --- a/be/src/exprs/hive-udf-call.cc +++ b/be/src/exprs/hive-udf-call.cc @@ -237,7 +237,7 @@ Status HiveUdfCall::Open(RuntimeState* state, ExprContext* ctx, } void HiveUdfCall::Close(RuntimeState* state, ExprContext* ctx, - FunctionContext::FunctionStateScope scope) { + FunctionContext::FunctionStateScope scope) { if (fn_context_index_ != -1) { FunctionContext* fn_ctx = ctx->fn_context(fn_context_index_); JniContext* jni_ctx = reinterpret_cast<JniContext*>( @@ -265,6 +265,7 @@ void HiveUdfCall::Close(RuntimeState* state, ExprContext* ctx, } jni_ctx->output_anyval = NULL; delete jni_ctx; + fn_ctx->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } else { DCHECK(!ctx->opened_); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/in-predicate.h ---------------------------------------------------------------------- diff --git a/be/src/exprs/in-predicate.h b/be/src/exprs/in-predicate.h index 52d2015..087c0e6 100644 --- a/be/src/exprs/in-predicate.h +++ b/be/src/exprs/in-predicate.h @@ -367,6 +367,7 @@ void InPredicate::SetLookupClose( SetLookupState<SetType>* state = reinterpret_cast<SetLookupState<SetType>*>(ctx->GetFunctionState(scope)); delete state; + ctx->SetFunctionState(scope, nullptr); } template <typename T, typename SetType> http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/like-predicate.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/like-predicate.cc b/be/src/exprs/like-predicate.cc index 18e0f9a..f479f12 100644 --- a/be/src/exprs/like-predicate.cc +++ b/be/src/exprs/like-predicate.cc @@ -110,9 +110,10 @@ void LikePredicate::LikePrepareInternal(FunctionContext* context, void LikePredicate::LikeClose(FunctionContext* context, FunctionContext::FunctionStateScope scope) { if (scope == FunctionContext::THREAD_LOCAL) { - LikePredicateState* state = reinterpret_cast<LikePredicateState*>( - context->GetFunctionState(FunctionContext::THREAD_LOCAL)); + LikePredicateState* state = reinterpret_cast<LikePredicateState*>( + context->GetFunctionState(FunctionContext::THREAD_LOCAL)); delete state; + context->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } } @@ -240,6 +241,7 @@ void LikePredicate::RegexClose(FunctionContext* context, LikePredicateState* state = reinterpret_cast<LikePredicateState*>( context->GetFunctionState(FunctionContext::THREAD_LOCAL)); delete state; + context->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/math-functions-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/math-functions-ir.cc b/be/src/exprs/math-functions-ir.cc index 6a331ba..0c76c4a 100644 --- a/be/src/exprs/math-functions-ir.cc +++ b/be/src/exprs/math-functions-ir.cc @@ -180,6 +180,7 @@ void MathFunctions::RandClose(FunctionContext* ctx, uint8_t* seed = reinterpret_cast<uint8_t*>( ctx->GetFunctionState(FunctionContext::THREAD_LOCAL)); ctx->Free(seed); + ctx->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/string-functions-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/string-functions-ir.cc b/be/src/exprs/string-functions-ir.cc index dcef9cf..0a05739 100644 --- a/be/src/exprs/string-functions-ir.cc +++ b/be/src/exprs/string-functions-ir.cc @@ -236,7 +236,8 @@ void StringFunctions::ReplaceClose(FunctionContext* context, if (scope != FunctionContext::FRAGMENT_LOCAL) return; ReplaceContext* rptr = reinterpret_cast<ReplaceContext*> (context->GetFunctionState(FunctionContext::FRAGMENT_LOCAL)); - if (rptr != nullptr) context->Free(reinterpret_cast<uint8_t*>(rptr)); + context->Free(reinterpret_cast<uint8_t*>(rptr)); + context->SetFunctionState(scope, nullptr); } StringVal StringFunctions::Replace(FunctionContext* context, const StringVal& str, @@ -612,6 +613,7 @@ void StringFunctions::RegexpClose( if (scope != FunctionContext::FRAGMENT_LOCAL) return; re2::RE2* re = reinterpret_cast<re2::RE2*>(context->GetFunctionState(scope)); delete re; + context->SetFunctionState(scope, nullptr); } StringVal StringFunctions::RegexpExtract(FunctionContext* context, const StringVal& str, @@ -879,8 +881,8 @@ void StringFunctions::ParseUrlClose( if (scope != FunctionContext::FRAGMENT_LOCAL) return; UrlParser::UrlPart* url_part = reinterpret_cast<UrlParser::UrlPart*>(ctx->GetFunctionState(scope)); - if (url_part == NULL) return; delete url_part; + ctx->SetFunctionState(scope, nullptr); } StringVal StringFunctions::ParseUrlKey(FunctionContext* ctx, const StringVal& url, @@ -941,7 +943,8 @@ void StringFunctions::BTrimClose( if (scope != FunctionContext::THREAD_LOCAL) return; bitset<256>* unique_chars = reinterpret_cast<bitset<256>*>( context->GetFunctionState(scope)); - if (unique_chars != NULL) delete unique_chars; + delete unique_chars; + context->SetFunctionState(scope, nullptr); } StringVal StringFunctions::BTrimString(FunctionContext* ctx, http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/timestamp-functions.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/timestamp-functions.cc b/be/src/exprs/timestamp-functions.cc index b2a33d4..b45610e 100644 --- a/be/src/exprs/timestamp-functions.cc +++ b/be/src/exprs/timestamp-functions.cc @@ -193,6 +193,7 @@ void TimestampFunctions::UnixAndFromUnixClose(FunctionContext* context, DateTimeFormatContext* dt_ctx = reinterpret_cast<DateTimeFormatContext*>(context->GetFunctionState(scope)); delete dt_ctx; + context->SetFunctionState(scope, nullptr); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/udf-builtins-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/udf-builtins-ir.cc b/be/src/exprs/udf-builtins-ir.cc index 452c5b4..40638f8 100644 --- a/be/src/exprs/udf-builtins-ir.cc +++ b/be/src/exprs/udf-builtins-ir.cc @@ -330,10 +330,8 @@ void UdfBuiltins::TruncPrepare(FunctionContext* ctx, void UdfBuiltins::TruncClose(FunctionContext* ctx, FunctionContext::FunctionStateScope scope) { void* state = ctx->GetFunctionState(scope); - if (state != NULL) { - ctx->Free(reinterpret_cast<uint8_t*>(state)); - ctx->SetFunctionState(scope, NULL); - } + ctx->Free(reinterpret_cast<uint8_t*>(state)); + ctx->SetFunctionState(scope, nullptr); } // Maps the user facing name of a unit to a TExtractField @@ -464,10 +462,8 @@ void UdfBuiltins::SwappedExtractPrepare(FunctionContext* ctx, void UdfBuiltins::ExtractClose(FunctionContext* ctx, FunctionContext::FunctionStateScope scope) { void* state = ctx->GetFunctionState(scope); - if (state != NULL) { - ctx->Free(reinterpret_cast<uint8_t*>(state)); - ctx->SetFunctionState(scope, NULL); - } + ctx->Free(reinterpret_cast<uint8_t*>(state)); + ctx->SetFunctionState(scope, nullptr); } bool ValidateMADlibVector(FunctionContext* context, const StringVal& arr) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/exprs/utility-functions.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/utility-functions.cc b/be/src/exprs/utility-functions.cc index 6159d6d..4fe41b6 100644 --- a/be/src/exprs/utility-functions.cc +++ b/be/src/exprs/utility-functions.cc @@ -56,8 +56,8 @@ void UtilityFunctions::UuidClose(FunctionContext* ctx, boost::uuids::random_generator* uuid_gen = reinterpret_cast<boost::uuids::random_generator*>( ctx->GetFunctionState(FunctionContext::THREAD_LOCAL)); - DCHECK(uuid_gen != NULL); delete uuid_gen; + ctx->SetFunctionState(FunctionContext::THREAD_LOCAL, nullptr); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/testutil/test-udfs.cc ---------------------------------------------------------------------- diff --git a/be/src/testutil/test-udfs.cc b/be/src/testutil/test-udfs.cc index 0c85c6e..894ce61 100644 --- a/be/src/testutil/test-udfs.cc +++ b/be/src/testutil/test-udfs.cc @@ -277,7 +277,7 @@ void CountClose(FunctionContext* context, FunctionContext::FunctionStateScope sc if (scope == FunctionContext::THREAD_LOCAL) { void* state = context->GetFunctionState(scope); context->Free(reinterpret_cast<uint8_t*>(state)); - context->SetFunctionState(scope, NULL); + context->SetFunctionState(scope, nullptr); } } @@ -306,7 +306,7 @@ void ConstantArgClose( if (scope == FunctionContext::THREAD_LOCAL) { void* state = context->GetFunctionState(scope); context->Free(reinterpret_cast<uint8_t*>(state)); - context->SetFunctionState(scope, NULL); + context->SetFunctionState(scope, nullptr); } } @@ -330,7 +330,7 @@ void ValidateOpenClose( if (scope == FunctionContext::THREAD_LOCAL) { void* state = context->GetFunctionState(scope); context->Free(reinterpret_cast<uint8_t*>(state)); - context->SetFunctionState(scope, NULL); + context->SetFunctionState(scope, nullptr); } } @@ -356,9 +356,11 @@ void MemTestClose(FunctionContext* context, FunctionContext::FunctionStateScope if (scope == FunctionContext::THREAD_LOCAL) { int64_t* total = reinterpret_cast<int64_t*>( context->GetFunctionState(FunctionContext::THREAD_LOCAL)); + // Initialization could have failed. Prepare() may not have been called. + if (total == nullptr) return; context->Free(*total); context->Free(reinterpret_cast<uint8_t*>(total)); - context->SetFunctionState(scope, NULL); + context->SetFunctionState(scope, nullptr); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/c26a485a/be/src/udf/udf-test.cc ---------------------------------------------------------------------- diff --git a/be/src/udf/udf-test.cc b/be/src/udf/udf-test.cc index 796bcb3..0bce70f 100644 --- a/be/src/udf/udf-test.cc +++ b/be/src/udf/udf-test.cc @@ -172,7 +172,7 @@ void ValidateSharedStateClose( if (scope == FunctionContext::THREAD_LOCAL) { void* state = context->GetFunctionState(scope); context->Free(reinterpret_cast<uint8_t*>(state)); - context->SetFunctionState(scope, NULL); + context->SetFunctionState(scope, nullptr); } }
