Skip to content

Commit

Permalink
Switch outgoing_messages to InplaceVector
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
  • Loading branch information
@davidben
davidben authored and Boringssl LUCI CQ committed Oct 8, 2024
1 parent 87d0c17 commit d0a1756
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 48 deletions.
54 changes: 14 additions & 40 deletions ssl/d1_both.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,13 +483,8 @@ ssl_open_record_t dtls1_open_change_cipher_spec(SSL *ssl, size_t *out_consumed,

// 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;
Expand Down Expand Up @@ -524,20 +519,6 @@ bool dtls1_finish_message(const SSL *ssl, CBB *cbb, Array<uint8_t> *out_msg) {
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) {
Expand All @@ -548,16 +529,6 @@ static bool add_outgoing(SSL *ssl, bool is_ccs, Array<uint8_t> data) {
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.
Expand All @@ -569,13 +540,16 @@ static bool add_outgoing(SSL *ssl, bool is_ccs, Array<uint8_t> data) {
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;
}

Expand Down Expand Up @@ -626,7 +600,7 @@ enum seal_result_t {
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);
Expand Down Expand Up @@ -715,8 +689,8 @@ static bool seal_next_packet(SSL *ssl, uint8_t *out, size_t *out_len,
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];
Expand Down Expand Up @@ -772,7 +746,7 @@ static int send_flight(SSL *ssl) {
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;

Expand Down
27 changes: 19 additions & 8 deletions ssl/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -608,13 +608,30 @@ class InplaceVector {
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)); }
void ResizeMaybeUninit(size_t size) {
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])];
Expand Down Expand Up @@ -1458,12 +1475,6 @@ bool dtls_has_unprocessed_handshake_data(const SSL *ssl);
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;
Expand Down Expand Up @@ -3294,8 +3305,8 @@ struct DTLS1_STATE {

// 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.
Expand Down
8 changes: 8 additions & 0 deletions ssl/ssl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,12 @@ TEST(InplaceVectorTest, ComplexType) {
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) {
Expand All @@ -811,6 +817,8 @@ TEST(InplaceVectorDeathTest, BoundsChecks) {
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) {
Expand Down

0 comments on commit d0a1756

Please sign in to comment.