mesos git commit: Fixed a potential deadlock in the 'CombinedAuthenticator'.

[vi...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master 2f90b70b5 -> cd927874e


Fixed a potential deadlock in the 'CombinedAuthenticator'.

This patch fixes a bug in `CombinedAuthenticator.authenticate()`.
Previously, the function registered a callback on the results of
`Authenticator::authenticate()` calls, which would be executed in
the context of the individual authenticators' processes. Since this
callback captured a copy of the `Owned<Authenticator>` referring to
that authenticator itself, it was possible that during teardown the
authenticator could be destroyed from its own context, leading to
a deadlock.

This patch eliminates the capture of a reference to the
authenticator, instead simply capturing the authenticator's scheme.

Review: https://reviews.apache.org/r/58464/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/cd927874
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/cd927874
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/cd927874

Branch: refs/heads/master
Commit: cd927874eec22fdb67b5e5b2b7bda687ab82cd42
Parents: 2f90b70
Author: Greg Mann <gr...@mesosphere.io>
Authored: Tue Apr 18 15:42:18 2017 -0700
Committer: Vinod Kone <vi...@gmail.com>
Committed: Tue Apr 18 15:42:18 2017 -0700

----------------------------------------------------------------------
 .../http/combined_authenticator.cpp             | 25 ++++++++++++--------
 1 file changed, 15 insertions(+), 10 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/cd927874/src/authentication/http/combined_authenticator.cpp
----------------------------------------------------------------------
diff --git a/src/authentication/http/combined_authenticator.cpp b/src/authentication/http/combined_authenticator.cpp
index 51ec882..836afc5 100644
--- a/src/authentication/http/combined_authenticator.cpp
+++ b/src/authentication/http/combined_authenticator.cpp
@@ -288,10 +288,20 @@ Future<AuthenticationResult> CombinedAuthenticatorProcess::authenticate(
           return combineFailed(results);
         }
 
+        // Instead of capturing `authenticator` we capture the scheme by copy,
+        // since that's all we need. This avoids an issue during teardown of the
+        // authenticator: if the `CombinedAuthenticator` is destroyed before the
+        // callback below executes, the `authenticator` reference here could be
+        // the last remaining reference to that authenticator. Capturing this
+        // reference could cause the authenticator's destructor to be called
+        // from within its own context when the callback completes, leading to a
+        // deadlock. See MESOS-7065.
+        const string scheme = authenticator.get()->scheme();
+
         return authenticator.get()->authenticate(request)
           .then(defer(
               self_,
-              [&results, authenticator](const AuthenticationResult& result)
+              [&results, scheme](const AuthenticationResult& result)
                   -> ControlFlow<AuthenticationResult> {
                 // Validate that exactly 1 member is set.
                 size_t count =
@@ -300,8 +310,7 @@ Future<AuthenticationResult> CombinedAuthenticatorProcess::authenticate(
                   (result.forbidden.isSome()    ? 1 : 0);
 
                 if (count != 1) {
-                  LOG(WARNING) << "HTTP authenticator for scheme '"
-                               << authenticator.get()->scheme()
+                  LOG(WARNING) << "HTTP authenticator for scheme '" << scheme
                                << "' returned a result with " << count
                                << " members set, which is an error";
                   return Continue();
@@ -313,17 +322,13 @@ Future<AuthenticationResult> CombinedAuthenticatorProcess::authenticate(
                 }
 
                 // Authentication unsuccessful; append the result and continue.
-                results.push_back(make_pair(
-                    authenticator.get()->scheme(),
-                    result));
+                results.push_back(make_pair(scheme, result));
                 return Continue();
               }))
-          .repair([&results, authenticator](
+          .repair([&results, scheme](
               const Future<ControlFlow<AuthenticationResult>>& failedResult)
                   -> ControlFlow<AuthenticationResult> {
-            results.push_back(make_pair(
-                authenticator.get()->scheme(),
-                Error(failedResult.failure())));
+            results.push_back(make_pair(scheme, Error(failedResult.failure())));
             return Continue();
           });
       });