commit bfdd1a93084f55d01448648e98a3fb3e97e0e8c1
author David Benjamin <davidben@google.com> Fri Jun 29 16:26:38 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jul 03 22:57:56 2018 +0000
tree cd0d3f7444c46919baf569f074f9a88186d3efd4
parent 58150ed59b69c2e6c35379d96f4f43c5f5838d03

Give SSL_SESSION a destructor.

Previously we'd partially attempted the ssl_st / bssl::SSLConnection
subclassing split, but that gets messy when we actually try to add a
destructor, because CRYPTO_EX_DATA's cleanup function needs an ssl_st*,
not a bssl::SSLConnection*. Downcasting is technically undefined at this
point and will likely offend some CFI-like check.

Moreover, it appears that even with today's subclassing split,
New<SSL>() emits symbols like:

W ssl_st*& std::forward<ssl_st*&>(std::remove_reference<ssl_st*&>::type&)

The compiler does not bother emitting them in optimized builds, but it
does suggest we can't really avoid claiming the ssl_st type name at the
symbol level, short of doing reinterpret_casts at all API boundaries.
And, of course, we've already long claimed it at the #include level.

So I've just left this defining directly on ssl_session_st. The cost is
we need to write some silly "bssl::" prefixes in the headers, but so it
goes. In the likely event we change our minds again, we can always
revise this.

Change-Id: Ieb429e8eaabe7c2961ef7f8d9234fb71f19a5e2a
Reviewed-on: https://boringssl-review.googlesource.com/29587
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>

ssl/tls13_both.cc

diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc
index 495838c..9149e76 100644
--- a/ssl/tls13_both.cc
+++ b/ssl/tls13_both.cc
@@ -256,9 +256,8 @@
       }
 
       if (sk_CRYPTO_BUFFER_num(certs.get()) == 1) {
-        CRYPTO_BUFFER_free(hs->new_session->ocsp_response);
-        hs->new_session->ocsp_response =
-            CRYPTO_BUFFER_new_from_CBS(&ocsp_response, ssl->ctx->pool);
+        hs->new_session->ocsp_response.reset(
+            CRYPTO_BUFFER_new_from_CBS(&ocsp_response, ssl->ctx->pool));
         if (hs->new_session->ocsp_response == nullptr) {
           ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
           return 0;
@@ -280,9 +279,8 @@
       }
 
       if (sk_CRYPTO_BUFFER_num(certs.get()) == 1) {
-        CRYPTO_BUFFER_free(hs->new_session->signed_cert_timestamp_list);
-        hs->new_session->signed_cert_timestamp_list =
-            CRYPTO_BUFFER_new_from_CBS(&sct, ssl->ctx->pool);
+        hs->new_session->signed_cert_timestamp_list.reset(
+            CRYPTO_BUFFER_new_from_CBS(&sct, ssl->ctx->pool));
         if (hs->new_session->signed_cert_timestamp_list == nullptr) {
           ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
           return 0;
@@ -298,9 +296,7 @@
   }
 
   hs->peer_pubkey = std::move(pkey);
-
-  sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
-  hs->new_session->certs = certs.release();
+  hs->new_session->certs = std::move(certs);
 
   if (!ssl->ctx->x509_method->session_cache_objects(hs->new_session.get())) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
@@ -308,7 +304,7 @@
     return 0;
   }
 
-  if (sk_CRYPTO_BUFFER_num(hs->new_session->certs) == 0) {
+  if (sk_CRYPTO_BUFFER_num(hs->new_session->certs.get()) == 0) {
     if (!allow_anonymous) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE);
       ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_CERTIFICATE_REQUIRED);