Add bssl::UpRef.
bssl::UniquePtr and FOO_up_ref do not play well together. Add a helper
to simplify this. This allows us to write things like:
foo->cert = UpRef(bar->cert);
instead of:
if (bar->cert) {
X509_up_ref(bar->cert.get());
}
foo->cert.reset(bar->cert.get());
This also plays well with PushToStack. To append something to a stack
while taking a reference, it's just:
PushToStack(certs, UpRef(cert))
Change-Id: I99ae8de22b837588a2d8ffb58f86edc1d03ed46a
Reviewed-on: https://boringssl-review.googlesource.com/29584
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pool/pool_test.cc b/crypto/pool/pool_test.cc
index 1bf9bd1..3310fc3 100644
--- a/crypto/pool/pool_test.cc
+++ b/crypto/pool/pool_test.cc
@@ -29,8 +29,7 @@
Bytes(CRYPTO_BUFFER_data(buf.get()), CRYPTO_BUFFER_len(buf.get())));
// Test that reference-counting works properly.
- CRYPTO_BUFFER_up_ref(buf.get());
- bssl::UniquePtr<CRYPTO_BUFFER> buf2(buf.get());
+ bssl::UniquePtr<CRYPTO_BUFFER> buf2 = bssl::UpRef(buf);
}
TEST(PoolTest, Empty) {
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index b118215..0711992 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -530,10 +530,9 @@
return nullptr;
}
for (auto cert : certs) {
- if (!sk_X509_push(stack.get(), cert)) {
+ if (!bssl::PushToStack(stack.get(), bssl::UpRef(cert))) {
return nullptr;
}
- X509_up_ref(cert);
}
return stack;
@@ -548,10 +547,9 @@
return nullptr;
}
for (auto crl : crls) {
- if (!sk_X509_CRL_push(stack.get(), crl)) {
+ if (!bssl::PushToStack(stack.get(), bssl::UpRef(crl))) {
return nullptr;
}
- X509_CRL_up_ref(crl);
}
return stack;
diff --git a/fuzz/ssl_ctx_api.cc b/fuzz/ssl_ctx_api.cc
index 316980c..1566e79 100644
--- a/fuzz/ssl_ctx_api.cc
+++ b/fuzz/ssl_ctx_api.cc
@@ -212,10 +212,8 @@
cert_.reset(d2i_X509(NULL, &bufp, sizeof(kCertificateDER)));
assert(cert_.get() != nullptr);
- X509_up_ref(cert_.get());
-
certs_.reset(sk_X509_new_null());
- sk_X509_push(certs_.get(), cert_.get());
+ PushToStack(certs_.get(), UpRef(cert_));
}
bssl::UniquePtr<EVP_PKEY> pkey_;
diff --git a/include/openssl/base.h b/include/openssl/base.h
index 99505d1..84b847a 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -448,6 +448,18 @@
template <typename T>
using UniquePtr = std::unique_ptr<T, internal::Deleter<T>>;
+#define BORINGSSL_MAKE_UP_REF(type, up_ref_func) \
+ static inline UniquePtr<type> UpRef(type *v) { \
+ if (v != nullptr) { \
+ up_ref_func(v); \
+ } \
+ return UniquePtr<type>(v); \
+ } \
+ \
+ static inline UniquePtr<type> UpRef(const UniquePtr<type> &ptr) { \
+ return UpRef(ptr.get()); \
+ }
+
} // namespace bssl
} // extern C++
diff --git a/include/openssl/bio.h b/include/openssl/bio.h
index 5e3e2ef..adb641b 100644
--- a/include/openssl/bio.h
+++ b/include/openssl/bio.h
@@ -874,6 +874,7 @@
namespace bssl {
BORINGSSL_MAKE_DELETER(BIO, BIO_free)
+BORINGSSL_MAKE_UP_REF(BIO, BIO_up_ref)
} // namespace bssl
diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index bb017f1..c187ac1 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h
@@ -840,6 +840,7 @@
namespace bssl {
BORINGSSL_MAKE_DELETER(EVP_PKEY, EVP_PKEY_free)
+BORINGSSL_MAKE_UP_REF(EVP_PKEY, EVP_PKEY_up_ref)
BORINGSSL_MAKE_DELETER(EVP_PKEY_CTX, EVP_PKEY_CTX_free)
} // namespace bssl
diff --git a/include/openssl/pool.h b/include/openssl/pool.h
index 2c19c88..1259f4a 100644
--- a/include/openssl/pool.h
+++ b/include/openssl/pool.h
@@ -91,6 +91,7 @@
BORINGSSL_MAKE_DELETER(CRYPTO_BUFFER_POOL, CRYPTO_BUFFER_POOL_free)
BORINGSSL_MAKE_DELETER(CRYPTO_BUFFER, CRYPTO_BUFFER_free)
+BORINGSSL_MAKE_UP_REF(CRYPTO_BUFFER, CRYPTO_BUFFER_up_ref)
} // namespace bssl
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 4b42f9a..f04e0ec 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4444,7 +4444,9 @@
BORINGSSL_MAKE_DELETER(SSL, SSL_free)
BORINGSSL_MAKE_DELETER(SSL_CTX, SSL_CTX_free)
+BORINGSSL_MAKE_UP_REF(SSL_CTX, SSL_CTX_up_ref)
BORINGSSL_MAKE_DELETER(SSL_SESSION, SSL_SESSION_free)
+BORINGSSL_MAKE_UP_REF(SSL_SESSION, SSL_SESSION_up_ref)
// Certificate compression.
//
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 01c0ff2..79cadc3 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -1132,8 +1132,10 @@
BORINGSSL_MAKE_DELETER(NETSCAPE_SPKI, NETSCAPE_SPKI_free)
BORINGSSL_MAKE_DELETER(RSA_PSS_PARAMS, RSA_PSS_PARAMS_free)
BORINGSSL_MAKE_DELETER(X509, X509_free)
+BORINGSSL_MAKE_UP_REF(X509, X509_up_ref)
BORINGSSL_MAKE_DELETER(X509_ALGOR, X509_ALGOR_free)
BORINGSSL_MAKE_DELETER(X509_CRL, X509_CRL_free)
+BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref)
BORINGSSL_MAKE_DELETER(X509_CRL_METHOD, X509_CRL_METHOD_free)
BORINGSSL_MAKE_DELETER(X509_EXTENSION, X509_EXTENSION_free)
BORINGSSL_MAKE_DELETER(X509_INFO, X509_INFO_free)
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index bb67235..c649eba 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -465,8 +465,7 @@
// Stash the early data session, so connection properties may be queried out
// of it.
hs->in_early_data = true;
- SSL_SESSION_up_ref(ssl->session);
- hs->early_session.reset(ssl->session);
+ hs->early_session = UpRef(ssl->session);
hs->can_early_write = true;
hs->state = state_read_server_hello;
@@ -1622,8 +1621,7 @@
ssl->method->on_handshake_complete(ssl);
if (ssl->session != NULL) {
- SSL_SESSION_up_ref(ssl->session);
- ssl->s3->established_session.reset(ssl->session);
+ ssl->s3->established_session = UpRef(ssl->session);
} else {
// We make a copy of the session in order to maintain the immutability
// of the new established_session due to False Start. The caller may
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index f733402..65a6299 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1474,8 +1474,7 @@
}
if (ssl->session != NULL) {
- SSL_SESSION_up_ref(ssl->session);
- ssl->s3->established_session.reset(ssl->session);
+ ssl->s3->established_session = UpRef(ssl->session);
} else {
ssl->s3->established_session = std::move(hs->new_session);
ssl->s3->established_session->not_resumable = false;
diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc
index 531a8ee..54e87bf 100644
--- a/ssl/ssl_asn1.cc
+++ b/ssl/ssl_asn1.cc
@@ -703,10 +703,9 @@
return nullptr;
}
- CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&cert, pool);
- if (buffer == NULL ||
- !sk_CRYPTO_BUFFER_push(ret->certs, buffer)) {
- CRYPTO_BUFFER_free(buffer);
+ UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new_from_CBS(&cert, pool));
+ if (buffer == nullptr ||
+ !PushToStack(ret->certs, std::move(buffer))) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
return nullptr;
}
diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc
index a089c40..8b869c8 100644
--- a/ssl/ssl_cert.cc
+++ b/ssl/ssl_cert.cc
@@ -162,11 +162,7 @@
}
}
- if (cert->privatekey) {
- EVP_PKEY_up_ref(cert->privatekey.get());
- ret->privatekey.reset(cert->privatekey.get());
- }
-
+ ret->privatekey = UpRef(cert->privatekey);
ret->key_method = cert->key_method;
if (!ret->sigalgs.CopyFrom(cert->sigalgs)) {
@@ -178,16 +174,8 @@
ret->x509_method->cert_dup(ret.get(), cert);
- if (cert->signed_cert_timestamp_list) {
- CRYPTO_BUFFER_up_ref(cert->signed_cert_timestamp_list.get());
- ret->signed_cert_timestamp_list.reset(
- cert->signed_cert_timestamp_list.get());
- }
-
- if (cert->ocsp_response) {
- CRYPTO_BUFFER_up_ref(cert->ocsp_response.get());
- ret->ocsp_response.reset(cert->ocsp_response.get());
- }
+ ret->signed_cert_timestamp_list = UpRef(cert->signed_cert_timestamp_list);
+ ret->ocsp_response = UpRef(cert->ocsp_response);
ret->sid_ctx_length = cert->sid_ctx_length;
OPENSSL_memcpy(ret->sid_ctx, cert->sid_ctx, sizeof(ret->sid_ctx));
@@ -289,16 +277,12 @@
}
for (size_t i = 0; i < num_certs; i++) {
- if (!sk_CRYPTO_BUFFER_push(certs_sk.get(), certs[i])) {
+ if (!PushToStack(certs_sk.get(), UpRef(certs[i]))) {
return 0;
}
- CRYPTO_BUFFER_up_ref(certs[i]);
}
- if (privkey != nullptr) {
- EVP_PKEY_up_ref(privkey);
- }
- cert->privatekey.reset(privkey);
+ cert->privatekey = UpRef(privkey);
cert->key_method = privkey_method;
cert->chain = std::move(certs_sk);
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 9796f0c..0bf58a4 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -295,10 +295,10 @@
SSL_CTX_add_session(ctx, ssl->s3->established_session.get());
}
if (ctx->new_session_cb != NULL) {
- SSL_SESSION_up_ref(ssl->s3->established_session.get());
- if (!ctx->new_session_cb(ssl, ssl->s3->established_session.get())) {
+ UniquePtr<SSL_SESSION> ref = UpRef(ssl->s3->established_session);
+ if (ctx->new_session_cb(ssl, ref.get())) {
// |new_session_cb|'s return value signals whether it took ownership.
- SSL_SESSION_free(ssl->s3->established_session.get());
+ ref.release();
}
}
}
@@ -2836,8 +2836,7 @@
// depends on this behavior, so emulate it.
UniquePtr<SSL_SESSION> session;
if (!ssl->server && ssl->s3->established_session != NULL) {
- session.reset(ssl->s3->established_session.get());
- SSL_SESSION_up_ref(session.get());
+ session = UpRef(ssl->s3->established_session);
}
// The ssl->d1->mtu is simultaneously configuration (preserved across
diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index 0fcd805..d41ea5d 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -89,9 +89,7 @@
return 0;
}
- EVP_PKEY_up_ref(pkey);
- cert->privatekey.reset(pkey);
-
+ cert->privatekey = UpRef(pkey);
return 1;
}
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 703225e..1a098fb 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -209,17 +209,15 @@
}
}
if (session->certs != NULL) {
- new_session->certs = sk_CRYPTO_BUFFER_new_null();
+ auto buf_up_ref = [](CRYPTO_BUFFER *buf) {
+ CRYPTO_BUFFER_up_ref(buf);
+ return buf;
+ };
+ new_session->certs = sk_CRYPTO_BUFFER_deep_copy(session->certs, buf_up_ref,
+ CRYPTO_BUFFER_free);
if (new_session->certs == NULL) {
return nullptr;
}
- for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(session->certs); i++) {
- CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(session->certs, i);
- if (!sk_CRYPTO_BUFFER_push(new_session->certs, buffer)) {
- return nullptr;
- }
- CRYPTO_BUFFER_up_ref(buffer);
- }
}
if (!session->x509_method->session_dup(new_session.get(), session)) {
@@ -677,11 +675,8 @@
OPENSSL_memcpy(data.session_id, session_id, session_id_len);
MutexReadLock lock(&ssl->session_ctx->lock);
- session.reset(lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data));
- if (session) {
- // |lh_SSL_SESSION_retrieve| returns a non-owning pointer.
- SSL_SESSION_up_ref(session.get());
- }
+ // |lh_SSL_SESSION_retrieve| returns a non-owning pointer.
+ session = UpRef(lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data));
// TODO(davidben): This should probably move it to the front of the list.
}
@@ -1118,8 +1113,7 @@
int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *session) {
// Although |session| is inserted into two structures (a doubly-linked list
// and the hash table), |ctx| only takes one reference.
- SSL_SESSION_up_ref(session);
- UniquePtr<SSL_SESSION> owned_session(session);
+ UniquePtr<SSL_SESSION> owned_session = UpRef(session);
SSL_SESSION *old_session;
MutexWriteLock lock(&ctx->lock);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index cfcaa73..6fd5f36 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1322,9 +1322,7 @@
bssl::UniquePtr<STACK_OF(X509_NAME)> stack(sk_X509_NAME_new_null());
ASSERT_TRUE(stack);
-
- ASSERT_TRUE(sk_X509_NAME_push(stack.get(), name_dup.get()));
- name_dup.release();
+ ASSERT_TRUE(PushToStack(stack.get(), std::move(name_dup)));
// |SSL_set_client_CA_list| takes ownership.
SSL_set_client_CA_list(ssl.get(), stack.release());
@@ -3235,8 +3233,7 @@
bssl::UniquePtr<STACK_OF(CRYPTO_BUFFER)> ca_names(
sk_CRYPTO_BUFFER_new_null());
ASSERT_TRUE(ca_names);
- ASSERT_TRUE(sk_CRYPTO_BUFFER_push(ca_names.get(), ca_name.get()));
- ca_name.release();
+ ASSERT_TRUE(PushToStack(ca_names.get(), std::move(ca_name)));
SSL_CTX_set0_client_CAs(server_ctx.get(), ca_names.release());
// Configure client and server to accept all certificates.
diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc
index b9ea170..0af4167 100644
--- a/ssl/ssl_x509.cc
+++ b/ssl/ssl_x509.cc
@@ -209,13 +209,10 @@
return 0;
}
- CRYPTO_BUFFER *leaf = sk_CRYPTO_BUFFER_value(cert->chain.get(), 0);
- if (!sk_CRYPTO_BUFFER_push(new_chain.get(), leaf)) {
- return 0;
- }
// |leaf| might be NULL if it's a “leafless” chain.
- if (leaf != nullptr) {
- CRYPTO_BUFFER_up_ref(leaf);
+ CRYPTO_BUFFER *leaf = sk_CRYPTO_BUFFER_value(cert->chain.get(), 0);
+ if (!PushToStack(new_chain.get(), UpRef(leaf))) {
+ return 0;
}
}
@@ -552,12 +549,11 @@
for (size_t i = 1; i < sk_X509_num(session->x509_chain); i++) {
X509 *cert = sk_X509_value(session->x509_chain, i);
- if (!sk_X509_push(session->x509_chain_without_leaf, cert)) {
+ if (!PushToStack(session->x509_chain_without_leaf, UpRef(cert))) {
sk_X509_pop_free(session->x509_chain_without_leaf, X509_free);
session->x509_chain_without_leaf = NULL;
return NULL;
}
- X509_up_ref(cert);
}
}
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index d12cd18..c300ab0 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -203,10 +203,9 @@
break;
}
- if (!sk_X509_push(out_chain->get(), cert.get())) {
+ if (!bssl::PushToStack(out_chain->get(), std::move(cert))) {
return false;
}
- cert.release(); // sk_X509_push takes ownership.
}
uint32_t err = ERR_peek_last_error();
@@ -316,10 +315,9 @@
return nullptr;
}
- if (!sk_X509_NAME_push(ret.get(), name.get())) {
+ if (!bssl::PushToStack(ret.get(), std::move(name))) {
return nullptr;
}
- name.release();
}
return ret;
@@ -1640,7 +1638,7 @@
if (!sk_X509_insert(expect_chain.get(), expect_leaf.get(), 0)) {
return false;
}
- X509_up_ref(expect_leaf.get()); // sk_X509_push takes ownership.
+ X509_up_ref(expect_leaf.get()); // sk_X509_insert takes ownership.
}
bssl::UniquePtr<X509> leaf(SSL_get_peer_certificate(ssl));
