KUDU-1719: Heap use-after-free and deadlock on Messenger::Init() failure We may end up with a use-after-free or a deadlock depending on a destruction order race, on a Messenger::Init() failure during a MessengerBuilder::Build().
The main reason for this is because previously a gscoped_ptr and a shared_ptr pointed to the same object, causing the destructor to be called on the same object twice on destruction. This patch does away with the gscoped_ptr and uses a raw pointer in MessengerBuilder::Build() instead, where we don't explicitly free the raw pointer since it will be freed when 'retain_self_' is reset() anyway. A more detailed explanation is given in the JIRA. Change-Id: If48c8481c4bf2255f40ddd38460c1ba40c1b0faa Reviewed-on: http://gerrit.cloudera.org:8080/4782 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/2426ef30 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/2426ef30 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/2426ef30 Branch: refs/heads/master Commit: 2426ef30650318bafd72cc49984aa7b5719b58e0 Parents: 49d0d7f Author: Sailesh Mukil <[email protected]> Authored: Fri Oct 21 11:55:10 2016 -0700 Committer: Todd Lipcon <[email protected]> Committed: Fri Oct 21 21:22:14 2016 +0000 ---------------------------------------------------------------------- src/kudu/rpc/messenger.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/2426ef30/src/kudu/rpc/messenger.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/messenger.cc b/src/kudu/rpc/messenger.cc index a543f5d..501191d 100644 --- a/src/kudu/rpc/messenger.cc +++ b/src/kudu/rpc/messenger.cc @@ -102,16 +102,17 @@ MessengerBuilder &MessengerBuilder::set_metric_entity( Status MessengerBuilder::Build(shared_ptr<Messenger> *msgr) { RETURN_NOT_OK(SaslInit(kSaslAppName)); // Initialize SASL library before we start making requests - gscoped_ptr<Messenger> new_msgr(new Messenger(*this)); + Messenger* new_msgr(new Messenger(*this)); Status build_status = new_msgr->Init(); if (!build_status.ok()) { + // 'new_msgr' will be freed when 'retain_self_' is reset, so no need to explicitly free it. new_msgr->AllExternalReferencesDropped(); return build_status; } // See docs on Messenger::retain_self_ for info about this odd hack. *msgr = shared_ptr<Messenger>( - new_msgr.release(), std::mem_fun(&Messenger::AllExternalReferencesDropped)); + new_msgr, std::mem_fun(&Messenger::AllExternalReferencesDropped)); return Status::OK(); }
