You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/04/17 23:06:57 UTC
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13057
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
IMPALA-8270: fix MemTracker teardown in FeSupport
This patch tries to simplify and standardise the order
in which control structures are torn down. As a
consequence the bug is fixed. I've described the bug
below.
The changes are:
* Make more control structures owned directly by
QueryState::obj_pool_, so that they are all
destroyed at the same time via ~QueryState.
* Tear down local_query_state_ explicitly before
other destructors run.
Either change is sufficient to fix the bug, but
I preferred to do both to reduce the chances
of similar bugs in future.
Description of bug:
===================
In the normal query execution flow:
- RuntimeState is in QueryState::obj_pool_
- RuntimeState owns RuntimeState::instance_mem_tracker_ via unique_ptr
- QueryState::query_mem_tracker_ is in QueryState::obj_pool_
- QueryState::query_mem_tracker_ has a reference to
RuntimeState::instance_mem_tracker_
The tear-down works because ~QueryState unregisters query_mem_tracker_
from its parent, making the whole subtree unreachable before destroying
QueryState::obj_pool_. It is thus safe to destroy instance_mem_tracker_
along with the rest of obj_pool_.
FeSupport messes this up by having RuntimeState own the QueryState
RuntimeState::local_query_state_ via a unique_ptr, and the implied
destructor order means that RuntimeState::instance_mem_tracker_ is
destroyed before RuntimeState::local_query_state_, which breaks the
above flow and the destroyed local_query_state_ is reachable from
the process MemTracker via QueryState::query_mem_tracker_ for a
small window until it is unregistered.
Testing:
Added a backend test that reproduced the ASAN use-after-free
failure when run against unmodified RuntimeState code. I did
not make it a unified backend test so that it would be easier
to backport this fix to older versions that don't have unified
tests.
Change-Id: If815130cd4db00917746f10b28514f779ee254f0
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-state-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
4 files changed, 99 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/13057/1
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2843/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 16:00:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Carry
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state.h@377
PS1, Line 377: this
> nit: this is
Done
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 17:08:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 20:25:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
IMPALA-8270: fix MemTracker teardown in FeSupport
This patch tries to simplify and standardise the order
in which control structures are torn down. As a
consequence the bug is fixed. I've described the bug
below.
The changes are:
* Make more control structures owned directly by
QueryState::obj_pool_, so that they are all
destroyed at the same time via ~QueryState.
* Tear down local_query_state_ explicitly before
other destructors run.
Either change is sufficient to fix the bug, but
I preferred to do both to reduce the chances
of similar bugs in future.
Description of bug:
===================
In the normal query execution flow:
- RuntimeState is in QueryState::obj_pool_
- RuntimeState owns RuntimeState::instance_mem_tracker_ via scoped_ptr
- QueryState::query_mem_tracker_ is in QueryState::obj_pool_
- QueryState::query_mem_tracker_ has a reference to
RuntimeState::instance_mem_tracker_
The tear-down works because ~QueryState unregisters query_mem_tracker_
from its parent, making the whole subtree unreachable before destroying
QueryState::obj_pool_. It is thus safe to destroy instance_mem_tracker_
along with the rest of obj_pool_.
FeSupport messes this up by having RuntimeState own the QueryState
RuntimeState::local_query_state_ via a scoped_ptr, and the implied
destructor order means that RuntimeState::instance_mem_tracker_ is
destroyed before RuntimeState::local_query_state_, which breaks the
above flow and the destroyed instance_mem_tracker_ is reachable from
the process MemTracker via QueryState::query_mem_tracker_ for a
small window until it is unregistered.
Testing:
Added a backend test that reproduced the ASAN use-after-free
failure when run against unmodified RuntimeState code. I did
not make it a unified backend test so that it would be easier
to backport this fix to older versions that don't have unified
tests.
Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Reviewed-on: http://gerrit.cloudera.org:8080/13057
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-state-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
4 files changed, 92 insertions(+), 12 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, but someone else must approve
Joe McDonnell: Looks good to me, but someone else must approve
Bikramjeet Vig: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13057
to look at the new patch set (#4).
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
IMPALA-8270: fix MemTracker teardown in FeSupport
This patch tries to simplify and standardise the order
in which control structures are torn down. As a
consequence the bug is fixed. I've described the bug
below.
The changes are:
* Make more control structures owned directly by
QueryState::obj_pool_, so that they are all
destroyed at the same time via ~QueryState.
* Tear down local_query_state_ explicitly before
other destructors run.
Either change is sufficient to fix the bug, but
I preferred to do both to reduce the chances
of similar bugs in future.
Description of bug:
===================
In the normal query execution flow:
- RuntimeState is in QueryState::obj_pool_
- RuntimeState owns RuntimeState::instance_mem_tracker_ via scoped_ptr
- QueryState::query_mem_tracker_ is in QueryState::obj_pool_
- QueryState::query_mem_tracker_ has a reference to
RuntimeState::instance_mem_tracker_
The tear-down works because ~QueryState unregisters query_mem_tracker_
from its parent, making the whole subtree unreachable before destroying
QueryState::obj_pool_. It is thus safe to destroy instance_mem_tracker_
along with the rest of obj_pool_.
FeSupport messes this up by having RuntimeState own the QueryState
RuntimeState::local_query_state_ via a scoped_ptr, and the implied
destructor order means that RuntimeState::instance_mem_tracker_ is
destroyed before RuntimeState::local_query_state_, which breaks the
above flow and the destroyed instance_mem_tracker_ is reachable from
the process MemTracker via QueryState::query_mem_tracker_ for a
small window until it is unregistered.
Testing:
Added a backend test that reproduced the ASAN use-after-free
failure when run against unmodified RuntimeState code. I did
not make it a unified backend test so that it would be easier
to backport this fix to older versions that don't have unified
tests.
Change-Id: If815130cd4db00917746f10b28514f779ee254f0
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-state-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
4 files changed, 92 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/13057/4
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2830/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 23:50:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state.h@377
PS1, Line 377: this
nit: this is
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 01:42:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13057/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13057/1//COMMIT_MSG@42
PS1, Line 42: local_query_state_
Should this be instance_mem_tracker_?
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state-test.cc
File be/src/runtime/runtime-state-test.cc:
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state-test.cc@43
PS1, Line 43: TEST
I think this should be TEST_F? (Or use TEST and remove the RuntimeStateTest class)
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Apr 2019 23:56:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13057
to look at the new patch set (#2).
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
IMPALA-8270: fix MemTracker teardown in FeSupport
This patch tries to simplify and standardise the order
in which control structures are torn down. As a
consequence the bug is fixed. I've described the bug
below.
The changes are:
* Make more control structures owned directly by
QueryState::obj_pool_, so that they are all
destroyed at the same time via ~QueryState.
* Tear down local_query_state_ explicitly before
other destructors run.
Either change is sufficient to fix the bug, but
I preferred to do both to reduce the chances
of similar bugs in future.
Description of bug:
===================
In the normal query execution flow:
- RuntimeState is in QueryState::obj_pool_
- RuntimeState owns RuntimeState::instance_mem_tracker_ via scoped_ptr
- QueryState::query_mem_tracker_ is in QueryState::obj_pool_
- QueryState::query_mem_tracker_ has a reference to
RuntimeState::instance_mem_tracker_
The tear-down works because ~QueryState unregisters query_mem_tracker_
from its parent, making the whole subtree unreachable before destroying
QueryState::obj_pool_. It is thus safe to destroy instance_mem_tracker_
along with the rest of obj_pool_.
FeSupport messes this up by having RuntimeState own the QueryState
RuntimeState::local_query_state_ via a scoped_ptr, and the implied
destructor order means that RuntimeState::instance_mem_tracker_ is
destroyed before RuntimeState::local_query_state_, which breaks the
above flow and the destroyed instance_mem_tracker_ is reachable from
the process MemTracker via QueryState::query_mem_tracker_ for a
small window until it is unregistered.
Testing:
Added a backend test that reproduced the ASAN use-after-free
failure when run against unmodified RuntimeState code. I did
not make it a unified backend test so that it would be easier
to backport this fix to older versions that don't have unified
tests.
Change-Id: If815130cd4db00917746f10b28514f779ee254f0
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-state-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
4 files changed, 92 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/13057/2
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4046/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2840/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 01:34:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Thomas Marshall, Joe McDonnell, Bikramjeet Vig, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13057
to look at the new patch set (#3).
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
IMPALA-8270: fix MemTracker teardown in FeSupport
This patch tries to simplify and standardise the order
in which control structures are torn down. As a
consequence the bug is fixed. I've described the bug
below.
The changes are:
* Make more control structures owned directly by
QueryState::obj_pool_, so that they are all
destroyed at the same time via ~QueryState.
* Tear down local_query_state_ explicitly before
other destructors run.
Either change is sufficient to fix the bug, but
I preferred to do both to reduce the chances
of similar bugs in future.
Description of bug:
===================
In the normal query execution flow:
- RuntimeState is in QueryState::obj_pool_
- RuntimeState owns RuntimeState::instance_mem_tracker_ via scoped_ptr
- QueryState::query_mem_tracker_ is in QueryState::obj_pool_
- QueryState::query_mem_tracker_ has a reference to
RuntimeState::instance_mem_tracker_
The tear-down works because ~QueryState unregisters query_mem_tracker_
from its parent, making the whole subtree unreachable before destroying
QueryState::obj_pool_. It is thus safe to destroy instance_mem_tracker_
along with the rest of obj_pool_.
FeSupport messes this up by having RuntimeState own the QueryState
RuntimeState::local_query_state_ via a scoped_ptr, and the implied
destructor order means that RuntimeState::instance_mem_tracker_ is
destroyed before RuntimeState::local_query_state_, which breaks the
above flow and the destroyed instance_mem_tracker_ is reachable from
the process MemTracker via QueryState::query_mem_tracker_ for a
small window until it is unregistered.
Testing:
Added a backend test that reproduced the ASAN use-after-free
failure when run against unmodified RuntimeState code. I did
not make it a unified backend test so that it would be easier
to backport this fix to older versions that don't have unified
tests.
Change-Id: If815130cd4db00917746f10b28514f779ee254f0
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/runtime-state-test.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
4 files changed, 92 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/13057/3
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13057/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13057/1//COMMIT_MSG@42
PS1, Line 42: local_query_state_
> Should this be instance_mem_tracker_?
Done
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state-test.cc
File be/src/runtime/runtime-state-test.cc:
http://gerrit.cloudera.org:8080/#/c/13057/1/be/src/runtime/runtime-state-test.cc@43
PS1, Line 43: TEST
> I think this should be TEST_F? (Or use TEST and remove the RuntimeStateTest
Done
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 00:24:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 16:50:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13057 )
Change subject: IMPALA-8270: fix MemTracker teardown in FeSupport
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/2839/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13057
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If815130cd4db00917746f10b28514f779ee254f0
Gerrit-Change-Number: 13057
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Apr 2019 01:07:04 +0000
Gerrit-HasComments: No