Convert a few more scopers.

Bug: 132
Change-Id: I75d6ce5a2256a4b464ca6a9378ac6b63a9bd47e2
Reviewed-on: https://boringssl-review.googlesource.com/18644
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/handshake_server.cc

@@ -817,14 +817,10 @@ static int ssl3_select_parameters(SSL_HANDSHAKE *hs) {
   }
 
   /* Determine whether we are doing session resumption. */
+  UniquePtr<SSL_SESSION> session;
   int tickets_supported = 0, renew_ticket = 0;
-  /* TODO(davidben): Switch |ssl_get_prev_session| to take a |UniquePtr|
-   * output and simplify this. */
-  SSL_SESSION *session_raw = nullptr;
-  auto session_ret = ssl_get_prev_session(ssl, &session_raw, &tickets_supported,
-                                          &renew_ticket, &client_hello);
-  UniquePtr<SSL_SESSION> session(session_raw);
-  switch (session_ret) {
+  switch (ssl_get_prev_session(ssl, &session, &tickets_supported, &renew_ticket,
+                               &client_hello)) {
     case ssl_session_success:
       break;
     case ssl_session_error:

ssl/internal.h

@@ -2083,13 +2083,13 @@ enum ssl_session_result_t {
 };
 
 /* ssl_get_prev_session looks up the previous session based on |client_hello|.
- * On success, it sets |*out_session| to the session or NULL if none was found.
- * If the session could not be looked up synchronously, it returns
+ * On success, it sets |*out_session| to the session or nullptr if none was
+ * found. If the session could not be looked up synchronously, it returns
  * |ssl_session_retry| and should be called again. If a ticket could not be
  * decrypted immediately it returns |ssl_session_ticket_retry| and should also
  * be called again. Otherwise, it returns |ssl_session_error|.  */
 enum ssl_session_result_t ssl_get_prev_session(
-    SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported,
+    SSL *ssl, UniquePtr<SSL_SESSION> *out_session, int *out_tickets_supported,
     int *out_renew_ticket, const SSL_CLIENT_HELLO *client_hello);
 
 /* The following flags determine which parts of the session are duplicated. */
@@ -2267,7 +2267,7 @@ int ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs);
  *       Retry later.
  *   |ssl_ticket_aead_error|: an error occured that is fatal to the connection. */
 enum ssl_ticket_aead_result_t ssl_process_ticket(
-    SSL *ssl, SSL_SESSION **out_session, int *out_renew_ticket,
+    SSL *ssl, UniquePtr<SSL_SESSION> *out_session, int *out_renew_ticket,
     const uint8_t *ticket, size_t ticket_len, const uint8_t *session_id,
     size_t session_id_len);
 

ssl/ssl_session.cc

@@ -610,18 +610,17 @@ int ssl_session_is_resumable(const SSL_HANDSHAKE *hs,
 }
 
 /* ssl_lookup_session looks up |session_id| in the session cache and sets
- * |*out_session| to an |SSL_SESSION| object if found. The caller takes
- * ownership of the result. */
+ * |*out_session| to an |SSL_SESSION| object if found. */
 static enum ssl_session_result_t ssl_lookup_session(
-    SSL *ssl, SSL_SESSION **out_session, const uint8_t *session_id,
+    SSL *ssl, UniquePtr<SSL_SESSION> *out_session, const uint8_t *session_id,
     size_t session_id_len) {
-  *out_session = NULL;
+  out_session->reset();
 
   if (session_id_len == 0 || session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) {
     return ssl_session_success;
   }
 
-  SSL_SESSION *session = NULL;
+  UniquePtr<SSL_SESSION> session;
   /* Try the internal cache, if it exists. */
   if (!(ssl->session_ctx->session_cache_mode &
         SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) {
@@ -631,26 +630,27 @@ static enum ssl_session_result_t ssl_lookup_session(
     OPENSSL_memcpy(data.session_id, session_id, session_id_len);
 
     CRYPTO_MUTEX_lock_read(&ssl->session_ctx->lock);
-    session = lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data);
-    if (session != NULL) {
-      SSL_SESSION_up_ref(session);
+    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());
     }
     /* TODO(davidben): This should probably move it to the front of the list. */
     CRYPTO_MUTEX_unlock_read(&ssl->session_ctx->lock);
   }
 
   /* Fall back to the external cache, if it exists. */
-  if (session == NULL &&
-      ssl->session_ctx->get_session_cb != NULL) {
+  if (!session && ssl->session_ctx->get_session_cb != NULL) {
     int copy = 1;
-    session = ssl->session_ctx->get_session_cb(ssl, (uint8_t *)session_id,
-                                               session_id_len, &copy);
+    session.reset(ssl->session_ctx->get_session_cb(ssl, (uint8_t *)session_id,
+                                                   session_id_len, &copy));
 
-    if (session == NULL) {
+    if (!session) {
       return ssl_session_success;
     }
 
-    if (session == SSL_magic_pending_session_ptr()) {
+    if (session.get() == SSL_magic_pending_session_ptr()) {
+      session.release();  // This pointer is not actually owned.
       return ssl_session_retry;
     }
 
@@ -659,34 +659,32 @@ static enum ssl_session_result_t ssl_lookup_session(
      * between threads, it must handle the reference count itself [i.e. copy ==
      * 0], or things won't be thread-safe). */
     if (copy) {
-      SSL_SESSION_up_ref(session);
+      SSL_SESSION_up_ref(session.get());
     }
 
     /* Add the externally cached session to the internal cache if necessary. */
     if (!(ssl->session_ctx->session_cache_mode &
           SSL_SESS_CACHE_NO_INTERNAL_STORE)) {
-      SSL_CTX_add_session(ssl->session_ctx, session);
+      SSL_CTX_add_session(ssl->session_ctx, session.get());
     }
   }
 
-  if (session != NULL &&
-      !ssl_session_is_time_valid(ssl, session)) {
+  if (session && !ssl_session_is_time_valid(ssl, session.get())) {
     /* The session was from the cache, so remove it. */
-    SSL_CTX_remove_session(ssl->session_ctx, session);
-    SSL_SESSION_free(session);
-    session = NULL;
+    SSL_CTX_remove_session(ssl->session_ctx, session.get());
+    session.reset();
   }
 
-  *out_session = session;
+  *out_session = std::move(session);
   return ssl_session_success;
 }
 
 enum ssl_session_result_t ssl_get_prev_session(
-    SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported,
+    SSL *ssl, UniquePtr<SSL_SESSION> *out_session, int *out_tickets_supported,
     int *out_renew_ticket, const SSL_CLIENT_HELLO *client_hello) {
   /* This is used only by servers. */
   assert(ssl->server);
-  SSL_SESSION *session = NULL;
+  UniquePtr<SSL_SESSION> session;
   int renew_ticket = 0;
 
   /* If tickets are disabled, always behave as if no tickets are present. */
@@ -704,7 +702,7 @@ enum ssl_session_result_t ssl_get_prev_session(
       case ssl_ticket_aead_success:
         break;
       case ssl_ticket_aead_ignore_ticket:
-        assert(session == NULL);
+        assert(!session);
         break;
       case ssl_ticket_aead_error:
         return ssl_session_error;
@@ -720,7 +718,7 @@ enum ssl_session_result_t ssl_get_prev_session(
     }
   }
 
-  *out_session = session;
+  *out_session = std::move(session);
   *out_tickets_supported = tickets_supported;
   *out_renew_ticket = renew_ticket;
   return ssl_session_success;

ssl/t1_lib.cc

@@ -3123,11 +3123,11 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_method(
 }
 
 enum ssl_ticket_aead_result_t ssl_process_ticket(
-    SSL *ssl, SSL_SESSION **out_session, int *out_renew_ticket,
+    SSL *ssl, UniquePtr<SSL_SESSION> *out_session, int *out_renew_ticket,
     const uint8_t *ticket, size_t ticket_len, const uint8_t *session_id,
     size_t session_id_len) {
   *out_renew_ticket = 0;
-  *out_session = NULL;
+  out_session->reset();
 
   if ((SSL_get_options(ssl) & SSL_OP_NO_TICKET) ||
       session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) {
@@ -3150,11 +3150,11 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
   }
 
   /* Decode the session. */
-  SSL_SESSION *session =
-      SSL_SESSION_from_bytes(plaintext, plaintext_len, ssl->ctx);
+  UniquePtr<SSL_SESSION> session(
+      SSL_SESSION_from_bytes(plaintext, plaintext_len, ssl->ctx));
   OPENSSL_free(plaintext);
 
-  if (session == NULL) {
+  if (!session) {
     ERR_clear_error(); /* Don't leave an error on the queue. */
     return ssl_ticket_aead_ignore_ticket;
   }
@@ -3164,7 +3164,7 @@ enum ssl_ticket_aead_result_t ssl_process_ticket(
   OPENSSL_memcpy(session->session_id, session_id, session_id_len);
   session->session_id_length = session_id_len;
 
-  *out_session = session;
+  *out_session = std::move(session);
   return ssl_ticket_aead_success;
 }
 

ssl/tls13_server.cc

@@ -255,7 +255,7 @@ static enum ssl_hs_wait_t do_select_parameters(SSL_HANDSHAKE *hs) {
 }
 
 static enum ssl_ticket_aead_result_t select_session(
-    SSL_HANDSHAKE *hs, uint8_t *out_alert, SSL_SESSION **out_session,
+    SSL_HANDSHAKE *hs, uint8_t *out_alert, UniquePtr<SSL_SESSION> *out_session,
     int32_t *out_ticket_age_skew, const SSL_CLIENT_HELLO *client_hello) {
   SSL *const ssl = hs->ssl;
   *out_session = NULL;
@@ -288,7 +288,7 @@ static enum ssl_ticket_aead_result_t select_session(
   /* TLS 1.3 session tickets are renewed separately as part of the
    * NewSessionTicket. */
   int unused_renew;
-  SSL_SESSION *session = NULL;
+  UniquePtr<SSL_SESSION> session;
   enum ssl_ticket_aead_result_t ret =
       ssl_process_ticket(ssl, &session, &unused_renew, CBS_data(&ticket),
                          CBS_len(&ticket), NULL, 0);
@@ -302,10 +302,9 @@ static enum ssl_ticket_aead_result_t select_session(
       return ret;
   }
 
-  if (!ssl_session_is_resumable(hs, session) ||
+  if (!ssl_session_is_resumable(hs, session.get()) ||
       /* Historically, some TLS 1.3 tickets were missing ticket_age_add. */
       !session->ticket_age_add_valid) {
-    SSL_SESSION_free(session);
     return ssl_ticket_aead_ignore_ticket;
   }
 
@@ -323,7 +322,6 @@ static enum ssl_ticket_aead_result_t select_session(
   /* To avoid overflowing |hs->ticket_age_skew|, we will not resume
    * 68-year-old sessions. */
   if (server_ticket_age > INT32_MAX) {
-    SSL_SESSION_free(session);
     return ssl_ticket_aead_ignore_ticket;
   }
 
@@ -333,13 +331,12 @@ static enum ssl_ticket_aead_result_t select_session(
       (int32_t)client_ticket_age - (int32_t)server_ticket_age;
 
   /* Check the PSK binder. */
-  if (!tls13_verify_psk_binder(hs, session, &binders)) {
-    SSL_SESSION_free(session);
+  if (!tls13_verify_psk_binder(hs, session.get(), &binders)) {
     *out_alert = SSL_AD_DECRYPT_ERROR;
     return ssl_ticket_aead_error;
   }
 
-  *out_session = session;
+  *out_session = std::move(session);
   return ssl_ticket_aead_success;
 }
 
@@ -354,11 +351,11 @@ static enum ssl_hs_wait_t do_select_session(SSL_HANDSHAKE *hs) {
   }
 
   uint8_t alert = SSL_AD_DECODE_ERROR;
-  SSL_SESSION *session = NULL;
+  UniquePtr<SSL_SESSION> session;
   switch (select_session(hs, &alert, &session, &ssl->s3->ticket_age_skew,
                          &client_hello)) {
     case ssl_ticket_aead_ignore_ticket:
-      assert(session == NULL);
+      assert(!session);
       if (!ssl_get_new_session(hs, 1 /* server */)) {
         ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
         return ssl_hs_error;
@@ -368,7 +365,8 @@ static enum ssl_hs_wait_t do_select_session(SSL_HANDSHAKE *hs) {
     case ssl_ticket_aead_success:
       /* Carry over authentication information from the previous handshake into
        * a fresh session. */
-      hs->new_session = SSL_SESSION_dup(session, SSL_SESSION_DUP_AUTH_ONLY);
+      hs->new_session =
+          SSL_SESSION_dup(session.get(), SSL_SESSION_DUP_AUTH_ONLY);
 
       if (/* Early data must be acceptable for this ticket. */
           ssl->cert->enable_early_data &&
@@ -384,7 +382,6 @@ static enum ssl_hs_wait_t do_select_session(SSL_HANDSHAKE *hs) {
         ssl->early_data_accepted = 1;
       }
 
-      SSL_SESSION_free(session);
       if (hs->new_session == NULL) {
         ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
         return ssl_hs_error;