Back out "Use weak_ptr for holding operation during certificate validation"

Summary:
Original commit changeset: f592b3ba0419

Original Phabricator Diff: D66730727

 ---

The following ran on D66730727 and failed:
https://www.internalfb.com/intern/hhvm/automator/workflows/205817/

The following ran on **parent** of D66730727 and it succeeded:
https://www.internalfb.com/intern/hhvm/automator/workflows/205818/

Reviewed By: jtwarren

Differential Revision: D67992747

fbshipit-source-id: e58876849b730a7c39745f0edd5d536bf8285672

Author: mdko

hphp/runtime/ext/async_mysql/ext_async_mysql.cpp

@@ -166,7 +166,7 @@ static std::shared_ptr<am::AsyncMysqlClient> getClient() {
 static std::vector<std::string> certLoggingImpl(
     X509* cert,
     const std::vector<std::string>& extNames,
-    const am::Operation& op,
+    am::ConnectOperation& op,
     bool validated) {
   // Capture the certificare Common Name
   std::string cn =
@@ -217,13 +217,12 @@ static bool certValidationImpl(
 
 static bool serverCertLoggingCallback(
     X509* server_cert,
-    const std::weak_ptr<am::Operation>& wptr,
+    const void* context,
     folly::StringPiece& /*errMsg*/,
     const std::vector<std::string>& extNames) {
-  auto op = wptr.lock();
-  if (!op) {
-    return false;
-  }
+  am::ConnectOperation* op = reinterpret_cast<am::ConnectOperation*>(
+          const_cast<void*>(context));
+  CHECK(op);
 
   // Log the server cert content
   certLoggingImpl(server_cert, extNames, *op, false);
@@ -233,41 +232,38 @@ static bool serverCertLoggingCallback(
 
 static bool serverCertValidationCallback(
     X509* server_cert,
-    const std::weak_ptr<am::Operation>& wptr,
+    const void* context,
     folly::StringPiece& /* errMsg */,
     const std::vector<std::string>& extNames,
     const std::vector<std::string>& extValues) {
-  auto op = wptr.lock();
-  if (!op) {
-    return false;
-  }
+  facebook::common::mysql_client::ConnectOperation* op =
+      reinterpret_cast<facebook::common::mysql_client::ConnectOperation*>(
+          const_cast<void*>(context));
+  CHECK(op);
 
   // Log the server cert content
   auto certValues = certLoggingImpl(server_cert, extNames, *op, true);
   return certValidationImpl(extValues, certValues);
 }
 
-static am::CertValidatorCallback generateCertValidationCallback(
+static facebook::common::mysql_client::CertValidatorCallback
+generateCertValidationCallback(
     const std::string& serverCertExtNames,
     const std::string& extensionValues) {
   std::vector<std::string> extNames;
   folly::split(',', serverCertExtNames, extNames);
   if (extensionValues.empty()) {
     return [extNames = std::move(extNames)] (
-          X509* server_cert,
-          const std::weak_ptr<am::Operation>& wptr,
-          folly::StringPiece& errMsg) {
-      return serverCertLoggingCallback(server_cert, wptr, errMsg, extNames);
+          X509* server_cert, const void* context, folly::StringPiece& errMsg) {
+      return serverCertLoggingCallback(server_cert, context, errMsg, extNames);
     };
   } else {
     std::vector<std::string> extValues;
     folly::split(',', extensionValues, extValues);
     return [extNames = std::move(extNames), extValues = std::move(extValues)] (
-          X509* server_cert,
-          const std::weak_ptr<am::Operation>& wptr,
-          folly::StringPiece& errMsg) {
+          X509* server_cert, const void* context, folly::StringPiece& errMsg) {
         return serverCertValidationCallback(
-            server_cert, wptr, errMsg, extNames, extValues);
+            server_cert, context, errMsg, extNames, extValues);
     };
   }
 }
@@ -493,7 +489,8 @@ HHVM_METHOD(AsyncMysqlConnectionOptions,
   data->m_conn_opts.setCertValidationCallback(
       generateCertValidationCallback(
           std::string(serverCertExtNames), std::string(extensionValues)),
-      {} /* empty weak_ptr<Operation> */);
+      nullptr,
+      true);
   #endif
 }
 
@@ -682,7 +679,8 @@ Object HHVM_STATIC_METHOD(
     op->setCertValidationCallback(
         generateCertValidationCallback(
             std::string(serverCertExtNames), std::string(serverCertExtValues)),
-        {} /* empty weak_ptr<Operation> */);
+        nullptr,
+        true);
   }
 
   return newAsyncMysqlConnectEvent(std::move(op), getClient());
@@ -882,7 +880,8 @@ static Object HHVM_METHOD(
     op->setCertValidationCallback(
         generateCertValidationCallback(
             std::string(serverCertExtNames), std::string(serverCertExtValues)),
-        {} /* empty weak_ptr<Operation> */);
+        nullptr,
+        true);
   }
 
   return newAsyncMysqlConnectEvent(

third-party/squangle/src/squangle/mysql_client/ConnectOperation.cpp

@@ -47,7 +47,8 @@ void ConnectOperationImpl::setConnectionOptions(
   if (conn_opts.getCertValidationCallback()) {
     setCertValidationCallback(
         conn_opts.getCertValidationCallback(),
-        conn_opts.getCertValidationContext());
+        conn_opts.getCertValidationContext(),
+        conn_opts.isOpPtrAsValidationContext());
   }
   if (auto cats = conn_opts.getCryptoAuthTokenList()) {
     setCryptoAuthTokenList(std::move(*cats));
@@ -84,10 +85,12 @@ void ConnectOperationImpl::enableChangeUser() {
 
 void ConnectOperationImpl::setCertValidationCallback(
     CertValidatorCallback callback,
-    std::weak_ptr<Operation> wptr) {
+    const void* context,
+    bool opPtrAsContext) {
   CHECK_THROW(
       state() == OperationState::Unstarted, db::OperationStateException);
-  conn_options_.setCertValidationCallback(std::move(callback), std::move(wptr));
+  conn_options_.setCertValidationCallback(
+      std::move(callback), context, opPtrAsContext);
 }
 
 void ConnectOperationImpl::setTimeout(Duration timeout) {

third-party/squangle/src/squangle/mysql_client/ConnectOperation.h

@@ -65,7 +65,8 @@ class ConnectOperationImpl : virtual public OperationBase {
 
   void setCertValidationCallback(
       CertValidatorCallback callback,
-      std::weak_ptr<Operation> wptr);
+      const void* context = nullptr,
+      bool opPtrAsContext = false);
 
   const CertValidatorCallback& getCertValidationCallback() const {
     return conn_options_.getCertValidationCallback();
@@ -272,8 +273,10 @@ class ConnectOperation : public Operation {
   }
   ConnectOperation& setCertValidationCallback(
       CertValidatorCallback callback,
-      std::weak_ptr<Operation> wptr) {
-    impl()->setCertValidationCallback(std::move(callback), std::move(wptr));
+      const void* context = nullptr,
+      bool opPtrAsContext = false) {
+    impl()->setCertValidationCallback(
+        std::move(callback), context, opPtrAsContext);
     return *this;
   }
 
@@ -295,7 +298,7 @@ class ConnectOperation : public Operation {
       const std::string& sslCertCn,
       const std::vector<std::string>& sslCertSan,
       const std::vector<std::string>& sslCertIdentities,
-      bool isValidated) const override {
+      bool isValidated) {
     // Make sure we have an impl - it is possible to not have one.
     if (auto* implPtr = impl()) {
       implPtr->withOptionalConnectionContext([&](auto& ctx) {

third-party/squangle/src/squangle/mysql_client/ConnectionOptions.cpp

@@ -16,8 +16,6 @@
 
 namespace facebook::common::mysql_client {
 
-class Operation;
-
 ConnectionOptions::ConnectionOptions()
     : connection_timeout_(FLAGS_async_mysql_connect_timeout_micros),
       total_timeout_(FLAGS_async_mysql_timeout_micros * 2),
@@ -79,9 +77,11 @@ ConnectionOptions& ConnectionOptions::setDscp(uint8_t dscp) {
 
 ConnectionOptions& ConnectionOptions::setCertValidationCallback(
     CertValidatorCallback callback,
-    std::weak_ptr<Operation> wptr) noexcept {
+    const void* context,
+    bool opPtrAsContext) noexcept {
   certValidationCallback_ = std::move(callback);
-  certValidationWeakPtr_ = std::move(wptr);
+  opPtrAsCertValidationContext_ = opPtrAsContext;
+  certValidationContext_ = opPtrAsCertValidationContext_ ? nullptr : context;
   return *this;
 }
 

third-party/squangle/src/squangle/mysql_client/ConnectionOptions.h

@@ -15,13 +15,10 @@
 
 namespace facebook::common::mysql_client {
 
-class Operation;
 class SSLOptionsProviderBase;
 
-using CertValidatorCallback = std::function<bool(
-    X509* server_cert,
-    const std::weak_ptr<Operation>& context,
-    folly::StringPiece& errMsg)>;
+using CertValidatorCallback = std::function<
+    bool(X509* server_cert, const void* context, folly::StringPiece& errMsg)>;
 
 class ConnectionOptions {
  public:
@@ -195,14 +192,19 @@ class ConnectionOptions {
 
   ConnectionOptions& setCertValidationCallback(
       CertValidatorCallback callback,
-      std::weak_ptr<Operation> wptr) noexcept;
+      const void* context,
+      bool opPtrAsContext) noexcept;
 
   const CertValidatorCallback& getCertValidationCallback() const noexcept {
     return certValidationCallback_;
   }
 
-  const std::weak_ptr<Operation>& getCertValidationContext() const noexcept {
-    return certValidationWeakPtr_;
+  const void* getCertValidationContext() const noexcept {
+    return certValidationContext_;
+  }
+
+  bool isOpPtrAsValidationContext() const noexcept {
+    return opPtrAsCertValidationContext_;
   }
 
   ConnectionOptions& setCryptoAuthTokenList(const std::string& tokenList) {
@@ -231,7 +233,8 @@ class ConnectionOptions {
   bool delayed_reset_conn_ = false;
   bool change_user_ = false;
   CertValidatorCallback certValidationCallback_{nullptr};
-  std::weak_ptr<Operation> certValidationWeakPtr_;
+  const void* certValidationContext_{nullptr};
+  bool opPtrAsCertValidationContext_{false};
 };
 
 } // namespace facebook::common::mysql_client

third-party/squangle/src/squangle/mysql_client/Operation.h

@@ -235,7 +235,7 @@ class OperationBase {
     return connection_context_.get();
   }
   void withOptionalConnectionContext(
-      std::function<void(db::ConnectionContextBase&)> func) const {
+      std::function<void(db::ConnectionContextBase&)> func) {
     if (connection_context_) {
       func(*connection_context_);
     }
@@ -623,12 +623,6 @@ class Operation : public std::enable_shared_from_this<Operation> {
     return impl()->endTime();
   }
 
-  virtual void reportServerCertContent(
-      const std::string& /*sslCertCn*/,
-      const std::vector<std::string>& /*sslCertSan*/,
-      const std::vector<std::string>& /*sslCertIdentities*/,
-      bool /*isValidated*/) const {}
-
   static folly::StringPiece toString(OperationState state);
   static folly::StringPiece toString(OperationResult result);
   static folly::StringPiece toString(StreamState state);

third-party/squangle/src/squangle/mysql_client/mysql_protocol/MysqlConnectOperationImpl.cpp

@@ -373,13 +373,15 @@ int MysqlConnectOperationImpl::mysqlCertValidator(
   const CertValidatorCallback callback =
       self->getConnectionOptions().getCertValidationCallback();
   CHECK(callback);
-  const auto& wptrOperation =
-      self->getConnectionOptions().getCertValidationContext();
+  const void* callbackContext =
+      self->getConnectionOptions().isOpPtrAsValidationContext()
+      ? self
+      : self->getConnectionOptions().getCertValidationContext();
   folly::StringPiece errorMessage;
 
   // "libmysql" expects this callback to return "0" if the cert validation was
   // successful, and return "1" if validation failed.
-  int result = callback(server_cert, wptrOperation, errorMessage) ? 0 : 1;
+  int result = callback(server_cert, callbackContext, errorMessage) ? 0 : 1;
   if (!errorMessage.empty()) {
     *errptr = errorMessage.data();
   }