src: more automatic memory management in node_crypto.cc

Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of `goto`s.

PR-URL: https://github.com/nodejs/node/pull/20238
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

1 files changed, 55 insertions, 67 deletions

diff --git a/src/node_crypto.h b/src/node_crypto.h
index 6f34eae359..ee933ede1f 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -75,6 +75,32 @@ struct MarkPopErrorOnReturn {
   ~MarkPopErrorOnReturn() { ERR_pop_to_mark(); }
 };
 
+template <typename T, void (*function)(T*)>
+struct FunctionDeleter {
+  void operator()(T* pointer) const { function(pointer); }
+  typedef std::unique_ptr<T, FunctionDeleter> Pointer;
+};
+
+template <typename T, void (*function)(T*)>
+using DeleteFnPtr = typename FunctionDeleter<T, function>::Pointer;
+
+// Define smart pointers for the most commonly used OpenSSL types:
+using X509Pointer = DeleteFnPtr<X509, X509_free>;
+using BIOPointer = DeleteFnPtr<BIO, BIO_free_all>;
+using SSLCtxPointer = DeleteFnPtr<SSL_CTX, SSL_CTX_free>;
+using SSLSessionPointer = DeleteFnPtr<SSL_SESSION, SSL_SESSION_free>;
+using SSLPointer = DeleteFnPtr<SSL, SSL_free>;
+using EVPKeyPointer = DeleteFnPtr<EVP_PKEY, EVP_PKEY_free>;
+using EVPKeyCtxPointer = DeleteFnPtr<EVP_PKEY_CTX, EVP_PKEY_CTX_free>;
+using EVPMDPointer = DeleteFnPtr<EVP_MD_CTX, EVP_MD_CTX_free>;
+using RSAPointer = DeleteFnPtr<RSA, RSA_free>;
+using BignumPointer = DeleteFnPtr<BIGNUM, BN_free>;
+using NetscapeSPKIPointer = DeleteFnPtr<NETSCAPE_SPKI, NETSCAPE_SPKI_free>;
+using ECGroupPointer = DeleteFnPtr<EC_GROUP, EC_GROUP_free>;
+using ECPointPointer = DeleteFnPtr<EC_POINT, EC_POINT_free>;
+using ECKeyPointer = DeleteFnPtr<EC_KEY, EC_KEY_free>;
+using DHPointer = DeleteFnPtr<DH, DH_free>;
+
 enum CheckResult {
   CHECK_CERT_REVOKED = 0,
   CHECK_OK = 1
@@ -87,14 +113,14 @@ extern void UseExtraCaCerts(const std::string& file);
 class SecureContext : public BaseObject {
  public:
   ~SecureContext() override {
-    FreeCTXMem();
+    Reset();
   }
 
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
-  SSL_CTX* ctx_;
-  X509* cert_;
-  X509* issuer_;
+  SSLCtxPointer ctx_;
+  X509Pointer cert_;
+  X509Pointer issuer_;
 #ifndef OPENSSL_NO_ENGINE
   bool client_cert_engine_provided_ = false;
 #endif  // !OPENSSL_NO_ENGINE
@@ -171,28 +197,16 @@ class SecureContext : public BaseObject {
 #endif
 
   SecureContext(Environment* env, v8::Local<v8::Object> wrap)
-      : BaseObject(env, wrap),
-        ctx_(nullptr),
-        cert_(nullptr),
-        issuer_(nullptr) {
+      : BaseObject(env, wrap) {
     MakeWeak();
     env->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize);
   }
 
-  void FreeCTXMem() {
-    if (!ctx_) {
-      return;
-    }
-
+  inline void Reset() {
     env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
-    SSL_CTX_free(ctx_);
-    if (cert_ != nullptr)
-      X509_free(cert_);
-    if (issuer_ != nullptr)
-      X509_free(issuer_);
-    ctx_ = nullptr;
-    cert_ = nullptr;
-    issuer_ = nullptr;
+    ctx_.reset();
+    cert_.reset();
+    issuer_.reset();
   }
 };
 
@@ -215,20 +229,15 @@ class SSLWrap {
         cert_cb_(nullptr),
         cert_cb_arg_(nullptr),
         cert_cb_running_(false) {
-    ssl_ = SSL_new(sc->ctx_);
+    ssl_.reset(SSL_new(sc->ctx_.get()));
+    CHECK(ssl_);
     env_->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize);
-    CHECK_NE(ssl_, nullptr);
   }
 
   virtual ~SSLWrap() {
     DestroySSL();
-    if (next_sess_ != nullptr) {
-      SSL_SESSION_free(next_sess_);
-      next_sess_ = nullptr;
-    }
   }
 
-  inline SSL* ssl() const { return ssl_; }
   inline void enable_session_callbacks() { session_callbacks_ = true; }
   inline bool is_server() const { return kind_ == kServer; }
   inline bool is_client() const { return kind_ == kClient; }
@@ -319,8 +328,8 @@ class SSLWrap {
 
   Environment* const env_;
   Kind kind_;
-  SSL_SESSION* next_sess_;
-  SSL* ssl_;
+  SSLSessionPointer next_sess_;
+  SSLPointer ssl_;
   bool session_callbacks_;
   bool new_session_wait_;
 
@@ -344,10 +353,6 @@ class SSLWrap {
 
 class CipherBase : public BaseObject {
  public:
-  ~CipherBase() override {
-    EVP_CIPHER_CTX_free(ctx_);
-  }
-
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
  protected:
@@ -407,7 +412,7 @@ class CipherBase : public BaseObject {
   }
 
  private:
-  EVP_CIPHER_CTX* ctx_;
+  DeleteFnPtr<EVP_CIPHER_CTX, EVP_CIPHER_CTX_free> ctx_;
   const CipherKind kind_;
   bool auth_tag_set_;
   unsigned int auth_tag_len_;
@@ -418,8 +423,6 @@ class CipherBase : public BaseObject {
 
 class Hmac : public BaseObject {
  public:
-  ~Hmac() override;
-
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
  protected:
@@ -438,13 +441,11 @@ class Hmac : public BaseObject {
   }
 
  private:
-  HMAC_CTX* ctx_;
+  DeleteFnPtr<HMAC_CTX, HMAC_CTX_free> ctx_;
 };
 
 class Hash : public BaseObject {
  public:
-  ~Hash() override;
-
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
   bool HashInit(const char* hash_type);
@@ -463,7 +464,7 @@ class Hash : public BaseObject {
   }
 
  private:
-  EVP_MD_CTX* mdctx_;
+  EVPMDPointer mdctx_;
   bool finalized_;
 };
 
@@ -480,19 +481,16 @@ class SignBase : public BaseObject {
   } Error;
 
   SignBase(Environment* env, v8::Local<v8::Object> wrap)
-      : BaseObject(env, wrap),
-        mdctx_(nullptr) {
+      : BaseObject(env, wrap) {
   }
 
-  ~SignBase() override;
-
   Error Init(const char* sign_type);
   Error Update(const char* data, int len);
 
  protected:
   void CheckThrow(Error error);
 
-  EVP_MD_CTX* mdctx_;
+  EVPMDPointer mdctx_;
 };
 
 class Sign : public SignBase {
@@ -573,12 +571,6 @@ class PublicKeyCipher {
 
 class DiffieHellman : public BaseObject {
  public:
-  ~DiffieHellman() override {
-    if (dh != nullptr) {
-      DH_free(dh);
-    }
-  }
-
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
   bool Init(int primeLength, int g);
@@ -603,8 +595,7 @@ class DiffieHellman : public BaseObject {
   DiffieHellman(Environment* env, v8::Local<v8::Object> wrap)
       : BaseObject(env, wrap),
         initialised_(false),
-        verifyError_(0),
-        dh(nullptr) {
+        verifyError_(0) {
     MakeWeak();
   }
 
@@ -618,29 +609,26 @@ class DiffieHellman : public BaseObject {
 
   bool initialised_;
   int verifyError_;
-  DH* dh;
+  DHPointer dh_;
 };
 
 class ECDH : public BaseObject {
  public:
   ~ECDH() override {
-    if (key_ != nullptr)
-      EC_KEY_free(key_);
-    key_ = nullptr;
     group_ = nullptr;
   }
 
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
-  static EC_POINT* BufferToPoint(Environment* env,
-                                 const EC_GROUP* group,
-                                 char* data,
-                                 size_t len);
+  static ECPointPointer BufferToPoint(Environment* env,
+                                      const EC_GROUP* group,
+                                      char* data,
+                                      size_t len);
 
  protected:
-  ECDH(Environment* env, v8::Local<v8::Object> wrap, EC_KEY* key)
+  ECDH(Environment* env, v8::Local<v8::Object> wrap, ECKeyPointer&& key)
       : BaseObject(env, wrap),
-        key_(key),
-        group_(EC_KEY_get0_group(key_)) {
+        key_(std::move(key)),
+        group_(EC_KEY_get0_group(key_.get())) {
     MakeWeak();
     CHECK_NE(group_, nullptr);
   }
@@ -654,9 +642,9 @@ class ECDH : public BaseObject {
   static void SetPublicKey(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   bool IsKeyPairValid();
-  bool IsKeyValidForCurve(const BIGNUM* private_key);
+  bool IsKeyValidForCurve(const BignumPointer& private_key);
 
-  EC_KEY* key_;
+  ECKeyPointer key_;
   const EC_GROUP* group_;
 };