src: use smart pointers for NodeBIO

PR-URL: #21984
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

src/node_crypto.cc

@@ -483,7 +483,7 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
 
 // Takes a string or buffer and loads it into a BIO.
 // Caller responsible for BIO_free_all-ing the returned object.
-static BIO* LoadBIO(Environment* env, Local<Value> v) {
+static BIOPointer LoadBIO(Environment* env, Local<Value> v) {
   HandleScope scope(env->isolate());
 
   if (v->IsString()) {
@@ -738,9 +738,12 @@ static X509_STORE* NewRootCertStore() {
 
   if (root_certs_vector.empty()) {
     for (size_t i = 0; i < arraysize(root_certs); i++) {
-      BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
-      X509* x509 = PEM_read_bio_X509(bp, nullptr, NoPasswordCallback, nullptr);
-      BIO_free(bp);
+      X509* x509 =
+          PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i],
+                                              strlen(root_certs[i])).get(),
+                            nullptr,   // no re-use of X509 structure
+                            NoPasswordCallback,
+                            nullptr);  // no callback data
 
       // Parse errors from the built-in roots are fatal.
       CHECK_NOT_NULL(x509);

src/node_crypto_bio.cc

@@ -38,36 +38,32 @@ namespace crypto {
 #endif
 
 
-BIO* NodeBIO::New() {
+BIOPointer NodeBIO::New(Environment* env) {
   // The const_cast doesn't violate const correctness.  OpenSSL's usage of
   // BIO_METHOD is effectively const but BIO_new() takes a non-const argument.
-  return BIO_new(const_cast<BIO_METHOD*>(GetMethod()));
+  BIOPointer bio(BIO_new(const_cast<BIO_METHOD*>(GetMethod())));
+  if (bio && env != nullptr)
+    NodeBIO::FromBIO(bio.get())->env_ = env;
+  return bio;
 }
 
 
-BIO* NodeBIO::NewFixed(const char* data, size_t len) {
-  BIO* bio = New();
+BIOPointer NodeBIO::NewFixed(const char* data, size_t len, Environment* env) {
+  BIOPointer bio = New(env);
 
-  if (bio == nullptr ||
+  if (!bio ||
       len > INT_MAX ||
-      BIO_write(bio, data, len) != static_cast<int>(len) ||
-      BIO_set_mem_eof_return(bio, 0) != 1) {
-    BIO_free(bio);
-    return nullptr;
+      BIO_write(bio.get(), data, len) != static_cast<int>(len) ||
+      BIO_set_mem_eof_return(bio.get(), 0) != 1) {
+    return BIOPointer();
   }
 
   return bio;
 }
 
 
-void NodeBIO::AssignEnvironment(Environment* env) {
-  env_ = env;
-}
-
-
 int NodeBIO::New(BIO* bio) {
   BIO_set_data(bio, new NodeBIO());
-
   BIO_set_init(bio, 1);
 
   return 1;

src/node_crypto_bio.h

@@ -24,6 +24,7 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#include "node_crypto.h"
 #include "openssl/bio.h"
 #include "env-inl.h"
 #include "util-inl.h"
@@ -32,25 +33,21 @@
 namespace node {
 namespace crypto {
 
+// This class represents buffers for OpenSSL I/O, implemented as a singly-linked
+// list of chunks. It can be used both for writing data from Node to OpenSSL
+// and back, but only one direction per instance.
+// The structure is only accessed, and owned by, the OpenSSL BIOPointer
+// (a.k.a. std::unique_ptr<BIO>).
 class NodeBIO : public MemoryRetainer {
  public:
-  NodeBIO() : env_(nullptr),
-              initial_(kInitialBufferLength),
-              length_(0),
-              eof_return_(-1),
-              read_head_(nullptr),
-              write_head_(nullptr) {
-  }
-
   ~NodeBIO();
 
-  static BIO* New();
+  static BIOPointer New(Environment* env = nullptr);
 
   // NewFixed takes a copy of `len` bytes from `data` and returns a BIO that,
   // when read from, returns those bytes followed by EOF.
-  static BIO* NewFixed(const char* data, size_t len);
-
-  void AssignEnvironment(Environment* env);
+  static BIOPointer NewFixed(const char* data, size_t len,
+                             Environment* env = nullptr);
 
   // Move read head to next buffer if needed
   void TryMoveReadHead();
@@ -161,12 +158,12 @@ class NodeBIO : public MemoryRetainer {
     char* data_;
   };
 
-  Environment* env_;
-  size_t initial_;
-  size_t length_;
-  int eof_return_;
-  Buffer* read_head_;
-  Buffer* write_head_;
+  Environment* env_ = nullptr;
+  size_t initial_ = kInitialBufferLength;
+  size_t length_ = 0;
+  int eof_return_ = -1;
+  Buffer* read_head_ = nullptr;
+  Buffer* write_head_ = nullptr;
 };
 
 }  // namespace crypto

src/tls_wrap.cc

@@ -112,11 +112,9 @@ void TLSWrap::NewSessionDoneCb() {
 
 
 void TLSWrap::InitSSL() {
-  // Initialize SSL
-  enc_in_ = crypto::NodeBIO::New();
-  enc_out_ = crypto::NodeBIO::New();
-  crypto::NodeBIO::FromBIO(enc_in_)->AssignEnvironment(env());
-  crypto::NodeBIO::FromBIO(enc_out_)->AssignEnvironment(env());
+  // Initialize SSL – OpenSSL takes ownership of these.
+  enc_in_ = crypto::NodeBIO::New(env()).release();
+  enc_out_ = crypto::NodeBIO::New(env()).release();
 
   SSL_set_bio(ssl_.get(), enc_in_, enc_out_);