From 2908dd141fa7f654b712330220e5c9ec75e62fb4 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 29 Jun 2018 17:46:42 -0400 Subject: [PATCH] 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 CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Adam Langley --- crypto/pool/pool_test.cc | 3 +-- crypto/x509/x509_test.cc | 6 ++---- fuzz/ssl_ctx_api.cc | 4 +--- include/openssl/base.h | 12 ++++++++++++ include/openssl/bio.h | 1 + include/openssl/evp.h | 1 + include/openssl/pool.h | 1 + include/openssl/ssl.h | 2 ++ include/openssl/x509.h | 2 ++ ssl/handshake_client.cc | 6 ++---- ssl/handshake_server.cc | 3 +-- ssl/ssl_asn1.cc | 7 +++---- ssl/ssl_cert.cc | 26 +++++--------------------- ssl/ssl_lib.cc | 9 ++++----- ssl/ssl_privkey.cc | 4 +--- ssl/ssl_session.cc | 24 +++++++++--------------- ssl/ssl_test.cc | 7 ++----- ssl/ssl_x509.cc | 12 ++++-------- ssl/test/bssl_shim.cc | 8 +++----- 19 files changed, 57 insertions(+), 81 deletions(-) diff --git a/crypto/pool/pool_test.cc b/crypto/pool/pool_test.cc index 1bf9bd19..3310fc3f 100644 --- a/crypto/pool/pool_test.cc +++ b/crypto/pool/pool_test.cc @@ -29,8 +29,7 @@ TEST(PoolTest, Unpooled) { 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 buf2(buf.get()); + bssl::UniquePtr buf2 = bssl::UpRef(buf); } TEST(PoolTest, Empty) { diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index b1182155..07119924 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -530,10 +530,9 @@ static bssl::UniquePtr CertsToStack( 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 @@ static bssl::UniquePtr CRLsToStack( 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 316980cf..1566e796 100644 --- a/fuzz/ssl_ctx_api.cc +++ b/fuzz/ssl_ctx_api.cc @@ -212,10 +212,8 @@ struct GlobalState { 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 pkey_; diff --git a/include/openssl/base.h b/include/openssl/base.h index 99505d1f..84b847a6 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -448,6 +448,18 @@ class StackAllocated { template using UniquePtr = std::unique_ptr>; +#define BORINGSSL_MAKE_UP_REF(type, up_ref_func) \ + static inline UniquePtr UpRef(type *v) { \ + if (v != nullptr) { \ + up_ref_func(v); \ + } \ + return UniquePtr(v); \ + } \ + \ + static inline UniquePtr UpRef(const UniquePtr &ptr) { \ + return UpRef(ptr.get()); \ + } + } // namespace bssl } // extern C++ diff --git a/include/openssl/bio.h b/include/openssl/bio.h index 5e3e2ef2..adb641b2 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h @@ -874,6 +874,7 @@ extern "C++" { 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 bb017f17..c187ac19 100644 --- a/include/openssl/evp.h +++ b/include/openssl/evp.h @@ -840,6 +840,7 @@ extern "C++" { 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 2c19c889..1259f4a5 100644 --- a/include/openssl/pool.h +++ b/include/openssl/pool.h @@ -91,6 +91,7 @@ namespace bssl { 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 4b42f9a7..f04e0ecd 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4444,7 +4444,9 @@ namespace bssl { 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 01c0ff22..79cadc3e 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -1132,8 +1132,10 @@ namespace bssl { 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 bb67235b..c649eba4 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -465,8 +465,7 @@ static enum ssl_hs_wait_t do_enter_early_data(SSL_HANDSHAKE *hs) { // 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 @@ static enum ssl_hs_wait_t do_finish_client_handshake(SSL_HANDSHAKE *hs) { 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 f733402f..65a62990 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -1474,8 +1474,7 @@ static enum ssl_hs_wait_t do_finish_server_handshake(SSL_HANDSHAKE *hs) { } 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 531a8ee5..54e87bf0 100644 --- a/ssl/ssl_asn1.cc +++ b/ssl/ssl_asn1.cc @@ -703,10 +703,9 @@ UniquePtr SSL_SESSION_parse(CBS *cbs, 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 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 a089c40d..8b869c8a 100644 --- a/ssl/ssl_cert.cc +++ b/ssl/ssl_cert.cc @@ -162,11 +162,7 @@ UniquePtr ssl_cert_dup(CERT *cert) { } } - 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 @@ UniquePtr ssl_cert_dup(CERT *cert) { 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 @@ static int cert_set_chain_and_key( } 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 9796f0c5..0bf58a4f 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -295,10 +295,10 @@ void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) { 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 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 @@ int SSL_clear(SSL *ssl) { // depends on this behavior, so emulate it. UniquePtr 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 0fcd8051..d41ea5d8 100644 --- a/ssl/ssl_privkey.cc +++ b/ssl/ssl_privkey.cc @@ -89,9 +89,7 @@ static int ssl_set_pkey(CERT *cert, EVP_PKEY *pkey) { 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 703225e7..1a098fbd 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc @@ -209,17 +209,15 @@ UniquePtr SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) { } } 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 @@ static enum ssl_hs_wait_t ssl_lookup_session( 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 @@ void *SSL_SESSION_get_ex_data(const SSL_SESSION *session, int idx) { 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 owned_session(session); + UniquePtr 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 cfcaa73d..6fd5f363 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -1322,9 +1322,7 @@ TEST(SSLTest, ClientCAList) { bssl::UniquePtr 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 @@ TEST(SSLTest, ClientCABuffers) { bssl::UniquePtr 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 b9ea170e..0af4167a 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -209,13 +209,10 @@ static int ssl_cert_set_chain(CERT *cert, STACK_OF(X509) *chain) { 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 @@ STACK_OF(X509) *SSL_get_peer_cert_chain(const SSL *ssl) { 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 d12cd184..c300ab06 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -203,10 +203,9 @@ static bool LoadCertificate(bssl::UniquePtr *out_x509, 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 @@ static bssl::UniquePtr DecodeHexX509Names( 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 @@ static bool CheckAuthProperties(SSL *ssl, bool is_resume, 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 leaf(SSL_get_peer_certificate(ssl));