Repository: incubator-impala Updated Branches: refs/heads/master 2a314e780 -> 406632640
IMPALA-4801: fix heap use after free for MemTracker This is a minimal fix for IMPALA-4801. The bug was that "IMPALA-4678: move query MemTracker into QueryState" changed the lifecycle of RuntimeState::instance_mem_tracker_ so that it destroyed earlier in ReleaseResources(). A periodic counter referencing the MemTracker was only stopped after ReleaseResources() was called, leaving a window for a race. There are various issues with the lifecycle of RuntimeProfile objects (see IMPALA-4802) but I chose not to tackle them in this patch. Change-Id: Iccaf6700888c333b9c93d716d395d96bc5af6631 Reviewed-on: http://gerrit.cloudera.org:8080/5772 Reviewed-by: Tim Armstrong <[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/40663264 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/40663264 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/40663264 Branch: refs/heads/master Commit: 406632640d44c9eb45286d4ed73192b6c16daf2f Parents: 2a314e7 Author: Tim Armstrong <[email protected]> Authored: Mon Jan 23 13:43:35 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Jan 26 01:32:09 2017 +0000 ---------------------------------------------------------------------- be/src/runtime/plan-fragment-executor.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/40663264/be/src/runtime/plan-fragment-executor.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/plan-fragment-executor.cc b/be/src/runtime/plan-fragment-executor.cc index dbadf6d..bafba5d 100644 --- a/be/src/runtime/plan-fragment-executor.cc +++ b/be/src/runtime/plan-fragment-executor.cc @@ -499,13 +499,12 @@ void PlanFragmentExecutor::Close() { // Prepare should always have been called, and so runtime_state_ should be set DCHECK(prepared_promise_.IsSet()); if (exec_tree_ != NULL) exec_tree_->Close(runtime_state_.get()); - runtime_state_->ReleaseResources(); - if (mem_usage_sampled_counter_ != NULL) { + // This counter references runtime_state_->instance_mem_tracker() so must be + // stopped before calling ReleaseResources(). PeriodicCounterUpdater::StopTimeSeriesCounter(mem_usage_sampled_counter_); mem_usage_sampled_counter_ = NULL; } - closed_ = true; // Sanity timer checks #ifndef NDEBUG int64_t total_time = profile()->total_time_counter()->value(); @@ -516,6 +515,9 @@ void PlanFragmentExecutor::Close() { } DCHECK_LE(other_time, total_time); #endif + runtime_state_->ReleaseResources(); + + closed_ = true; } }
