[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Reviewed-by: Joe McDonnell Reviewed-by: Bikramjeet Vig Tested-by: Impala Public Jenkins --- 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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 20:25:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 17:08:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 16:50:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 16:00:17 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 15:16:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 01:42:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 01:34:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 01:07:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 19 Apr 2019 00:24:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 18 Apr 2019 23:56:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8270: fix MemTracker teardown in FeSupport
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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 17 Apr 2019 23:50:48 + Gerrit-HasComments: No
[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 Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall