Switch outgoing_messages to InplaceVector
This required adding a PushBack method. I removed the check for 32-bit
size_t. That dates to https://boringssl-review.googlesource.com/20671
when the input object had a 64-bit size_t, but we only stored a uint32_t
size in memory. We now just store an Array without any fuss (the
abstraction was worth more than the memory packing), so this is all
moot.
(It will also never get that large because CBB will refuse to construct
it.)
Change-Id: Idf4ad7e47a81b8086ef0fed2083308aaf0efb668
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/71848
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index ac47189..016e3a2 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -483,13 +483,8 @@
// Sending handshake messages.
-void DTLS_OUTGOING_MESSAGE::Clear() { data.Reset(); }
-
void dtls_clear_outgoing_messages(SSL *ssl) {
- for (size_t i = 0; i < ssl->d1->outgoing_messages_len; i++) {
- ssl->d1->outgoing_messages[i].Clear();
- }
- ssl->d1->outgoing_messages_len = 0;
+ ssl->d1->outgoing_messages.clear();
ssl->d1->outgoing_written = 0;
ssl->d1->outgoing_offset = 0;
ssl->d1->outgoing_messages_complete = false;
@@ -524,20 +519,6 @@
return true;
}
-// ssl_size_t_greater_than_32_bits returns whether |v| exceeds the bounds of a
-// 32-bit value. The obvious thing doesn't work because, in some 32-bit build
-// configurations, the compiler warns that the test is always false and breaks
-// the build.
-static bool ssl_size_t_greater_than_32_bits(size_t v) {
-#if defined(OPENSSL_64_BIT)
- return v > 0xffffffff;
-#elif defined(OPENSSL_32_BIT)
- return false;
-#else
-#error "Building for neither 32- nor 64-bits."
-#endif
-}
-
// add_outgoing adds a new handshake message or ChangeCipherSpec to the current
// outgoing flight. It returns true on success and false on error.
static bool add_outgoing(SSL *ssl, bool is_ccs, Array<uint8_t> data) {
@@ -548,16 +529,6 @@
dtls_clear_outgoing_messages(ssl);
}
- static_assert(SSL_MAX_HANDSHAKE_FLIGHT <
- (1 << 8 * sizeof(ssl->d1->outgoing_messages_len)),
- "outgoing_messages_len is too small");
- if (ssl->d1->outgoing_messages_len >= SSL_MAX_HANDSHAKE_FLIGHT ||
- ssl_size_t_greater_than_32_bits(data.size())) {
- assert(false);
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return false;
- }
-
if (!is_ccs) {
// TODO(svaldez): Move this up a layer to fix abstraction for SSLTranscript
// on hs.
@@ -569,13 +540,16 @@
ssl->d1->handshake_write_seq++;
}
- DTLS_OUTGOING_MESSAGE *msg =
- &ssl->d1->outgoing_messages[ssl->d1->outgoing_messages_len];
- msg->data = std::move(data);
- msg->epoch = ssl->d1->w_epoch;
- msg->is_ccs = is_ccs;
+ DTLS_OUTGOING_MESSAGE msg;
+ msg.data = std::move(data);
+ msg.epoch = ssl->d1->w_epoch;
+ msg.is_ccs = is_ccs;
+ if (!ssl->d1->outgoing_messages.TryPushBack(std::move(msg))) {
+ assert(false);
+ OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ return false;
+ }
- ssl->d1->outgoing_messages_len++;
return true;
}
@@ -626,7 +600,7 @@
static enum seal_result_t seal_next_message(SSL *ssl, uint8_t *out,
size_t *out_len, size_t max_out,
const DTLS_OUTGOING_MESSAGE *msg) {
- assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages_len);
+ assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size());
assert(msg == &ssl->d1->outgoing_messages[ssl->d1->outgoing_written]);
size_t overhead = dtls_max_seal_overhead(ssl, msg->epoch);
@@ -715,8 +689,8 @@
size_t max_out) {
bool made_progress = false;
size_t total = 0;
- assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages_len);
- for (; ssl->d1->outgoing_written < ssl->d1->outgoing_messages_len;
+ assert(ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size());
+ for (; ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size();
ssl->d1->outgoing_written++) {
const DTLS_OUTGOING_MESSAGE *msg =
&ssl->d1->outgoing_messages[ssl->d1->outgoing_written];
@@ -772,7 +746,7 @@
return -1;
}
- while (ssl->d1->outgoing_written < ssl->d1->outgoing_messages_len) {
+ while (ssl->d1->outgoing_written < ssl->d1->outgoing_messages.size()) {
uint8_t old_written = ssl->d1->outgoing_written;
uint32_t old_offset = ssl->d1->outgoing_offset;
diff --git a/ssl/internal.h b/ssl/internal.h
index e6e69c7..5e5d3fd 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -608,6 +608,18 @@
return true;
}
+ // TryPushBack appends |val| to the vector and returns a pointer to the
+ // newly-inserted value, or nullptr if the vector is at capacity.
+ T *TryPushBack(T val) {
+ if (size() >= capacity()) {
+ return nullptr;
+ }
+ T *ret = &data()[size_];
+ new (ret) T(std::move(val));
+ size_++;
+ return ret;
+ }
+
// The following methods behave like their |Try*| counterparts, but abort the
// program on failure.
void Resize(size_t size) { BSSL_CHECK(TryResize(size)); }
@@ -615,6 +627,11 @@
BSSL_CHECK(TryResizeMaybeUninit(size));
}
void CopyFrom(Span<const T> in) { BSSL_CHECK(TryCopyFrom(in)); }
+ T &PushBack(T val) {
+ T *ret = TryPushBack(std::move(val));
+ BSSL_CHECK(ret != nullptr);
+ return *ret;
+ }
private:
alignas(T) char storage_[sizeof(T[N])];
@@ -1458,12 +1475,6 @@
bool tls_flush_pending_hs_data(SSL *ssl);
struct DTLS_OUTGOING_MESSAGE {
- DTLS_OUTGOING_MESSAGE() {}
- DTLS_OUTGOING_MESSAGE(const DTLS_OUTGOING_MESSAGE &) = delete;
- DTLS_OUTGOING_MESSAGE &operator=(const DTLS_OUTGOING_MESSAGE &) = delete;
-
- void Clear();
-
Array<uint8_t> data;
uint16_t epoch = 0;
bool is_ccs = false;
@@ -3294,8 +3305,8 @@
// outgoing_messages is the queue of outgoing messages from the last handshake
// flight.
- DTLS_OUTGOING_MESSAGE outgoing_messages[SSL_MAX_HANDSHAKE_FLIGHT];
- uint8_t outgoing_messages_len = 0;
+ InplaceVector<DTLS_OUTGOING_MESSAGE, SSL_MAX_HANDSHAKE_FLIGHT>
+ outgoing_messages;
// outgoing_written is the number of outgoing messages that have been
// written.
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 46445bb..4d74bfa 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -794,6 +794,12 @@
for (const auto &vec : vec_of_vecs4) {
EXPECT_TRUE(vec.empty());
}
+
+ std::vector<int> v = {42};
+ vec_of_vecs5.Resize(3);
+ EXPECT_TRUE(vec_of_vecs5.TryPushBack(v));
+ EXPECT_EQ(v, vec_of_vecs5[3]);
+ EXPECT_FALSE(vec_of_vecs5.TryPushBack(v));
}
TEST(InplaceVectorDeathTest, BoundsChecks) {
@@ -811,6 +817,8 @@
EXPECT_DEATH_IF_SUPPORTED(vec.ResizeMaybeUninit(5), "");
int too_much_data[] = {1, 2, 3, 4, 5};
EXPECT_DEATH_IF_SUPPORTED(vec.CopyFrom(too_much_data), "");
+ vec.Resize(4);
+ EXPECT_DEATH_IF_SUPPORTED(vec.PushBack(42), "");
}
TEST(ReconstructSeqnumTest, Increment) {
