Repository: incubator-impala Updated Branches: refs/heads/master fe8d994f0 -> e2cde13a2
IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync() Once the Promise (of Status) is set in ProcessBuildInputAsync(), the main thread in ProcessBuildInputAndOpenProbe() may proceed to finish up the query and free RuntimeState. So, it's unsafe to access the RuntimeState afterwards. Commit bb1c633 broke that assumption (which is arguably fragile). This change fixes the problem by adding a scope for the that counter to avoid the use-after-free problem. Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4 Reviewed-on: http://gerrit.cloudera.org:8080/5246 Reviewed-by: Dan Hecht <[email protected]> Reviewed-by: Tim Armstrong <[email protected]> Tested-by: Internal 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/f2ce30e6 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/f2ce30e6 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/f2ce30e6 Branch: refs/heads/master Commit: f2ce30e6448fb1a5c1d5721c0de1c768261770b3 Parents: fe8d994 Author: Michael Ho <[email protected]> Authored: Mon Nov 28 12:16:07 2016 -0800 Committer: Internal Jenkins <[email protected]> Committed: Tue Nov 29 01:45:13 2016 +0000 ---------------------------------------------------------------------- be/src/exec/blocking-join-node.cc | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f2ce30e6/be/src/exec/blocking-join-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/blocking-join-node.cc b/be/src/exec/blocking-join-node.cc index 0e23727..fca2f72 100644 --- a/be/src/exec/blocking-join-node.cc +++ b/be/src/exec/blocking-join-node.cc @@ -145,25 +145,29 @@ void BlockingJoinNode::Close(RuntimeState* state) { void BlockingJoinNode::ProcessBuildInputAsync(RuntimeState* state, DataSink* build_sink, Promise<Status>* status) { - SCOPED_THREAD_COUNTER_MEASUREMENT(state->total_thread_statistics()); Status s; { + SCOPED_THREAD_COUNTER_MEASUREMENT(state->total_thread_statistics()); if (build_sink == NULL){ s = ProcessBuildInput(state); } else { s = SendBuildInputToSink<true>(state, build_sink); } + // IMPALA-1863: If the build-side thread failed, then we need to close the right + // (build-side) child to avoid a potential deadlock between fragment instances. This + // is safe to do because while the build may have partially completed, it will not be + // probed. BlockJoinNode::Open() will return failure as soon as child(0)->Open() + // completes. + if (!s.ok()) child(1)->Close(state); + // Release the thread token as soon as possible (before the main thread joins + // on it). This way, if we had a chain of 10 joins using 1 additional thread, + // we'd keep the additional thread busy the whole time. + state->resource_pool()->ReleaseThreadToken(false); } - // IMPALA-1863: If the build-side thread failed, then we need to close the right - // (build-side) child to avoid a potential deadlock between fragment instances. This - // is safe to do because while the build may have partially completed, it will not be - // probed. BlockJoinNode::Open() will return failure as soon as child(0)->Open() - // completes. - if (!s.ok()) child(1)->Close(state); - // Release the thread token as soon as possible (before the main thread joins - // on it). This way, if we had a chain of 10 joins using 1 additional thread, - // we'd keep the additional thread busy the whole time. - state->resource_pool()->ReleaseThreadToken(false); + // Please keep this as the last line in this function to avoid use-after-free problem. + // Once 'status' is set, ProcessBuildInputAndProbe() will start running and 'states' + // may have been freed after this line once the query completes. IMPALA-4532. + // TODO: Make this less fragile. status->Set(s); }
