Skip to content

Commit 2f17c52

Browse files
bnoordhuisMylesBorins
authored andcommitted
src: use std::unique_ptr for STACK_OF(X509)
Convert manual memory management of STACK_OF(X509) instances to std::unique_ptr with a custom deleter. Note the tasteful application of std::move() to indicate a function that consumes its argument. PR-URL: #19087 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
1 parent f10470c commit 2f17c52

File tree

1 file changed

+44
-54
lines changed

1 file changed

+44
-54
lines changed

src/node_crypto.cc

+44-54
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,14 @@
4343
// StartComAndWoSignData.inc
4444
#include "StartComAndWoSignData.inc"
4545

46-
#include <algorithm>
4746
#include <errno.h>
4847
#include <limits.h> // INT_MAX
4948
#include <math.h>
5049
#include <stdlib.h>
5150
#include <string.h>
51+
52+
#include <algorithm>
53+
#include <memory>
5254
#include <vector>
5355

5456
#define THROW_AND_RETURN_IF_NOT_STRING_OR_BUFFER(val, prefix) \
@@ -115,6 +117,12 @@ using v8::String;
115117
using v8::Value;
116118

117119

120+
struct StackOfX509Deleter {
121+
void operator()(STACK_OF(X509)* p) const { sk_X509_pop_free(p, X509_free); }
122+
};
123+
124+
using StackOfX509 = std::unique_ptr<STACK_OF(X509), StackOfX509Deleter>;
125+
118126
#if OPENSSL_VERSION_NUMBER < 0x10100000L
119127
static void RSA_get0_key(const RSA* r, const BIGNUM** n, const BIGNUM** e,
120128
const BIGNUM** d) {
@@ -839,17 +847,15 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
839847
int ret = 0;
840848
unsigned long err = 0; // NOLINT(runtime/int)
841849

842-
// Read extra certs
843-
STACK_OF(X509)* extra_certs = sk_X509_new_null();
844-
if (extra_certs == nullptr) {
850+
StackOfX509 extra_certs(sk_X509_new_null());
851+
if (!extra_certs)
845852
goto done;
846-
}
847853

848854
while ((extra = PEM_read_bio_X509(in,
849855
nullptr,
850856
NoPasswordCallback,
851857
nullptr))) {
852-
if (sk_X509_push(extra_certs, extra))
858+
if (sk_X509_push(extra_certs.get(), extra))
853859
continue;
854860

855861
// Failure, free all certs
@@ -867,13 +873,11 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
867873
goto done;
868874
}
869875

870-
ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs, cert, issuer);
876+
ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs.get(), cert, issuer);
871877
if (!ret)
872878
goto done;
873879

