From ec783839be5830d8893e60566909ef5c9fb25334 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Tue, 25 Jul 2017 23:23:03 -0400
Subject: [PATCH] Make ranged for loops work with STACK_OF(T).

My original plan here was to make STACK_OF(T) expand to a template so
the inner type were extractable. Unfortunately, we cannot sanely make
STACK_OF(T) expand to a different type in C and C++ even across
compilation units because UBSan sometimes explodes. This is nuts, but so
it goes.

Instead, use StackTraits to extract the STACK_OF(T) parameters and
define an iterator type.

Bug: 189
Change-Id: I64f5173b34b723ec471f7a355ff46b04f161386a
Reviewed-on: https://boringssl-review.googlesource.com/18467
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
---
 include/openssl/stack.h | 85 +++++++++++++++++++++++++++++++++--------
 ssl/ssl_cert.cc         |  4 +-
 2 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/include/openssl/stack.h b/include/openssl/stack.h
index 8f15ea2b11..0e59d6a550 100644
--- a/include/openssl/stack.h
+++ b/include/openssl/stack.h
@@ -222,21 +222,22 @@ struct StackTraits {};
 }
 }
 
-#define BORINGSSL_DEFINE_STACK_TRAITS(name, is_const) \
-  extern "C++" {                                      \
-  namespace bssl {                                    \
-  namespace internal {                                \
-  template <>                                         \
-  struct StackTraits<STACK_OF(name)> {                \
-    using Type = name;                                \
-    static constexpr bool kIsConst = is_const;        \
-  };                                                  \
-  }                                                   \
-  }                                                   \
+#define BORINGSSL_DEFINE_STACK_TRAITS(name, type, is_const) \
+  extern "C++" {                                            \
+  namespace bssl {                                          \
+  namespace internal {                                      \
+  template <>                                               \
+  struct StackTraits<STACK_OF(name)> {                      \
+    static constexpr bool kIsStack = true;                  \
+    using Type = type;                                      \
+    static constexpr bool kIsConst = is_const;              \
+  };                                                        \
+  }                                                         \
+  }                                                         \
   }
 
 #else
-#define BORINGSSL_DEFINE_STACK_TRAITS(name, is_const)
+#define BORINGSSL_DEFINE_STACK_TRAITS(name, type, is_const)
 #endif
 
 /* Stack functions must be tagged unused to support file-local stack types.
@@ -350,15 +351,15 @@ struct StackTraits {};
 
 /* DEFINE_STACK_OF defines |STACK_OF(type)| to be a stack whose elements are
  * |type| *. */
-#define DEFINE_STACK_OF(type) \
+#define DEFINE_STACK_OF(type)                                \
   BORINGSSL_DEFINE_STACK_OF_IMPL(type, type *, const type *) \
-  BORINGSSL_DEFINE_STACK_TRAITS(type, false)
+  BORINGSSL_DEFINE_STACK_TRAITS(type, type, false)
 
 /* DEFINE_CONST_STACK_OF defines |STACK_OF(type)| to be a stack whose elements
  * are const |type| *. */
-#define DEFINE_CONST_STACK_OF(type) \
+#define DEFINE_CONST_STACK_OF(type)                                \
   BORINGSSL_DEFINE_STACK_OF_IMPL(type, const type *, const type *) \
-  BORINGSSL_DEFINE_STACK_TRAITS(type, true)
+  BORINGSSL_DEFINE_STACK_TRAITS(type, const type, true)
 
 /* DEFINE_SPECIAL_STACK_OF defines |STACK_OF(type)| to be a stack whose elements
  * are |type|, where |type| must be a typedef for a pointer. */
@@ -407,10 +408,62 @@ struct DeleterImpl<
   }
 };
 
+template <typename Stack>
+class StackIteratorImpl {
+ public:
+  using Type = typename StackTraits<Stack>::Type;
+  // Iterators must be default-constructable.
+  StackIteratorImpl() : sk_(nullptr), idx_(0) {}
+  StackIteratorImpl(const Stack *sk, size_t idx) : sk_(sk), idx_(idx) {}
+
+  bool operator==(StackIteratorImpl other) const {
+    return sk_ == other.sk_ && idx_ == other.idx_;
+  }
+  bool operator!=(StackIteratorImpl other) const {
+    return !(*this == other);
+  }
+
+  Type *operator*() const {
+    return reinterpret_cast<Type *>(
+        sk_value(reinterpret_cast<const _STACK *>(sk_), idx_));
+  }
+
+  StackIteratorImpl &operator++(/* prefix */) {
+    idx_++;
+    return *this;
+  }
+
+  StackIteratorImpl operator++(int /* postfix */) {
+    StackIteratorImpl copy(*this);
+    ++(*this);
+    return copy;
+  }
+
+ private:
+  const Stack *sk_;
+  size_t idx_;
+};
+
+template <typename Stack>
+using StackIterator = typename std::enable_if<StackTraits<Stack>::kIsStack,
+                                              StackIteratorImpl<Stack>>::type;
+
 }  // namespace internal
 
 }  // namespace bssl
 
+// Define begin() and end() for stack types so C++ range for loops work.
+template <typename Stack>
+static inline bssl::internal::StackIterator<Stack> begin(const Stack *sk) {
+  return bssl::internal::StackIterator<Stack>(sk, 0);
+}
+
+template <typename Stack>
+static inline bssl::internal::StackIterator<Stack> end(const Stack *sk) {
+  return bssl::internal::StackIterator<Stack>(
+      sk, sk_num(reinterpret_cast<const _STACK *>(sk)));
+}
+
 }  // extern C++
 #endif
 
diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc
index 0fcf48d499..4337ec03e8 100644
--- a/ssl/ssl_cert.cc
+++ b/ssl/ssl_cert.cc
@@ -721,9 +721,7 @@ int ssl_add_client_CA_list(SSL *ssl, CBB *cbb) {
     return CBB_flush(cbb);
   }
 
-  for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(names); i++) {
-    const CRYPTO_BUFFER *name = sk_CRYPTO_BUFFER_value(names, i);
-
+  for (const CRYPTO_BUFFER *name : names) {
     if (!CBB_add_u16_length_prefixed(&child, &name_cbb) ||
         !CBB_add_bytes(&name_cbb, CRYPTO_BUFFER_data(name),
                        CRYPTO_BUFFER_len(name))) {