Use new STACK_OF helpers.

Bug: 132
Change-Id: Ib9bc3ce5f60d0c5bf7922b3d3ccfcd15ef4972a1
Reviewed-on: https://boringssl-review.googlesource.com/18466
Reviewed-by: David Benjamin <davidben@google.com>

ssl/handshake_client.cc

@@ -599,10 +599,8 @@ static int ssl_write_client_cipher_list(SSL_HANDSHAKE *hs, CBB *out) {
   }
 
   if (hs->min_version < TLS1_3_VERSION) {
-    STACK_OF(SSL_CIPHER) *ciphers = SSL_get_ciphers(ssl);
     int any_enabled = 0;
-    for (size_t i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) {
-      const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(ciphers, i);
+    for (const SSL_CIPHER *cipher : SSL_get_ciphers(ssl)) {
       /* Skip disabled ciphers */
       if ((cipher->algorithm_mkey & mask_k) ||
           (cipher->algorithm_auth & mask_a)) {

ssl/handshake_server.cc

@@ -558,16 +558,16 @@ static int negotiate_version(SSL_HANDSHAKE *hs, uint8_t *out_alert,
   return 1;
 }
 
-static STACK_OF(SSL_CIPHER) *
-    ssl_parse_client_cipher_list(const SSL_CLIENT_HELLO *client_hello) {
+static UniquePtr<STACK_OF(SSL_CIPHER)> ssl_parse_client_cipher_list(
+    const SSL_CLIENT_HELLO *client_hello) {
   CBS cipher_suites;
   CBS_init(&cipher_suites, client_hello->cipher_suites,
            client_hello->cipher_suites_len);
 
-  STACK_OF(SSL_CIPHER) *sk = sk_SSL_CIPHER_new_null();
-  if (sk == NULL) {
+  UniquePtr<STACK_OF(SSL_CIPHER)> sk(sk_SSL_CIPHER_new_null());
+  if (!sk) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-    goto err;
+    return nullptr;
   }
 
   while (CBS_len(&cipher_suites) > 0) {
@@ -575,21 +575,17 @@ static STACK_OF(SSL_CIPHER) *
 
     if (!CBS_get_u16(&cipher_suites, &cipher_suite)) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_IN_RECEIVED_CIPHER_LIST);
-      goto err;
+      return nullptr;
     }
 
     const SSL_CIPHER *c = SSL_get_cipher_by_value(cipher_suite);
-    if (c != NULL && !sk_SSL_CIPHER_push(sk, c)) {
+    if (c != NULL && !sk_SSL_CIPHER_push(sk.get(), c)) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-      goto err;
+      return nullptr;
     }
   }
 
   return sk;
-
-err:
-  sk_SSL_CIPHER_free(sk);
-  return NULL;
 }
 
 /* ssl_get_compatible_server_ciphers determines the key exchange and
@@ -640,18 +636,18 @@ static const SSL_CIPHER *ssl3_choose_cipher(
    * such value exists yet. */
   int group_min = -1;
 
-  STACK_OF(SSL_CIPHER) *client_pref =
+  UniquePtr<STACK_OF(SSL_CIPHER)> client_pref =
       ssl_parse_client_cipher_list(client_hello);
-  if (client_pref == NULL) {
-    return NULL;
+  if (!client_pref) {
+    return nullptr;
   }
 
   if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) {
     prio = server_pref->ciphers;
     in_group_flags = server_pref->in_group_flags;
-    allow = client_pref;
+    allow = client_pref.get();
   } else {
-    prio = client_pref;
+    prio = client_pref.get();
     in_group_flags = NULL;
     allow = server_pref->ciphers;
   }
@@ -659,7 +655,6 @@ static const SSL_CIPHER *ssl3_choose_cipher(
   uint32_t mask_k, mask_a;
   ssl_get_compatible_server_ciphers(hs, &mask_k, &mask_a);
 
-  const SSL_CIPHER *ret = NULL;
   for (size_t i = 0; i < sk_SSL_CIPHER_num(prio); i++) {
     const SSL_CIPHER *c = sk_SSL_CIPHER_value(prio, i);
 
@@ -682,21 +677,18 @@ static const SSL_CIPHER *ssl3_choose_cipher(
         if (group_min != -1 && (size_t)group_min < cipher_index) {
           cipher_index = group_min;
         }
-        ret = sk_SSL_CIPHER_value(allow, cipher_index);
-        break;
+        return sk_SSL_CIPHER_value(allow, cipher_index);
       }
     }
 
     if (in_group_flags != NULL && in_group_flags[i] == 0 && group_min != -1) {
       /* We are about to leave a group, but we found a match in it, so that's
        * our answer. */
-      ret = sk_SSL_CIPHER_value(allow, group_min);
-      break;
+      return sk_SSL_CIPHER_value(allow, group_min);
     }
   }
 
-  sk_SSL_CIPHER_free(client_pref);
-  return ret;
+  return nullptr;
 }
 
 static int ssl3_process_client_hello(SSL_HANDSHAKE *hs) {

ssl/internal.h

@@ -2032,7 +2032,7 @@ CERT *ssl_cert_new(const SSL_X509_METHOD *x509_method);
 CERT *ssl_cert_dup(CERT *cert);
 void ssl_cert_clear_certs(CERT *c);
 void ssl_cert_free(CERT *c);
-int ssl_set_cert(CERT *cert, CRYPTO_BUFFER *buffer);
+int ssl_set_cert(CERT *cert, UniquePtr<CRYPTO_BUFFER> buffer);
 int ssl_is_key_type_supported(int key_type);
 /* ssl_compare_public_and_private_key returns one if |pubkey| is the public
  * counterpart to |privkey|. Otherwise it returns zero and pushes a helpful

ssl/ssl_asn1.cc

@@ -92,6 +92,8 @@
 #include <limits.h>
 #include <string.h>
 
+#include <utility>
+
 #include <openssl/buf.h>
 #include <openssl/bytestring.h>
 #include <openssl/err.h>
@@ -678,10 +680,9 @@ UniquePtr<SSL_SESSION> SSL_SESSION_parse(CBS *cbs,
     }
 
     if (has_peer) {
-      CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new_from_CBS(&peer, pool);
-      if (buffer == NULL ||
-          !sk_CRYPTO_BUFFER_push(ret->certs, buffer)) {
-        CRYPTO_BUFFER_free(buffer);
+      UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new_from_CBS(&peer, pool));
+      if (!buffer ||
+          !PushToStack(ret->certs, std::move(buffer))) {
         OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
         return nullptr;
       }

ssl/ssl_cert.cc

@@ -337,8 +337,8 @@ static int cert_set_chain_and_key(
   return 1;
 }
 
-int ssl_set_cert(CERT *cert, CRYPTO_BUFFER *buffer) {
-  switch (check_leaf_cert_and_privkey(buffer, cert->privatekey)) {
+int ssl_set_cert(CERT *cert, UniquePtr<CRYPTO_BUFFER> buffer) {
+  switch (check_leaf_cert_and_privkey(buffer.get(), cert->privatekey)) {
     case leaf_cert_and_privkey_error:
       return 0;
     case leaf_cert_and_privkey_mismatch:
@@ -356,8 +356,7 @@ int ssl_set_cert(CERT *cert, CRYPTO_BUFFER *buffer) {
 
   if (cert->chain != NULL) {
     CRYPTO_BUFFER_free(sk_CRYPTO_BUFFER_value(cert->chain, 0));
-    sk_CRYPTO_BUFFER_set(cert->chain, 0, buffer);
-    CRYPTO_BUFFER_up_ref(buffer);
+    sk_CRYPTO_BUFFER_set(cert->chain, 0, buffer.release());
     return 1;
   }
 
@@ -366,12 +365,11 @@ int ssl_set_cert(CERT *cert, CRYPTO_BUFFER *buffer) {
     return 0;
   }
 
-  if (!sk_CRYPTO_BUFFER_push(cert->chain, buffer)) {
+  if (!PushToStack(cert->chain, std::move(buffer))) {
     sk_CRYPTO_BUFFER_free(cert->chain);
     cert->chain = NULL;
     return 0;
   }
-  CRYPTO_BUFFER_up_ref(buffer);
 
   return 1;
 }
@@ -431,12 +429,11 @@ bool ssl_parse_cert_chain(uint8_t *out_alert,
       }
     }
 
-    CRYPTO_BUFFER *buf =
-        CRYPTO_BUFFER_new_from_CBS(&certificate, pool);
-    if (buf == NULL ||
-        !sk_CRYPTO_BUFFER_push(chain.get(), buf)) {
+    UniquePtr<CRYPTO_BUFFER> buf(
+        CRYPTO_BUFFER_new_from_CBS(&certificate, pool));
+    if (!buf ||
+        !PushToStack(chain.get(), std::move(buf))) {
       *out_alert = SSL_AD_INTERNAL_ERROR;
-      CRYPTO_BUFFER_free(buf);
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
       return false;
     }
@@ -687,11 +684,10 @@ UniquePtr<STACK_OF(CRYPTO_BUFFER)> ssl_parse_client_CA_list(SSL *ssl,
       return nullptr;
     }
 
-    CRYPTO_BUFFER *buffer =
-        CRYPTO_BUFFER_new_from_CBS(&distinguished_name, pool);
-    if (buffer == NULL ||
-        !sk_CRYPTO_BUFFER_push(ret.get(), buffer)) {
-      CRYPTO_BUFFER_free(buffer);
+    UniquePtr<CRYPTO_BUFFER> buffer(
+        CRYPTO_BUFFER_new_from_CBS(&distinguished_name, pool));
+    if (!buffer ||
+        !PushToStack(ret.get(), std::move(buffer))) {
       *out_alert = SSL_AD_INTERNAL_ERROR;
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
       return nullptr;
@@ -811,25 +807,21 @@ int SSL_CTX_set_chain_and_key(SSL_CTX *ctx, CRYPTO_BUFFER *const *certs,
 
 int SSL_CTX_use_certificate_ASN1(SSL_CTX *ctx, size_t der_len,
                                  const uint8_t *der) {
-  CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new(der, der_len, NULL);
-  if (buffer == NULL) {
+  UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(der, der_len, NULL));
+  if (!buffer) {
     return 0;
   }
 
-  const int ok = ssl_set_cert(ctx->cert, buffer);
-  CRYPTO_BUFFER_free(buffer);
-  return ok;
+  return ssl_set_cert(ctx->cert, std::move(buffer));
 }
 
 int SSL_use_certificate_ASN1(SSL *ssl, const uint8_t *der, size_t der_len) {
-  CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new(der, der_len, NULL);
-  if (buffer == NULL) {
+  UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(der, der_len, NULL));
+  if (!buffer) {
     return 0;
   }
 
-  const int ok = ssl_set_cert(ssl->cert, buffer);
-  CRYPTO_BUFFER_free(buffer);
-  return ok;
+  return ssl_set_cert(ssl->cert, std::move(buffer));
 }
 
 void SSL_CTX_set_cert_cb(SSL_CTX *ctx, int (*cb)(SSL *ssl, void *arg),

ssl/ssl_x509.cc

@@ -172,14 +172,14 @@ static void check_ssl_ctx_x509_method(const SSL_CTX *ctx) {
 
 /* x509_to_buffer returns a |CRYPTO_BUFFER| that contains the serialised
  * contents of |x509|. */
-static CRYPTO_BUFFER *x509_to_buffer(X509 *x509) {
+static UniquePtr<CRYPTO_BUFFER> x509_to_buffer(X509 *x509) {
   uint8_t *buf = NULL;
   int cert_len = i2d_X509(x509, &buf);
   if (cert_len <= 0) {
     return 0;
   }
 
-  CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new(buf, cert_len, NULL);
+  UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(buf, cert_len, NULL));
   OPENSSL_free(buf);
 
   return buffer;
@@ -205,17 +205,17 @@ static STACK_OF(CRYPTO_BUFFER) *new_leafless_chain(void) {
  * which case no change to |cert->chain| is made. It preverses the existing
  * leaf from |cert->chain|, if any. */
 static int ssl_cert_set_chain(CERT *cert, STACK_OF(X509) *chain) {
-  STACK_OF(CRYPTO_BUFFER) *new_chain = NULL;
+  UniquePtr<STACK_OF(CRYPTO_BUFFER)> new_chain;
 
   if (cert->chain != NULL) {
-    new_chain = sk_CRYPTO_BUFFER_new_null();
-    if (new_chain == NULL) {
+    new_chain.reset(sk_CRYPTO_BUFFER_new_null());
+    if (!new_chain) {
       return 0;
     }
 
     CRYPTO_BUFFER *leaf = sk_CRYPTO_BUFFER_value(cert->chain, 0);
-    if (!sk_CRYPTO_BUFFER_push(new_chain, leaf)) {
-      goto err;
+    if (!sk_CRYPTO_BUFFER_push(new_chain.get(), leaf)) {
+      return 0;
     }
     /* |leaf| might be NULL if it's a “leafless” chain. */
     if (leaf != NULL) {
@@ -223,30 +223,25 @@ static int ssl_cert_set_chain(CERT *cert, STACK_OF(X509) *chain) {
     }
   }
 
-  for (size_t i = 0; i < sk_X509_num(chain); i++) {
-    if (new_chain == NULL) {
-      new_chain = new_leafless_chain();
-      if (new_chain == NULL) {
-        goto err;
+  for (X509 *x509 : chain) {
+    if (!new_chain) {
+      new_chain.reset(new_leafless_chain());
+      if (!new_chain) {
+        return 0;
       }
     }
 
-    CRYPTO_BUFFER *buffer = x509_to_buffer(sk_X509_value(chain, i));
-    if (buffer == NULL ||
-        !sk_CRYPTO_BUFFER_push(new_chain, buffer)) {
-      CRYPTO_BUFFER_free(buffer);
-      goto err;
+    UniquePtr<CRYPTO_BUFFER> buffer = x509_to_buffer(x509);
+    if (!buffer ||
+        !PushToStack(new_chain.get(), std::move(buffer))) {
+      return 0;
     }
   }
 
   sk_CRYPTO_BUFFER_pop_free(cert->chain, CRYPTO_BUFFER_free);
-  cert->chain = new_chain;
+  cert->chain = new_chain.release();
 
   return 1;
-
-err:
-  sk_CRYPTO_BUFFER_pop_free(new_chain, CRYPTO_BUFFER_free);
-  return 0;
 }
 
 static void ssl_crypto_x509_cert_flush_cached_leaf(CERT *cert) {
@@ -261,8 +256,7 @@ static void ssl_crypto_x509_cert_flush_cached_chain(CERT *cert) {
 
 static int ssl_crypto_x509_check_client_CA_list(
     STACK_OF(CRYPTO_BUFFER) *names) {
-  for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(names); i++) {
-    const CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(names, i);
+  for (const CRYPTO_BUFFER *buffer : names) {
     const uint8_t *inp = CRYPTO_BUFFER_data(buffer);
     X509_NAME *name = d2i_X509_NAME(NULL, &inp, CRYPTO_BUFFER_len(buffer));
     const int ok = name != NULL && inp == CRYPTO_BUFFER_data(buffer) +
@@ -298,8 +292,7 @@ static void ssl_crypto_x509_cert_dup(CERT *new_cert, const CERT *cert) {
 
 static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) {
   bssl::UniquePtr<STACK_OF(X509)> chain;
-  const size_t num_certs = sk_CRYPTO_BUFFER_num(sess->certs);
-  if (num_certs > 0) {
+  if (sk_CRYPTO_BUFFER_num(sess->certs) > 0) {
     chain.reset(sk_X509_new_null());
     if (!chain) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
@@ -307,21 +300,20 @@ static int ssl_crypto_x509_session_cache_objects(SSL_SESSION *sess) {
     }
   }
 
-  X509 *leaf = NULL;
-  for (size_t i = 0; i < num_certs; i++) {
-    X509 *x509 = X509_parse_from_buffer(sk_CRYPTO_BUFFER_value(sess->certs, i));
-    if (x509 == NULL) {
+  X509 *leaf = nullptr;
+  for (CRYPTO_BUFFER *cert : sess->certs) {
+    UniquePtr<X509> x509(X509_parse_from_buffer(cert));
+    if (!x509) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
       return 0;
     }
-    if (!sk_X509_push(chain.get(), x509)) {
+    if (leaf == nullptr) {
+      leaf = x509.get();
+    }
+    if (!PushToStack(chain.get(), std::move(x509))) {
       OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-      X509_free(x509);
       return 0;
     }
-    if (i == 0) {
-      leaf = x509;
-    }
   }
 
   sk_X509_pop_free(sess->x509_chain, X509_free);
@@ -797,14 +789,12 @@ static int ssl_use_certificate(CERT *cert, X509 *x) {
     return 0;
   }
 
-  CRYPTO_BUFFER *buffer = x509_to_buffer(x);
-  if (buffer == NULL) {
+  UniquePtr<CRYPTO_BUFFER> buffer = x509_to_buffer(x);
+  if (!buffer) {
     return 0;
   }
 
-  const int ok = ssl_set_cert(cert, buffer);
-  CRYPTO_BUFFER_free(buffer);
-  return ok;
+  return ssl_set_cert(cert, std::move(buffer));
 }
 
 int SSL_use_certificate(SSL *ssl, X509 *x) {
@@ -880,24 +870,18 @@ static int ssl_cert_set1_chain(CERT *cert, STACK_OF(X509) *chain) {
 static int ssl_cert_append_cert(CERT *cert, X509 *x509) {
   assert(cert->x509_method);
 
-  CRYPTO_BUFFER *buffer = x509_to_buffer(x509);
-  if (buffer == NULL) {
+  UniquePtr<CRYPTO_BUFFER> buffer = x509_to_buffer(x509);
+  if (!buffer) {
     return 0;
   }
 
   if (cert->chain != NULL) {
-    if (!sk_CRYPTO_BUFFER_push(cert->chain, buffer)) {
-      CRYPTO_BUFFER_free(buffer);
-      return 0;
-    }
-
-    return 1;
+    return PushToStack(cert->chain, std::move(buffer));
   }
 
   cert->chain = new_leafless_chain();
   if (cert->chain == NULL ||
-      !sk_CRYPTO_BUFFER_push(cert->chain, buffer)) {
-    CRYPTO_BUFFER_free(buffer);
+      !PushToStack(cert->chain, std::move(buffer))) {
     sk_CRYPTO_BUFFER_free(cert->chain);
     cert->chain = NULL;
     return 0;
@@ -997,27 +981,22 @@ static int ssl_cert_cache_chain_certs(CERT *cert) {
     return 1;
   }
 
-  STACK_OF(X509) *chain = sk_X509_new_null();
-  if (chain == NULL) {
+  UniquePtr<STACK_OF(X509)> chain(sk_X509_new_null());
+  if (!chain) {
     return 0;
   }
 
   for (size_t i = 1; i < sk_CRYPTO_BUFFER_num(cert->chain); i++) {
     CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(cert->chain, i);
-    X509 *x509 = X509_parse_from_buffer(buffer);
-    if (x509 == NULL ||
-        !sk_X509_push(chain, x509)) {
-      X509_free(x509);
-      goto err;
+    UniquePtr<X509> x509(X509_parse_from_buffer(buffer));
+    if (!x509 ||
+        !PushToStack(chain.get(), std::move(x509))) {
+      return 0;
     }
   }
 
-  cert->x509_chain = chain;
+  cert->x509_chain = chain.release();
   return 1;
-
-err:
-  sk_X509_pop_free(chain, X509_free);
-  return 0;
 }
 
 int SSL_CTX_get0_chain_certs(const SSL_CTX *ctx, STACK_OF(X509) **out_chain) {
@@ -1096,34 +1075,28 @@ STACK_OF(X509_NAME) *SSL_dup_CA_list(STACK_OF(X509_NAME) *list) {
 static void set_client_CA_list(STACK_OF(CRYPTO_BUFFER) **ca_list,
                                const STACK_OF(X509_NAME) *name_list,
                                CRYPTO_BUFFER_POOL *pool) {
-  STACK_OF(CRYPTO_BUFFER) *buffers = sk_CRYPTO_BUFFER_new_null();
-  if (buffers == NULL) {
+  UniquePtr<STACK_OF(CRYPTO_BUFFER)> buffers(sk_CRYPTO_BUFFER_new_null());
+  if (!buffers) {
     return;
   }
 
-  for (size_t i = 0; i < sk_X509_NAME_num(name_list); i++) {
-    X509_NAME *name = sk_X509_NAME_value(name_list, i);
+  for (X509_NAME *name : name_list) {
     uint8_t *outp = NULL;
     int len = i2d_X509_NAME(name, &outp);
     if (len < 0) {
-      goto err;
+      return;
     }
 
-    CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new(outp, len, pool);
+    UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(outp, len, pool));
     OPENSSL_free(outp);
-    if (buffer == NULL ||
-        !sk_CRYPTO_BUFFER_push(buffers, buffer)) {
-      CRYPTO_BUFFER_free(buffer);
-      goto err;
+    if (!buffer ||
+        !PushToStack(buffers.get(), std::move(buffer))) {
+      return;
     }
   }
 
   sk_CRYPTO_BUFFER_pop_free(*ca_list, CRYPTO_BUFFER_free);
-  *ca_list = buffers;
-  return;
-
-err:
-  sk_CRYPTO_BUFFER_pop_free(buffers, CRYPTO_BUFFER_free);
+  *ca_list = buffers.release();
 }
 
 void SSL_set_client_CA_list(SSL *ssl, STACK_OF(X509_NAME) *name_list) {
@@ -1151,30 +1124,25 @@ static STACK_OF(X509_NAME) *
     return *cached;
   }
 
-  STACK_OF(X509_NAME) *new_cache = sk_X509_NAME_new_null();
-  if (new_cache == NULL) {
+  UniquePtr<STACK_OF(X509_NAME)> new_cache(sk_X509_NAME_new_null());
+  if (!new_cache) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
     return NULL;
   }
 
-  for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(names); i++) {
-    const CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(names, i);
+  for (const CRYPTO_BUFFER *buffer : names) {
     const uint8_t *inp = CRYPTO_BUFFER_data(buffer);
-    X509_NAME *name = d2i_X509_NAME(NULL, &inp, CRYPTO_BUFFER_len(buffer));
-    if (name == NULL ||
+    UniquePtr<X509_NAME> name(
+        d2i_X509_NAME(nullptr, &inp, CRYPTO_BUFFER_len(buffer)));
+    if (!name ||
         inp != CRYPTO_BUFFER_data(buffer) + CRYPTO_BUFFER_len(buffer) ||
-        !sk_X509_NAME_push(new_cache, name)) {
-      X509_NAME_free(name);
-      goto err;
+        !PushToStack(new_cache.get(), std::move(name))) {
+      return NULL;
     }
   }
 
-  *cached = new_cache;
-  return new_cache;
-
-err:
-  sk_X509_NAME_pop_free(new_cache, X509_NAME_free);
-  return NULL;
+  *cached = new_cache.release();
+  return *cached;
 }
 
 STACK_OF(X509_NAME) *SSL_get_client_CA_list(const SSL *ssl) {
@@ -1222,9 +1190,9 @@ static int add_client_CA(STACK_OF(CRYPTO_BUFFER) **names, X509 *x509,
     return 0;
   }
 
-  CRYPTO_BUFFER *buffer = CRYPTO_BUFFER_new(outp, len, pool);
+  UniquePtr<CRYPTO_BUFFER> buffer(CRYPTO_BUFFER_new(outp, len, pool));
   OPENSSL_free(outp);
-  if (buffer == NULL) {
+  if (!buffer) {
     return 0;
   }
 
@@ -1234,13 +1202,11 @@ static int add_client_CA(STACK_OF(CRYPTO_BUFFER) **names, X509 *x509,
     alloced = 1;
 
     if (*names == NULL) {
-      CRYPTO_BUFFER_free(buffer);
       return 0;
     }
   }
 
-  if (!sk_CRYPTO_BUFFER_push(*names, buffer)) {
-    CRYPTO_BUFFER_free(buffer);
+  if (!PushToStack(*names, std::move(buffer))) {
     if (alloced) {
       sk_CRYPTO_BUFFER_pop_free(*names, CRYPTO_BUFFER_free);
       *names = NULL;

ssl/t1_lib.cc

@@ -1624,11 +1624,8 @@ static void ext_srtp_init(SSL_HANDSHAKE *hs) {
 static int ext_srtp_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
   SSL *const ssl = hs->ssl;
   STACK_OF(SRTP_PROTECTION_PROFILE) *profiles = SSL_get_srtp_profiles(ssl);
-  if (profiles == NULL) {
-    return 1;
-  }
-  const size_t num_profiles = sk_SRTP_PROTECTION_PROFILE_num(profiles);
-  if (num_profiles == 0) {
+  if (profiles == NULL ||
+      sk_SRTP_PROTECTION_PROFILE_num(profiles) == 0) {
     return 1;
   }
 
@@ -1639,9 +1636,8 @@ static int ext_srtp_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
     return 0;
   }
 
-  for (size_t i = 0; i < num_profiles; i++) {
-    if (!CBB_add_u16(&profile_ids,
-                     sk_SRTP_PROTECTION_PROFILE_value(profiles, i)->id)) {
+  for (const SRTP_PROTECTION_PROFILE *profile : profiles) {
+    if (!CBB_add_u16(&profile_ids, profile->id)) {
       return 0;
     }
   }
@@ -1687,10 +1683,7 @@ static int ext_srtp_parse_serverhello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
 
   /* Check to see if the server gave us something we support (and presumably
    * offered). */
-  for (size_t i = 0; i < sk_SRTP_PROTECTION_PROFILE_num(profiles); i++) {
-    const SRTP_PROTECTION_PROFILE *profile =
-        sk_SRTP_PROTECTION_PROFILE_value(profiles, i);
-
+  for (const SRTP_PROTECTION_PROFILE *profile : profiles) {
     if (profile->id == profile_id) {
       ssl->srtp_profile = profile;
       return 1;
@@ -1723,10 +1716,7 @@ static int ext_srtp_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert,
       SSL_get_srtp_profiles(ssl);
 
   /* Pick the server's most preferred profile. */
-  for (size_t i = 0; i < sk_SRTP_PROTECTION_PROFILE_num(server_profiles); i++) {
-    const SRTP_PROTECTION_PROFILE *server_profile =
-        sk_SRTP_PROTECTION_PROFILE_value(server_profiles, i);
-
+  for (const SRTP_PROTECTION_PROFILE *server_profile : server_profiles) {
     CBS profile_ids_tmp;
     CBS_init(&profile_ids_tmp, CBS_data(&profile_ids), CBS_len(&profile_ids));