src: use smart pointers for NodeBIO

PR-URL: #21984 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Jul 26, 2018 · d85b0a3c107f596dbd35d79ba321b1294d7be100 · d85b0a3
1 parent f506a5f
commit d85b0a3c107f596dbd35d79ba321b1294d7be100
Unified Split

Showing with 36 additions and 42 deletions.

+7 −4 src/node_crypto.cc

+11 −15 src/node_crypto_bio.cc

+15 −18 src/node_crypto_bio.h

+3 −5 src/tls_wrap.cc
diff --git a/src/node_crypto.cc b/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);

diff --git a/src/node_crypto_bio.cc b/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;

diff --git a/src/node_crypto_bio.h b/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

diff --git a/src/tls_wrap.cc b/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_);