Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 2f17c52

Browse filesBrowse files
bnoordhuisMylesBorins
bnoordhuis
authored and
MylesBorins
committed
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 <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
1 parent f10470c commit 2f17c52
Copy full SHA for 2f17c52

File tree

Expand file treeCollapse file tree

1 file changed

+44
-54
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

1 file changed

+44
-54
lines changed
Open diff view settings
Collapse file

‎src/node_crypto.cc‎

Copy file name to clipboardExpand all lines: src/node_crypto.cc
+44-54Lines changed: 44 additions & 54 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.