From 37af90f721f23991ee43345669de3f2a789b76aa Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 29 Jul 2017 01:42:16 -0400 Subject: [PATCH] Convert a few more scopers. Bug: 132 Change-Id: I75d6ce5a2256a4b464ca6a9378ac6b63a9bd47e2 Reviewed-on: https://boringssl-review.googlesource.com/18644 Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/handshake_server.cc | 10 +++------ ssl/internal.h | 8 +++---- ssl/ssl_session.cc | 50 ++++++++++++++++++++--------------------- ssl/t1_lib.cc | 12 +++++----- ssl/tls13_server.cc | 21 ++++++++--------- 5 files changed, 46 insertions(+), 55 deletions(-) diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 9e15e7e4..fb1c4d8c 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc @@ -817,14 +817,10 @@ static int ssl3_select_parameters(SSL_HANDSHAKE *hs) { } /* Determine whether we are doing session resumption. */ + UniquePtr 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 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: diff --git a/ssl/internal.h b/ssl/internal.h index 3ac6716d..fe73d1ec 100644 --- a/ssl/internal.h +++ b/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 *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 *out_session, int *out_renew_ticket, const uint8_t *ticket, size_t ticket_len, const uint8_t *session_id, size_t session_id_len); diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 02d64221..a6804718 100644 --- a/ssl/ssl_session.cc +++ b/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 *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 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, ©); + session.reset(ssl->session_ctx->get_session_cb(ssl, (uint8_t *)session_id, + session_id_len, ©)); - 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 *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 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; diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 9dbf24c1..27a942a8 100644 --- a/ssl/t1_lib.cc +++ b/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 *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 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; } diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 067c427c..933affa8 100644 --- a/ssl/tls13_server.cc +++ b/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 *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 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 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;