From 4590a1c3a236986d9214f856233c0b2c1459ea0c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 26 Jul 2018 12:53:23 +0200 Subject: [PATCH] src: use smart pointers for NodeBIO PR-URL: https://github.com/nodejs/node/pull/21984 Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell --- src/node_crypto.cc | 11 +++++++---- src/node_crypto_bio.cc | 26 +++++++++++--------------- src/node_crypto_bio.h | 33 +++++++++++++++------------------ src/tls_wrap.cc | 8 +++----- 4 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 6dc7aa2a73e1d8..de2545f3859970 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -483,7 +483,7 @@ void SecureContext::Init(const FunctionCallbackInfo& 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 v) { +static BIOPointer LoadBIO(Environment* env, Local 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 index 094bb9cc1f8822..ab68c0a08111f2 100644 --- 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(GetMethod())); + BIOPointer bio(BIO_new(const_cast(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(len) || - BIO_set_mem_eof_return(bio, 0) != 1) { - BIO_free(bio); - return nullptr; + BIO_write(bio.get(), data, len) != static_cast(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 index b4aa85f8fa36fa..fefd097b32a7ba 100644 --- 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). 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 index 3efa6adb4edb0e..3e6c66d5e36d07 100644 --- 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_);