[2/3] kudu git commit: KUDU-1719: Heap use-after-free and deadlock on Messenger::Init() failure

[ad...@apache.org]

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 <to...@apache.org>


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 <sa...@apache.com>
Authored: Fri Oct 21 11:55:10 2016 -0700
Committer: Todd Lipcon <to...@apache.org>
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();
 }