Give ssl_cipher_preference_list_st a destructor.

Change-Id: I578a284c6a8cae773a97d3d30ad8a5cd13f56164
Reviewed-on: https://boringssl-review.googlesource.com/27491
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>

ssl/handshake_server.cc

@@ -332,14 +332,14 @@ static void ssl_get_compatible_server_ciphers(SSL_HANDSHAKE *hs,
 
 static const SSL_CIPHER *ssl3_choose_cipher(
     SSL_HANDSHAKE *hs, const SSL_CLIENT_HELLO *client_hello,
-    const struct ssl_cipher_preference_list_st *server_pref) {
+    const SSLCipherPreferenceList *server_pref) {
   SSL *const ssl = hs->ssl;
   const STACK_OF(SSL_CIPHER) *prio, *allow;
   // in_group_flags will either be NULL, or will point to an array of bytes
   // which indicate equal-preference groups in the |prio| stack. See the
-  // comment about |in_group_flags| in the |ssl_cipher_preference_list_st|
+  // comment about |in_group_flags| in the |SSLCipherPreferenceList|
   // struct.
-  const uint8_t *in_group_flags;
+  const bool *in_group_flags;
   // group_min contains the minimal index so far found in a group, or -1 if no
   // such value exists yet.
   int group_min = -1;
@@ -351,13 +351,13 @@ static const SSL_CIPHER *ssl3_choose_cipher(
   }
 
   if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
-    prio = server_pref->ciphers;
+    prio = server_pref->ciphers.get();
     in_group_flags = server_pref->in_group_flags;
     allow = client_pref.get();
   } else {
     prio = client_pref.get();
     in_group_flags = NULL;
-    allow = server_pref->ciphers;
+    allow = server_pref->ciphers.get();
   }
 
   uint32_t mask_k, mask_a;
@@ -375,7 +375,7 @@ static const SSL_CIPHER *ssl3_choose_cipher(
         (c->algorithm_auth & mask_a) &&
         // Check the cipher is in the |allow| list.
         sk_SSL_CIPHER_find(allow, &cipher_index, c)) {
-      if (in_group_flags != NULL && in_group_flags[i] == 1) {
+      if (in_group_flags != NULL && in_group_flags[i]) {
         // This element of |prio| is in a group. Update the minimum index found
         // so far and continue looking.
         if (group_min == -1 || (size_t)group_min > cipher_index) {
@@ -389,7 +389,7 @@ static const SSL_CIPHER *ssl3_choose_cipher(
       }
     }
 
-    if (in_group_flags != NULL && in_group_flags[i] == 0 && group_min != -1) {
+    if (in_group_flags != NULL && !in_group_flags[i] && group_min != -1) {
       // We are about to leave a group, but we found a match in it, so that's
       // our answer.
       return sk_SSL_CIPHER_value(allow, group_min);

ssl/internal.h

@@ -457,9 +457,48 @@ namespace bssl {
 #define SSL_HANDSHAKE_MAC_SHA256 0x2
 #define SSL_HANDSHAKE_MAC_SHA384 0x4
 
-// SSL_MAX_DIGEST is the number of digest types which exist. When adding a new
-// one, update the table in ssl_cipher.c.
-#define SSL_MAX_DIGEST 4
+// An SSLCipherPreferenceList contains a list of SSL_CIPHERs with equal-
+// preference groups. For TLS clients, the groups are moot because the server
+// picks the cipher and groups cannot be expressed on the wire. However, for
+// servers, the equal-preference groups allow the client's preferences to be
+// partially respected. (This only has an effect with
+// SSL_OP_CIPHER_SERVER_PREFERENCE).
+//
+// The equal-preference groups are expressed by grouping SSL_CIPHERs together.
+// All elements of a group have the same priority: no ordering is expressed
+// within a group.
+//
+// The values in |ciphers| are in one-to-one correspondence with
+// |in_group_flags|. (That is, sk_SSL_CIPHER_num(ciphers) is the number of
+// bytes in |in_group_flags|.) The bytes in |in_group_flags| are either 1, to
+// indicate that the corresponding SSL_CIPHER is not the last element of a
+// group, or 0 to indicate that it is.
+//
+// For example, if |in_group_flags| contains all zeros then that indicates a
+// traditional, fully-ordered preference. Every SSL_CIPHER is the last element
+// of the group (i.e. they are all in a one-element group).
+//
+// For a more complex example, consider:
+//   ciphers:        A  B  C  D  E  F
+//   in_group_flags: 1  1  0  0  1  0
+//
+// That would express the following, order:
+//
+//    A         E
+//    B -> D -> F
+//    C
+struct SSLCipherPreferenceList {
+  static constexpr bool kAllowUniquePtr = true;
+
+  SSLCipherPreferenceList() = default;
+  ~SSLCipherPreferenceList();
+
+  bool Init(UniquePtr<STACK_OF(SSL_CIPHER)> ciphers,
+            Span<const bool> in_group_flags);
+
+  UniquePtr<STACK_OF(SSL_CIPHER)> ciphers;
+  bool *in_group_flags = nullptr;
+};
 
 // ssl_cipher_get_evp_aead sets |*out_aead| to point to the correct EVP_AEAD
 // object for |cipher| protocol version |version|. It sets |*out_mac_secret_len|
@@ -477,13 +516,12 @@ const EVP_MD *ssl_get_handshake_digest(uint16_t version,
                                        const SSL_CIPHER *cipher);
 
 // ssl_create_cipher_list evaluates |rule_str|. It sets |*out_cipher_list| to a
-// newly-allocated |ssl_cipher_preference_list_st| containing the result. It
-// returns true on success and false on failure. If |strict| is true, nonsense
-// will be rejected. If false, nonsense will be silently ignored. An empty
-// result is considered an error regardless of |strict|.
-bool ssl_create_cipher_list(
-    struct ssl_cipher_preference_list_st **out_cipher_list,
-    const char *rule_str, bool strict);
+// newly-allocated |SSLCipherPreferenceList| containing the result. It returns
+// true on success and false on failure. If |strict| is true, nonsense will be
+// rejected. If false, nonsense will be silently ignored. An empty result is
+// considered an error regardless of |strict|.
+bool ssl_create_cipher_list(SSLCipherPreferenceList **out_cipher_list,
+                            const char *rule_str, bool strict);
 
 // ssl_cipher_get_value returns the cipher suite id of |cipher|.
 uint16_t ssl_cipher_get_value(const SSL_CIPHER *cipher);
@@ -1921,41 +1959,6 @@ extern const SSL_X509_METHOD ssl_crypto_x509_method;
 // crypto/x509.
 extern const SSL_X509_METHOD ssl_noop_x509_method;
 
-// ssl_cipher_preference_list_st contains a list of SSL_CIPHERs with
-// equal-preference groups. For TLS clients, the groups are moot because the
-// server picks the cipher and groups cannot be expressed on the wire. However,
-// for servers, the equal-preference groups allow the client's preferences to
-// be partially respected. (This only has an effect with
-// SSL_OP_CIPHER_SERVER_PREFERENCE).
-//
-// The equal-preference groups are expressed by grouping SSL_CIPHERs together.
-// All elements of a group have the same priority: no ordering is expressed
-// within a group.
-//
-// The values in |ciphers| are in one-to-one correspondence with
-// |in_group_flags|. (That is, sk_SSL_CIPHER_num(ciphers) is the number of
-// bytes in |in_group_flags|.) The bytes in |in_group_flags| are either 1, to
-// indicate that the corresponding SSL_CIPHER is not the last element of a
-// group, or 0 to indicate that it is.
-//
-// For example, if |in_group_flags| contains all zeros then that indicates a
-// traditional, fully-ordered preference. Every SSL_CIPHER is the last element
-// of the group (i.e. they are all in a one-element group).
-//
-// For a more complex example, consider:
-//   ciphers:        A  B  C  D  E  F
-//   in_group_flags: 1  1  0  0  1  0
-//
-// That would express the following, order:
-//
-//    A         E
-//    B -> D -> F
-//    C
-struct ssl_cipher_preference_list_st {
-  STACK_OF(SSL_CIPHER) *ciphers;
-  uint8_t *in_group_flags;
-};
-
 struct tlsext_ticket_key {
   static constexpr bool kAllowUniquePtr = true;
 
@@ -1998,7 +2001,7 @@ struct SSLContext {
   // configuration.
   enum tls13_variant_t tls13_variant;
 
-  struct ssl_cipher_preference_list_st *cipher_list;
+  SSLCipherPreferenceList *cipher_list;
 
   X509_STORE *cert_store;
   LHASH_OF(SSL_SESSION) *sessions;
@@ -2621,7 +2624,7 @@ struct SSLConnection {
   X509_VERIFY_PARAM *param;
 
   // crypto
-  struct ssl_cipher_preference_list_st *cipher_list;
+  SSLCipherPreferenceList *cipher_list;
 
   // session info
 
@@ -2853,13 +2856,9 @@ void ssl_session_rebase_time(SSL *ssl, SSL_SESSION *session);
 void ssl_session_renew_timeout(SSL *ssl, SSL_SESSION *session,
                                uint32_t timeout);
 
-void ssl_cipher_preference_list_free(
-    struct ssl_cipher_preference_list_st *cipher_list);
-
 // ssl_get_cipher_preferences returns the cipher preference list for TLS 1.2 and
 // below.
-const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences(
-    const SSL *ssl);
+const SSLCipherPreferenceList *ssl_get_cipher_preferences(const SSL *ssl);
 
 void ssl_update_cache(SSL_HANDSHAKE *hs, int mode);
 

ssl/s3_lib.cc

@@ -215,8 +215,7 @@ void ssl3_free(SSL *ssl) {
   ssl->s3 = NULL;
 }
 
-const struct ssl_cipher_preference_list_st *ssl_get_cipher_preferences(
-    const SSL *ssl) {
+const SSLCipherPreferenceList *ssl_get_cipher_preferences(const SSL *ssl) {
   if (ssl->cipher_list != NULL) {
     return ssl->cipher_list;
   }

ssl/ssl_cipher.cc

@@ -811,9 +811,14 @@ static void ll_append_head(CIPHER_ORDER **head, CIPHER_ORDER *curr,
   *head = curr;
 }
 
-static void ssl_cipher_collect_ciphers(CIPHER_ORDER *co_list,
-                                       CIPHER_ORDER **head_p,
-                                       CIPHER_ORDER **tail_p) {
+static bool ssl_cipher_collect_ciphers(Array<CIPHER_ORDER> *out_co_list,
+                                       CIPHER_ORDER **out_head,
+                                       CIPHER_ORDER **out_tail) {
+  Array<CIPHER_ORDER> co_list;
+  if (!co_list.Init(kCiphersLen)) {
+    return false;
+  }
+
   size_t co_list_num = 0;
   for (const SSL_CIPHER &cipher : kCiphers) {
     // TLS 1.3 ciphers do not participate in this mechanism.
@@ -844,9 +849,35 @@ static void ssl_cipher_collect_ciphers(CIPHER_ORDER *co_list,
 
     co_list[co_list_num - 1].next = NULL;
 
-    *head_p = &co_list[0];
-    *tail_p = &co_list[co_list_num - 1];
+    *out_head = &co_list[0];
+    *out_tail = &co_list[co_list_num - 1];
+  } else {
+    *out_head = nullptr;
+    *out_tail = nullptr;
   }
+  *out_co_list = std::move(co_list);
+  return true;
+}
+
+SSLCipherPreferenceList::~SSLCipherPreferenceList() {
+  OPENSSL_free(in_group_flags);
+}
+
+bool SSLCipherPreferenceList::Init(UniquePtr<STACK_OF(SSL_CIPHER)> ciphers_arg,
+                                   Span<const bool> in_group_flags_arg) {
+  if (sk_SSL_CIPHER_num(ciphers_arg.get()) != in_group_flags_arg.size()) {
+    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+    return false;
+  }
+
+  Array<bool> copy;
+  if (!copy.CopyFrom(in_group_flags_arg)) {
+    return false;
+  }
+  ciphers = std::move(ciphers_arg);
+  size_t unused_len;
+  copy.Release(&in_group_flags, &unused_len);
+  return true;
 }
 
 // ssl_cipher_apply_rule applies the rule type |rule| to ciphers matching its
@@ -1201,15 +1232,8 @@ static bool ssl_cipher_process_rulestr(const char *rule_str,
   return true;
 }
 
-bool ssl_create_cipher_list(
-    struct ssl_cipher_preference_list_st **out_cipher_list,
-    const char *rule_str, bool strict) {
-  STACK_OF(SSL_CIPHER) *cipherstack = NULL;
-  CIPHER_ORDER *co_list = NULL, *head = NULL, *tail = NULL, *curr;
-  uint8_t *in_group_flags = NULL;
-  unsigned int num_in_group_flags = 0;
-  struct ssl_cipher_preference_list_st *pref_list = NULL;
-
+bool ssl_create_cipher_list(SSLCipherPreferenceList **out_cipher_list,
+                            const char *rule_str, bool strict) {
   // Return with error if nothing to do.
   if (rule_str == NULL || out_cipher_list == NULL) {
     return false;
@@ -1218,14 +1242,12 @@ bool ssl_create_cipher_list(
   // Now we have to collect the available ciphers from the compiled in ciphers.
   // We cannot get more than the number compiled in, so it is used for
   // allocation.
-  co_list = (CIPHER_ORDER *)OPENSSL_malloc(sizeof(CIPHER_ORDER) * kCiphersLen);
-  if (co_list == NULL) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+  Array<CIPHER_ORDER> co_list;
+  CIPHER_ORDER *head = nullptr, *tail = nullptr;
+  if (!ssl_cipher_collect_ciphers(&co_list, &head, &tail)) {
     return false;
   }
 
-  ssl_cipher_collect_ciphers(co_list, &head, &tail);
-
   // Now arrange all ciphers by preference:
   // TODO(davidben): Compute this order once and copy it.
 
@@ -1285,7 +1307,7 @@ bool ssl_create_cipher_list(
   if (strncmp(rule_str, "DEFAULT", 7) == 0) {
     if (!ssl_cipher_process_rulestr(SSL_DEFAULT_CIPHER_LIST, &head, &tail,
                                     strict)) {
-      goto err;
+      return false;
     }
     rule_p += 7;
     if (*rule_p == ':') {
@@ -1295,75 +1317,52 @@ bool ssl_create_cipher_list(
 
   if (*rule_p != '\0' &&
       !ssl_cipher_process_rulestr(rule_p, &head, &tail, strict)) {
-    goto err;
+    return false;
   }
 
   // Allocate new "cipherstack" for the result, return with error
   // if we cannot get one.
-  cipherstack = sk_SSL_CIPHER_new_null();
-  if (cipherstack == NULL) {
-    goto err;
-  }
-
-  in_group_flags = (uint8_t *)OPENSSL_malloc(kCiphersLen);
-  if (!in_group_flags) {
-    goto err;
+  UniquePtr<STACK_OF(SSL_CIPHER)> cipherstack(sk_SSL_CIPHER_new_null());
+  Array<bool> in_group_flags;
+  if (cipherstack == nullptr ||
+      !in_group_flags.Init(kCiphersLen)) {
+    return false;
   }
 
   // The cipher selection for the list is done. The ciphers are added
   // to the resulting precedence to the STACK_OF(SSL_CIPHER).
-  for (curr = head; curr != NULL; curr = curr->next) {
+  size_t num_in_group_flags = 0;
+  for (CIPHER_ORDER *curr = head; curr != NULL; curr = curr->next) {
     if (curr->active) {
-      if (!sk_SSL_CIPHER_push(cipherstack, curr->cipher)) {
-        goto err;
+      if (!sk_SSL_CIPHER_push(cipherstack.get(), curr->cipher)) {
+        return false;
       }
       in_group_flags[num_in_group_flags++] = curr->in_group;
     }
   }
-  OPENSSL_free(co_list);  // Not needed any longer
-  co_list = NULL;
 
-  pref_list = (ssl_cipher_preference_list_st *)OPENSSL_malloc(
-      sizeof(struct ssl_cipher_preference_list_st));
-  if (!pref_list) {
-    goto err;
-  }
-  pref_list->ciphers = cipherstack;
-  pref_list->in_group_flags = NULL;
-  if (num_in_group_flags) {
-    pref_list->in_group_flags = (uint8_t *)OPENSSL_malloc(num_in_group_flags);
-    if (!pref_list->in_group_flags) {
-      goto err;
-    }
-    OPENSSL_memcpy(pref_list->in_group_flags, in_group_flags,
-                   num_in_group_flags);
+  UniquePtr<SSLCipherPreferenceList> pref_list =
+      MakeUnique<SSLCipherPreferenceList>();
+  if (!pref_list ||
+      !pref_list->Init(
+          std::move(cipherstack),
+          MakeConstSpan(in_group_flags).subspan(0, num_in_group_flags))) {
+    return false;
   }
-  OPENSSL_free(in_group_flags);
-  in_group_flags = NULL;
-  if (*out_cipher_list != NULL) {
-    ssl_cipher_preference_list_free(*out_cipher_list);
+
+  if (*out_cipher_list) {
+    Delete(*out_cipher_list);
   }
-  *out_cipher_list = pref_list;
-  pref_list = NULL;
+  *out_cipher_list = pref_list.release();
 
   // Configuring an empty cipher list is an error but still updates the
   // output.
-  if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers) == 0) {
+  if (sk_SSL_CIPHER_num((*out_cipher_list)->ciphers.get()) == 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_NO_CIPHER_MATCH);
     return false;
   }
 
   return true;
-
-err:
-  OPENSSL_free(co_list);
-  OPENSSL_free(in_group_flags);
-  sk_SSL_CIPHER_free(cipherstack);
-  if (pref_list) {
-    OPENSSL_free(pref_list->in_group_flags);
-  }
-  OPENSSL_free(pref_list);
-  return false;
 }
 
 uint16_t ssl_cipher_get_value(const SSL_CIPHER *cipher) {

ssl/ssl_lib.cc

@@ -272,16 +272,6 @@ ssl_open_record_t ssl_open_app_data(SSL *ssl, Span<uint8_t> *out,
   return ret;
 }
 
-void ssl_cipher_preference_list_free(
-    struct ssl_cipher_preference_list_st *cipher_list) {
-  if (cipher_list == NULL) {
-    return;
-  }
-  sk_SSL_CIPHER_free(cipher_list->ciphers);
-  OPENSSL_free(cipher_list->in_group_flags);
-  OPENSSL_free(cipher_list);
-}
-
 void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) {
   SSL *const ssl = hs->ssl;
   SSL_CTX *ctx = ssl->session_ctx;
@@ -622,7 +612,7 @@ void SSL_CTX_free(SSL_CTX *ctx) {
 
   CRYPTO_MUTEX_cleanup(&ctx->lock);
   lh_SSL_SESSION_free(ctx->sessions);
-  ssl_cipher_preference_list_free(ctx->cipher_list);
+  Delete(ctx->cipher_list);
   Delete(ctx->cert);
   sk_SSL_CUSTOM_EXTENSION_pop_free(ctx->client_custom_extensions,
                                    SSL_CUSTOM_EXTENSION_free);
@@ -765,7 +755,7 @@ void SSL_free(SSL *ssl) {
   BIO_free_all(ssl->wbio);
 
   // add extra stuff
-  ssl_cipher_preference_list_free(ssl->cipher_list);
+  Delete(ssl->cipher_list);
 
   SSL_SESSION_free(ssl->session);
 
@@ -1820,11 +1810,11 @@ int SSL_set_tmp_dh(SSL *ssl, const DH *dh) {
 }
 
 STACK_OF(SSL_CIPHER) *SSL_CTX_get_ciphers(const SSL_CTX *ctx) {
-  return ctx->cipher_list->ciphers;
+  return ctx->cipher_list->ciphers.get();
 }
 
 int SSL_CTX_cipher_in_group(const SSL_CTX *ctx, size_t i) {
-  if (i >= sk_SSL_CIPHER_num(ctx->cipher_list->ciphers)) {
+  if (i >= sk_SSL_CIPHER_num(ctx->cipher_list->ciphers.get())) {
     return 0;
   }
   return ctx->cipher_list->in_group_flags[i];
@@ -1835,13 +1825,8 @@ STACK_OF(SSL_CIPHER) *SSL_get_ciphers(const SSL *ssl) {
     return NULL;
   }
 
-  const struct ssl_cipher_preference_list_st *prefs =
-      ssl_get_cipher_preferences(ssl);
-  if (prefs == NULL) {
-    return NULL;
-  }
-
-  return prefs->ciphers;
+  const SSLCipherPreferenceList *prefs = ssl_get_cipher_preferences(ssl);
+  return prefs == nullptr ? nullptr : prefs->ciphers.get();
 }
 
 const char *SSL_get_cipher_list(const SSL *ssl, int n) {