src: refactor some code around crypto::NodeBIO

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);
@@ -5157,6 +5160,8 @@ void InitCryptoOnce() {
   ERR_load_ENGINE_strings();
   ENGINE_load_builtin_engines();
 #endif  // !OPENSSL_NO_ENGINE
+
+  NodeBIO::GetMethod();
 }
 
 

src/node_crypto.h

@@ -97,6 +97,8 @@ extern int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx);
 
 extern void UseExtraCaCerts(const std::string& file);
 
+void InitCryptoOnce();
+
 class SecureContext : public BaseObject {
  public:
   ~SecureContext() override {

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;
@@ -251,6 +247,8 @@ const BIO_METHOD* NodeBIO::GetMethod() {
 
   return &method;
 #else
+  // This is called from InitCryptoOnce() to avoid race conditions during
+  // initialization.
   static BIO_METHOD* method = nullptr;
 
   if (method == nullptr) {

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,14 @@ 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;
+
+  friend void node::crypto::InitCryptoOnce();
 };
 
 }  // namespace crypto

src/tls_wrap.cc

@@ -60,8 +60,6 @@ TLSWrap::TLSWrap(Environment* env,
       SSLWrap<TLSWrap>(env, sc, kind),
       StreamBase(env),
       sc_(sc),
-      enc_in_(nullptr),
-      enc_out_(nullptr),
       write_size_(0),
       started_(false),
       established_(false),
@@ -86,8 +84,6 @@ TLSWrap::TLSWrap(Environment* env,
 
 
 TLSWrap::~TLSWrap() {
-  enc_in_ = nullptr;
-  enc_out_ = nullptr;
   sc_ = nullptr;
 }
 
@@ -112,11 +108,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_);