874880
done:
875-
if (extra_certs != nullptr)
876-
sk_X509_pop_free(extra_certs, X509_free);
877881
if (extra != nullptr)
878882
X509_free(extra);
879883
if (x != nullptr)
@@ -2004,14 +2008,14 @@ static Local<Object> X509ToObject(Environment* env, X509* cert) {
20042008

20052009
static Local<Object> AddIssuerChainToObject(X509** cert,
20062010
Local<Object> object,
2007-
STACK_OF(X509)* const peer_certs,
2011+
StackOfX509 peer_certs,
20082012
Environment* const env) {
20092013
Local<Context> context = env->isolate()->GetCurrentContext();
2010-
*cert = sk_X509_delete(peer_certs, 0);
2014+
*cert = sk_X509_delete(peer_certs.get(), 0);
20112015
for (;;) {
20122016
int i;
2013-
for (i = 0; i < sk_X509_num(peer_certs); i++) {
2014-
X509* ca = sk_X509_value(peer_certs, i);
2017+
for (i = 0; i < sk_X509_num(peer_certs.get()); i++) {
2018+
X509* ca = sk_X509_value(peer_certs.get(), i);
20152019
if (X509_check_issued(ca, *cert) != X509_V_OK)
20162020
continue;
20172021

@@ -2023,41 +2027,31 @@ static Local<Object> AddIssuerChainToObject(X509** cert,
20232027
X509_free(*cert);
20242028

20252029
// Delete cert and continue aggregating issuers.
2026-
*cert = sk_X509_delete(peer_certs, i);
2030+
*cert = sk_X509_delete(peer_certs.get(), i);
20272031
break;
20282032
}
20292033

20302034
// Issuer not found, break out of the loop.
2031-
if (i == sk_X509_num(peer_certs))
2035+
if (i == sk_X509_num(peer_certs.get()))
20322036
break;
20332037
}
2034-
sk_X509_pop_free(peer_certs, X509_free);
20352038
return object;
20362039
}
20372040

20382041

2039-
static bool CloneSSLCerts(X509** cert,
2040-
const STACK_OF(X509)* const ssl_certs,
2041-
STACK_OF(X509)** peer_certs) {
2042-
*peer_certs = sk_X509_new(nullptr);
2043-
bool result = true;
2042+
static StackOfX509 CloneSSLCerts(X509** cert,
2043+
const STACK_OF(X509)* const ssl_certs) {
2044+
StackOfX509 peer_certs(sk_X509_new(nullptr));
20442045
if (*cert != nullptr)
2045-
sk_X509_push(*peer_certs, *cert);
2046+
sk_X509_push(peer_certs.get(), *cert);
20462047
for (int i = 0; i < sk_X509_num(ssl_certs); i++) {
20472048
*cert = X509_dup(sk_X509_value(ssl_certs, i));
2048-
if (*cert == nullptr) {
2049-
result = false;
2050-
break;
2051-
}
2052-
if (!sk_X509_push(*peer_certs, *cert)) {
2053-
result = false;
2054-
break;
2055-
}
2056-
}
2057-
if (!result) {
2058-
sk_X509_pop_free(*peer_certs, X509_free);
2049+
if (*cert == nullptr)
2050+
return StackOfX509();
2051+
if (!sk_X509_push(peer_certs.get(), *cert))
2052+
return StackOfX509();
20592053
}
2060-
return result;
2054+
return peer_certs;
20612055
}
20622056

20632057

@@ -2091,7 +2085,6 @@ void SSLWrap<Base>::GetPeerCertificate(
20912085
Base* w;
20922086
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
20932087
Environment* env = w->ssl_env();
2094-
Local<Context> context = env->context();
20952088

20962089
ClearErrorOnReturn clear_error_on_return;
20972090

@@ -2103,23 +2096,25 @@ void SSLWrap<Base>::GetPeerCertificate(
21032096
// contains the `peer_certificate`, but on server it doesn't.
21042097
X509* cert = w->is_server() ? SSL_get_peer_certificate(w->ssl_) : nullptr;
21052098
STACK_OF(X509)* ssl_certs = SSL_get_peer_cert_chain(w->ssl_);
2106-
STACK_OF(X509)* peer_certs = nullptr;
21072099
if (cert == nullptr && (ssl_certs == nullptr || sk_X509_num(ssl_certs) == 0))
21082100
goto done;
21092101

21102102
// Short result requested.
21112103
if (args.Length() < 1 || !args[0]->IsTrue()) {
2112-
result = X509ToObject(env,
2113-
cert == nullptr ? sk_X509_value(ssl_certs, 0) : cert);
2104+
X509* target_cert = cert;
2105+
if (target_cert == nullptr)
2106+
target_cert = sk_X509_value(ssl_certs, 0);
2107+
result = X509ToObject(env, target_cert);
21142108
goto done;
21152109
}
21162110

2117-
if (CloneSSLCerts(&cert, ssl_certs, &peer_certs)) {
2111+
if (auto peer_certs = CloneSSLCerts(&cert, ssl_certs)) {
21182112
// First and main certificate.
2119-
cert = sk_X509_value(peer_certs, 0);
2113+
cert = sk_X509_value(peer_certs.get(), 0);
21202114
result = X509ToObject(env, cert);
21212115

2122-
issuer_chain = AddIssuerChainToObject(&cert, result, peer_certs, env);
2116+
issuer_chain =
2117+
AddIssuerChainToObject(&cert, result, std::move(peer_certs), env);
21232118
issuer_chain = GetLastIssuedCert(&cert, w->ssl_, issuer_chain, env);
21242119
// Last certificate should be self-signed.
21252120
if (X509_check_issued(cert, cert) == X509_V_OK)
@@ -3184,25 +3179,23 @@ inline CheckResult CheckWhitelistedServerCert(X509_STORE_CTX* ctx) {
31843179
unsigned char hash[CNNIC_WHITELIST_HASH_LEN];
31853180
unsigned int hashlen = CNNIC_WHITELIST_HASH_LEN;
31863181

3187-
STACK_OF(X509)* chain = X509_STORE_CTX_get1_chain(ctx);
3188-
CHECK_NE(chain, nullptr);
3189-
CHECK_GT(sk_X509_num(chain), 0);
3182+
StackOfX509 chain(X509_STORE_CTX_get1_chain(ctx));
3183+
CHECK(chain);
3184+
CHECK_GT(sk_X509_num(chain.get()), 0);
31903185

31913186
// Take the last cert as root at the first time.
3192-
X509* root_cert = sk_X509_value(chain, sk_X509_num(chain)-1);
3187+
X509* root_cert = sk_X509_value(chain.get(), sk_X509_num(chain.get())-1);
31933188
X509_NAME* root_name = X509_get_subject_name(root_cert);
31943189

31953190
if (!IsSelfSigned(root_cert)) {
3196-
root_cert = FindRoot(chain);
3191+
root_cert = FindRoot(chain.get());
31973192
CHECK_NE(root_cert, nullptr);
31983193
root_name = X509_get_subject_name(root_cert);
31993194
}
32003195

3201-
X509* leaf_cert = sk_X509_value(chain, 0);
3202-
if (!CheckStartComOrWoSign(root_name, leaf_cert)) {
3203-
sk_X509_pop_free(chain, X509_free);
3196+
X509* leaf_cert = sk_X509_value(chain.get(), 0);
3197+
if (!CheckStartComOrWoSign(root_name, leaf_cert))
32043198
return CHECK_CERT_REVOKED;
3205-
}
32063199

32073200
// When the cert is issued from either CNNNIC ROOT CA or CNNNIC EV
32083201
// ROOT CA, check a hash of its leaf cert if it is in the whitelist.
@@ -3215,13 +3208,10 @@ inline CheckResult CheckWhitelistedServerCert(X509_STORE_CTX* ctx) {
32153208
void* result = bsearch(hash, WhitelistedCNNICHashes,
32163209
arraysize(WhitelistedCNNICHashes),
32173210
CNNIC_WHITELIST_HASH_LEN, compar);
3218-
if (result == nullptr) {
3219-
sk_X509_pop_free(chain, X509_free);
3211+
if (result == nullptr)
32203212
return CHECK_CERT_REVOKED;
3221-
}
32223213
}
32233214

3224-
sk_X509_pop_free(chain, X509_free);
32253215
return CHECK_OK;
32263216
}
32273217

0 commit comments

Comments
 (0)