Repository: incubator-impala Updated Branches: refs/heads/master 0cb76b6e5 -> 09aa76186
IMPALA-4707: fix use-after-free in QueryExecMgr The bug is that the QueryState was referenced after the refcount is decremented. The fix is to not do that. Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535 Reviewed-on: http://gerrit.cloudera.org:8080/5615 Reviewed-by: Dan Hecht <[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/09aa7618 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/09aa7618 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/09aa7618 Branch: refs/heads/master Commit: 09aa7618604e2f184d29c1615d99d6d305e7e5e2 Parents: 0cb76b6 Author: Tim Armstrong <[email protected]> Authored: Thu Jan 5 12:09:07 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Wed Jan 11 00:43:16 2017 +0000 ---------------------------------------------------------------------- be/src/runtime/query-exec-mgr.cc | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/09aa7618/be/src/runtime/query-exec-mgr.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/query-exec-mgr.cc b/be/src/runtime/query-exec-mgr.cc index 4a3742c..2fd2f8f 100644 --- a/be/src/runtime/query-exec-mgr.cc +++ b/be/src/runtime/query-exec-mgr.cc @@ -132,34 +132,32 @@ QueryState* QueryExecMgr::GetQueryState(const TUniqueId& query_id) { void QueryExecMgr::ReleaseQueryState(QueryState* qs) { DCHECK(qs != nullptr); - TUniqueId query_id; - { - int32_t cnt = qs->refcnt_.Add(-1); - VLOG_QUERY << "ReleaseQueryState(): query_id=" << PrintId(qs->query_id()) - << " refcnt=" << cnt + 1; - DCHECK_GE(cnt, 0); - if (cnt > 0) return; - // don't reference anything from 'qs' beyond this point, 'qs' might get - // gc'd out from under us - query_id = qs->query_id(); - } - + TUniqueId query_id = qs->query_id(); + int32_t cnt = qs->refcnt_.Add(-1); + // don't reference anything from 'qs' beyond this point, 'qs' might get + // gc'd out from under us + qs = nullptr; + VLOG_QUERY << "ReleaseQueryState(): query_id=" << PrintId(query_id) + << " refcnt=" << cnt + 1; + DCHECK_GE(cnt, 0); + if (cnt > 0) return; + + QueryState* qs_from_map = nullptr; { // for now, gc right away lock_guard<mutex> l(qs_map_lock_); auto it = qs_map_.find(query_id); // someone else might have gc'd the entry if (it == qs_map_.end()) return; - qs = it->second; - DCHECK_EQ(qs->query_ctx().query_id, query_id); - int32_t cnt = qs->refcnt_.Load(); + qs_from_map = it->second; + DCHECK_EQ(qs_from_map->query_ctx().query_id, query_id); + int32_t cnt = qs_from_map->refcnt_.Load(); DCHECK_GE(cnt, 0); // someone else might have increased the refcnt in the meantime if (cnt > 0) return; - size_t num_erased = qs_map_.erase(qs->query_ctx().query_id); - DCHECK_EQ(num_erased, 1); + qs_map_.erase(it); } // TODO: send final status report during gc, but do this from a different thread - delete qs; + delete qs_from_map; }
