[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #816: MINIFICPP-1261 - Refactor non-trivial usages of ScopeGuard class

[GitBox <gi...@apache.org>]

szaszm commented on a change in pull request #816:
URL: https://github.com/apache/nifi-minifi-cpp/pull/816#discussion_r446315793



##########
File path: libminifi/include/io/tls/TLSSocket.h
##########
@@ -80,10 +77,13 @@ class TLSContext : public SocketContext {
   int16_t initialize(bool server_method = false);
 
  private:
+  static void deleteContext(SSL_CTX* ptr) { SSL_CTX_free(ptr); }
+
   std::shared_ptr<logging::Logger> logger_;
   std::shared_ptr<Configure> configure_;
   std::shared_ptr<minifi::controllers::SSLContextService> ssl_service_;
-  SSL_CTX *ctx;
+  using Context = std::unique_ptr<SSL_CTX, decltype(&deleteContext)>;
+  Context ctx;

Review comment:
       I would localize the context type alias to the implementation, so that pointer semantics are obvious when looking at the declaration. This is probably very subjective because no guidelines come to my mind to support this.
   ```suggestion
     std::unique_ptr<SSL_CTX, decltype(&deleteContext)> ctx;
   ```

##########
File path: libminifi/src/io/tls/TLSSocket.cpp
##########
@@ -72,30 +72,25 @@ int16_t TLSContext::initialize(bool server_method) {
   }
   const SSL_METHOD *method;
   method = server_method ? TLSv1_2_server_method() : TLSv1_2_client_method();
-  ctx = SSL_CTX_new(method);
-  if (ctx == nullptr) {
+  Context local_context = Context(SSL_CTX_new(method), deleteContext);
+  if (local_context == nullptr) {
     logger_->log_error("Could not create SSL context, error: %s.", std::strerror(errno));
     error_value = TLS_ERROR_CONTEXT;
     return error_value;
   }
 
-  utils::ScopeGuard ctxGuard([this]() {
-    SSL_CTX_free(ctx);
-    ctx = nullptr;
-  });
-
   if (needClientCert) {
     std::string certificate;
     std::string privatekey;
     std::string passphrase;
     std::string caCertificate;
 
     if (ssl_service_ != nullptr) {
-      if (!ssl_service_->configure_ssl_context(ctx)) {
+      if (!ssl_service_->configure_ssl_context(local_context.get())) {
         error_value = TLS_ERROR_CERT_ERROR;
         return error_value;
       }
-      ctxGuard.disable();
+      ctx.swap(local_context);

Review comment:
       I think a move assignment would be more clear and would expose potential lifetime issues earlier. (Unless it's implemented in terms of `swap`.)
   ```suggestion
         ctx = std::move(local_context);
   ```

##########
File path: libminifi/include/utils/ScopeGuard.h
##########
@@ -33,10 +33,6 @@ namespace utils {
 
 struct ScopeGuard : ::gsl::final_action<std::function<void()>> {
   using ::gsl::final_action<std::function<void()>>::final_action;
-
-  void disable() noexcept {
-    dismiss();
-  }

Review comment:
       I wouldn't remove this just for the sake of removing it, because external code might depend on it, and it's not a huge maintenance burden to keep it.

##########
File path: libminifi/src/io/tls/TLSSocket.cpp
##########
@@ -120,35 +115,35 @@ int16_t TLSContext::initialize(bool server_method) {
         file.close();
         passphrase = password;
       }
-      SSL_CTX_set_default_passwd_cb(ctx, io::tls::pemPassWordCb);
-      SSL_CTX_set_default_passwd_cb_userdata(ctx, &passphrase);
+      SSL_CTX_set_default_passwd_cb(local_context.get(), io::tls::pemPassWordCb);
+      SSL_CTX_set_default_passwd_cb_userdata(local_context.get(), &passphrase);
     }
 
-    int retp = SSL_CTX_use_PrivateKey_file(ctx, privatekey.c_str(), SSL_FILETYPE_PEM);
+    int retp = SSL_CTX_use_PrivateKey_file(local_context.get(), privatekey.c_str(), SSL_FILETYPE_PEM);
     if (retp != 1) {
       logger_->log_error("Could not create load private key,%i on %s error : %s", retp, privatekey, std::strerror(errno));
       error_value = TLS_ERROR_KEY_ERROR;
       return error_value;
     }
     // verify private key
-    if (!SSL_CTX_check_private_key(ctx)) {
+    if (!SSL_CTX_check_private_key(local_context.get())) {
       logger_->log_error("Private key does not match the public certificate, error : %s", std::strerror(errno));
       error_value = TLS_ERROR_KEY_ERROR;
       return error_value;
     }
     // load CA certificates
     if (ssl_service_ != nullptr || configure_->get(Configure::nifi_security_client_ca_certificate, caCertificate)) {
-      retp = SSL_CTX_load_verify_locations(ctx, caCertificate.c_str(), 0);
+      retp = SSL_CTX_load_verify_locations(local_context.get(), caCertificate.c_str(), 0);
       if (retp == 0) {
         logger_->log_error("Can not load CA certificate, Exiting, error : %s", std::strerror(errno));
         error_value = TLS_ERROR_CERT_ERROR;
         return error_value;
       }
     }
 
-    logger_->log_debug("Load/Verify Client Certificate OK. for %X and %X", this, ctx);
+    logger_->log_debug("Load/Verify Client Certificate OK. for %X and %X", this, local_context.get());
   }
-  ctxGuard.disable();
+  ctx.swap(local_context);

Review comment:
       Same as above, move assignment feels like it would be a more accurate description of intent.
   ```suggestion
     ctx = std::move(local_context);
   ```

##########
File path: libminifi/src/controllers/SSLContextService.cpp
##########
@@ -56,30 +56,30 @@ void SSLContextService::initialize() {
 bool SSLContextService::configure_ssl_context(SSL_CTX *ctx) {
   if (!IsNullOrEmpty(certificate)) {
     if (isFileTypeP12(certificate)) {
-      BIO* fp = BIO_new(BIO_s_file());
+      auto fp_deleter = [](BIO* ptr) { BIO_free(ptr); };
+      std::unique_ptr<BIO, decltype(fp_deleter)> fp(BIO_new(BIO_s_file()), fp_deleter);
       if (fp == nullptr) {
         logging::LOG_ERROR(logger_) << "Failed create new file BIO, " << getLatestOpenSSLErrorString();
         return false;
       }
-      utils::ScopeGuard fp_guard([fp]() { BIO_free(fp); });
-      if (BIO_read_filename(fp, certificate.c_str()) <= 0) {
+      if (BIO_read_filename(fp.get(), certificate.c_str()) <= 0) {
         logging::LOG_ERROR(logger_) << "Failed to read certificate file " << certificate << ", " << getLatestOpenSSLErrorString();
         return false;
       }
-      PKCS12* p12 = d2i_PKCS12_bio(fp, nullptr);
+      auto p12_deleter = [](PKCS12* ptr) { PKCS12_free(ptr); };
+      std::unique_ptr<PKCS12, decltype(p12_deleter)> p12(d2i_PKCS12_bio(fp.get(), nullptr), p12_deleter);
       if (p12 == nullptr) {
         logging::LOG_ERROR(logger_) << "Failed to DER decode certificate file " << certificate << ", " << getLatestOpenSSLErrorString();
         return false;
       }
-      utils::ScopeGuard p12_guard([p12]() { PKCS12_free(p12); });
       EVP_PKEY* pkey = nullptr;
       X509* cert = nullptr;
       STACK_OF(X509)* ca = nullptr;
-      if (!PKCS12_parse(p12, passphrase_.c_str(), &pkey, &cert, &ca)) {
+      if (!PKCS12_parse(p12.get(), passphrase_.c_str(), &pkey, &cert, &ca)) {
         logging::LOG_ERROR(logger_) << "Failed to parse certificate file " << certificate << " as PKCS#12, " << getLatestOpenSSLErrorString();
         return false;
       }
-      utils::ScopeGuard certs_guard([pkey, cert, ca]() {
+      ::gsl::final_action<std::function<void()>> certs_guard([pkey, cert, ca]() {

Review comment:
       Please use `auto` and `gsl::finally`. It's not recommended (by gsl-lite) to depend on the global `gsl` namespace, as they want to eventually move to the `gsl_lite` namespace. `ScopeGuard` has this dependency to greatly simplify the implementation, but here it doesn't provide a benefit.
   ```suggestion
         const auto certs_guard = gsl::finally([pkey, cert, ca]() {
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org