Standardize on Init vs InitForOverwrite for value vs default initialization
C++ is fun and has two notions of "default" initialization, `new T` and
`new T()`. These are default initialization and value initialization,
respectively.
They are identical except that POD types are uninit when
default-initialized and zero when value-initialized. InplaceVector
picked the safer option by default and called the other one
FooMaybeUninit. Array is older and uses the less safe one (it's almost
always the one we want; we usually allocate an array to immediately fill
it).
While MaybeUninit does capture what you do with it, it is slightly
ambiguous, as seen in Array's internal implementation: uninitialized
could also mean we haven't gotten around to initialize it at all. I.e.
we need to use a function like std::uninitialized_value_construct_n
instead of normal functions in <algorithm>.
C++20 has std::make_unique and std::make_unique_for_overwrite to capture
the two. This seems as fine a naming convention as any, so switch to it.
Along the way, make the internal bssl::Array default to the safer one.
This lets us remove a couple of memset(0)'s.
Change-Id: I32cede231da051a854e6251e10b87f8e4dd06ee6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/72268
Reviewed-by: Nick Harper <nharper@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc
index f8ac34a..e0c2614 100644
--- a/ssl/d1_both.cc
+++ b/ssl/d1_both.cc
@@ -785,7 +785,7 @@
dtls1_update_mtu(ssl);
Array<uint8_t> packet;
- if (!packet.Init(ssl->d1->mtu)) {
+ if (!packet.InitForOverwrite(ssl->d1->mtu)) {
return -1;
}
diff --git a/ssl/encrypted_client_hello.cc b/ssl/encrypted_client_hello.cc
index 8c4a42c..f9686fe 100644
--- a/ssl/encrypted_client_hello.cc
+++ b/ssl/encrypted_client_hello.cc
@@ -303,7 +303,7 @@
return false;
}
#else
- if (!encoded.Init(payload.size())) {
+ if (!encoded.InitForOverwrite(payload.size())) {
*out_alert = SSL_AD_INTERNAL_ERROR;
return false;
}
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index c9424c9..ad4bf81 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -175,7 +175,7 @@
}
Array<uint16_t> extension_types;
- if (!extension_types.Init(num_extensions)) {
+ if (!extension_types.InitForOverwrite(num_extensions)) {
return false;
}
@@ -2526,7 +2526,7 @@
}
Array<uint16_t> ret;
- if (!ret.Init(CBS_len(©) / 2)) {
+ if (!ret.InitForOverwrite(CBS_len(©) / 2)) {
return false;
}
for (size_t i = 0; i < ret.size(); i++) {
@@ -2878,7 +2878,7 @@
const size_t num_given_alg_ids = CBS_len(&alg_ids) / 2;
Array<uint16_t> given_alg_ids;
- if (!given_alg_ids.Init(num_given_alg_ids)) {
+ if (!given_alg_ids.InitForOverwrite(num_given_alg_ids)) {
return false;
}
@@ -3352,7 +3352,7 @@
uint32_t seeds[kNumExtensions - 1];
Array<uint8_t> permutation;
if (!RAND_bytes(reinterpret_cast<uint8_t *>(seeds), sizeof(seeds)) ||
- !permutation.Init(kNumExtensions)) {
+ !permutation.InitForOverwrite(kNumExtensions)) {
return false;
}
for (size_t i = 0; i < kNumExtensions; i++) {
@@ -3918,7 +3918,7 @@
if (ciphertext.size() >= INT_MAX) {
return ssl_ticket_aead_ignore_ticket;
}
- if (!plaintext.Init(ciphertext.size())) {
+ if (!plaintext.InitForOverwrite(ciphertext.size())) {
return ssl_ticket_aead_error;
}
int len1, len2;
@@ -4006,7 +4006,7 @@
SSL_HANDSHAKE *hs, Array<uint8_t> *out, bool *out_renew_ticket,
Span<const uint8_t> ticket) {
Array<uint8_t> plaintext;
- if (!plaintext.Init(ticket.size())) {
+ if (!plaintext.InitForOverwrite(ticket.size())) {
return ssl_ticket_aead_error;
}
@@ -4115,7 +4115,7 @@
// Envoy's tests expect the session to have a session ID that matches the
// placeholder used by the client. It's unclear whether this is a good idea,
// but we maintain it for now.
- session->session_id.ResizeMaybeUninit(SHA256_DIGEST_LENGTH);
+ session->session_id.ResizeForOverwrite(SHA256_DIGEST_LENGTH);
SHA256(ticket.data(), ticket.size(), session->session_id.data());
*out_session = std::move(session);
@@ -4356,7 +4356,7 @@
}
size_t digest_len;
- hs->new_session->original_handshake_hash.ResizeMaybeUninit(
+ hs->new_session->original_handshake_hash.ResizeForOverwrite(
hs->transcript.DigestLen());
if (!hs->transcript.GetHash(hs->new_session->original_handshake_hash.data(),
&digest_len)) {
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index 6f5c7e2..d317269 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -176,7 +176,7 @@
return false;
}
Array<uint16_t> supported_groups;
- if (!supported_groups.Init(CBS_len(&groups) / 2)) {
+ if (!supported_groups.InitForOverwrite(CBS_len(&groups) / 2)) {
return false;
}
size_t idx = 0;
@@ -190,7 +190,7 @@
Span<const uint16_t> configured_groups =
tls1_get_grouplist(ssl->s3->hs.get());
Array<uint16_t> new_configured_groups;
- if (!new_configured_groups.Init(configured_groups.size())) {
+ if (!new_configured_groups.InitForOverwrite(configured_groups.size())) {
return false;
}
idx = 0;
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index f87e000..031ed81 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -542,7 +542,7 @@
if (has_id_session) {
hs->session_id = ssl->session->session_id;
} else if (ticket_session_requires_random_id || enable_compatibility_mode) {
- hs->session_id.ResizeMaybeUninit(SSL_MAX_SSL_SESSION_ID_LENGTH);
+ hs->session_id.ResizeForOverwrite(SSL_MAX_SSL_SESSION_ID_LENGTH);
if (!RAND_bytes(hs->session_id.data(), hs->session_id.size())) {
return ssl_hs_error;
}
@@ -1528,16 +1528,15 @@
// Depending on the key exchange method, compute |pms|.
if (alg_k & SSL_kRSA) {
- if (!pms.Init(SSL_MAX_MASTER_KEY_LENGTH)) {
- return ssl_hs_error;
- }
-
RSA *rsa = EVP_PKEY_get0_RSA(hs->peer_pubkey.get());
if (rsa == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
return ssl_hs_error;
}
+ if (!pms.InitForOverwrite(SSL_MAX_MASTER_KEY_LENGTH)) {
+ return ssl_hs_error;
+ }
pms[0] = hs->client_version >> 8;
pms[1] = hs->client_version & 0xff;
if (!RAND_bytes(&pms[2], SSL_MAX_MASTER_KEY_LENGTH - 2)) {
@@ -1581,7 +1580,6 @@
if (!pms.Init(psk_len)) {
return ssl_hs_error;
}
- OPENSSL_memset(pms.data(), 0, pms.size());
} else {
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
@@ -1609,7 +1607,7 @@
return ssl_hs_error;
}
- hs->new_session->secret.ResizeMaybeUninit(SSL3_MASTER_SECRET_SIZE);
+ hs->new_session->secret.ResizeForOverwrite(SSL3_MASTER_SECRET_SIZE);
if (!tls1_generate_master_secret(hs, MakeSpan(hs->new_session->secret),
pms)) {
return ssl_hs_error;
@@ -1850,7 +1848,7 @@
// Historically, OpenSSL filled in fake session IDs for ticket-based sessions.
// TODO(davidben): Are external callers relying on this? Try removing this.
- hs->new_session->session_id.ResizeMaybeUninit(SHA256_DIGEST_LENGTH);
+ hs->new_session->session_id.ResizeForOverwrite(SHA256_DIGEST_LENGTH);
SHA256(CBS_data(&ticket), CBS_len(&ticket),
hs->new_session->session_id.data());
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 0eb037b..1b292a7 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -941,7 +941,7 @@
// Assign a session ID if not using session tickets.
if (!hs->ticket_expected &&
(ssl->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)) {
- hs->new_session->session_id.ResizeMaybeUninit(SSL3_SSL_SESSION_ID_LENGTH);
+ hs->new_session->session_id.ResizeForOverwrite(SSL3_SSL_SESSION_ID_LENGTH);
RAND_bytes(hs->new_session->session_id.data(),
hs->new_session->session_id.size());
}
@@ -1464,7 +1464,8 @@
// Allocate a buffer large enough for an RSA decryption.
Array<uint8_t> decrypt_buf;
- if (!decrypt_buf.Init(EVP_PKEY_size(hs->credential->pubkey.get()))) {
+ if (!decrypt_buf.InitForOverwrite(
+ EVP_PKEY_size(hs->credential->pubkey.get()))) {
return ssl_hs_error;
}
@@ -1492,7 +1493,7 @@
// Prepare a random premaster, to be used on invalid padding. See RFC 5246,
// section 7.4.7.1.
- if (!premaster_secret.Init(SSL_MAX_MASTER_KEY_LENGTH) ||
+ if (!premaster_secret.InitForOverwrite(SSL_MAX_MASTER_KEY_LENGTH) ||
!RAND_bytes(premaster_secret.data(), premaster_secret.size())) {
return ssl_hs_error;
}
@@ -1583,7 +1584,6 @@
if (!premaster_secret.Init(psk_len)) {
return ssl_hs_error;
}
- OPENSSL_memset(premaster_secret.data(), 0, premaster_secret.size());
}
ScopedCBB new_premaster;
@@ -1605,7 +1605,7 @@
}
// Compute the master secret.
- hs->new_session->secret.ResizeMaybeUninit(SSL3_MASTER_SECRET_SIZE);
+ hs->new_session->secret.ResizeForOverwrite(SSL3_MASTER_SECRET_SIZE);
if (!tls1_generate_master_secret(hs, MakeSpan(hs->new_session->secret),
premaster_secret)) {
return ssl_hs_error;
diff --git a/ssl/internal.h b/ssl/internal.h
index c4a36ac..4a1f84c 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -334,14 +334,24 @@
}
// Init replaces the array with a newly-allocated array of |new_size|
- // default-constructed copies of |T|. It returns true on success and false on
- // error.
- //
- // Note that if |T| is a primitive type like |uint8_t|, it is uninitialized.
+ // value-constructed copies of |T|. It returns true on success and false on
+ // error. If |T| is a primitive type like |uint8_t|, value-construction means
+ // it will be zero-initialized.
bool Init(size_t new_size) {
if (!InitUninitialized(new_size)) {
return false;
}
+ cxx17_uninitialized_value_construct_n(data_, size_);
+ return true;
+ }
+
+ // InitForOverwrite behaves like |Init| but it default-constructs each element
+ // instead. This means that, if |T| is a primitive type, the array will be
+ // uninitialized and thus must be filled in by the caller.
+ bool InitForOverwrite(size_t new_size) {
+ if (!InitUninitialized(new_size)) {
+ return false;
+ }
cxx17_uninitialized_default_construct_n(data_, size_);
return true;
}
@@ -585,10 +595,10 @@
return true;
}
- // TryResizeMaybeUninit behaves like |TryResize|, but newly-added elements are
- // default-initialized, so POD types may contain uninitialized values that the
- // caller is responsible for filling in.
- bool TryResizeMaybeUninit(size_t new_size) {
+ // TryResizeForOverwrite behaves like |TryResize|, but newly-added elements
+ // are default-initialized, so POD types may contain uninitialized values that
+ // the caller is responsible for filling in.
+ bool TryResizeForOverwrite(size_t new_size) {
if (new_size <= size_) {
Shrink(new_size);
return true;
@@ -628,8 +638,8 @@
// 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 ResizeForOverwrite(size_t size) {
+ BSSL_CHECK(TryResizeForOverwrite(size));
}
void CopyFrom(Span<const T> in) { BSSL_CHECK(TryCopyFrom(in)); }
T &PushBack(T val) {
diff --git a/ssl/ssl_cipher.cc b/ssl/ssl_cipher.cc
index 97e69ff..48f90a1 100644
--- a/ssl/ssl_cipher.cc
+++ b/ssl/ssl_cipher.cc
@@ -919,7 +919,6 @@
if (!number_uses.Init(max_strength_bits + 1)) {
return false;
}
- OPENSSL_memset(number_uses.data(), 0, (max_strength_bits + 1) * sizeof(int));
// Now find the strength_bits values actually used.
curr = *head_p;
@@ -1231,7 +1230,7 @@
UniquePtr<STACK_OF(SSL_CIPHER)> cipherstack(sk_SSL_CIPHER_new_null());
Array<bool> in_group_flags;
if (cipherstack == nullptr ||
- !in_group_flags.Init(OPENSSL_ARRAY_SIZE(kCiphers))) {
+ !in_group_flags.InitForOverwrite(OPENSSL_ARRAY_SIZE(kCiphers))) {
return false;
}
@@ -1246,13 +1245,11 @@
in_group_flags[num_in_group_flags++] = curr->in_group;
}
}
+ in_group_flags.Shrink(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))) {
+ if (!pref_list || !pref_list->Init(std::move(cipherstack), in_group_flags)) {
return false;
}
diff --git a/ssl/ssl_credential.cc b/ssl/ssl_credential.cc
index d5df82e..b5282b8 100644
--- a/ssl/ssl_credential.cc
+++ b/ssl/ssl_credential.cc
@@ -48,7 +48,7 @@
num_creds++;
}
- if (!out->Init(num_creds)) {
+ if (!out->InitForOverwrite(num_creds)) {
return false;
}
diff --git a/ssl/ssl_internal_test.cc b/ssl/ssl_internal_test.cc
index 258d39d..f5982a2 100644
--- a/ssl/ssl_internal_test.cc
+++ b/ssl/ssl_internal_test.cc
@@ -23,6 +23,15 @@
BSSL_NAMESPACE_BEGIN
namespace {
+TEST(ArrayTest, InitValueConstructs) {
+ Array<uint8_t> array;
+ ASSERT_TRUE(array.Init(10));
+ EXPECT_EQ(array.size(), 10u);
+ for (size_t i = 0; i < 10u; i++) {
+ EXPECT_EQ(0u, array[i]);
+ }
+}
+
TEST(ArrayDeathTest, BoundsChecks) {
Array<int> array;
const int v[] = {1, 2, 3, 4};
@@ -345,7 +354,7 @@
EXPECT_DEATH_IF_SUPPORTED(vec[1000], "");
// The vector cannot be resized past the capacity.
EXPECT_DEATH_IF_SUPPORTED(vec.Resize(5), "");
- EXPECT_DEATH_IF_SUPPORTED(vec.ResizeMaybeUninit(5), "");
+ EXPECT_DEATH_IF_SUPPORTED(vec.ResizeForOverwrite(5), "");
int too_much_data[] = {1, 2, 3, 4, 5};
EXPECT_DEATH_IF_SUPPORTED(vec.CopyFrom(too_much_data), "");
vec.Resize(4);
diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 88144b0..0342e62 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -108,7 +108,7 @@
// Encode the x-coordinate left-padded with zeros.
Array<uint8_t> secret;
- if (!secret.Init((EC_GROUP_get_degree(group_) + 7) / 8) ||
+ if (!secret.InitForOverwrite((EC_GROUP_get_degree(group_) + 7) / 8) ||
!BN_bn2bin_padded(secret.data(), secret.size(), x.get())) {
return false;
}
@@ -162,7 +162,7 @@
*out_alert = SSL_AD_INTERNAL_ERROR;
Array<uint8_t> secret;
- if (!secret.Init(32)) {
+ if (!secret.InitForOverwrite(32)) {
return false;
}
@@ -220,7 +220,7 @@
bool Encap(CBB *out_ciphertext, Array<uint8_t> *out_secret,
uint8_t *out_alert, Span<const uint8_t> peer_key) override {
Array<uint8_t> secret;
- if (!secret.Init(32 + KYBER_SHARED_SECRET_BYTES)) {
+ if (!secret.InitForOverwrite(32 + KYBER_SHARED_SECRET_BYTES)) {
return false;
}
@@ -260,7 +260,7 @@
*out_alert = SSL_AD_INTERNAL_ERROR;
Array<uint8_t> secret;
- if (!secret.Init(32 + KYBER_SHARED_SECRET_BYTES)) {
+ if (!secret.InitForOverwrite(32 + KYBER_SHARED_SECRET_BYTES)) {
return false;
}
@@ -308,7 +308,8 @@
bool Encap(CBB *out_ciphertext, Array<uint8_t> *out_secret,
uint8_t *out_alert, Span<const uint8_t> peer_key) override {
Array<uint8_t> secret;
- if (!secret.Init(MLKEM_SHARED_SECRET_BYTES + X25519_SHARED_KEY_LEN)) {
+ if (!secret.InitForOverwrite(MLKEM_SHARED_SECRET_BYTES +
+ X25519_SHARED_KEY_LEN)) {
return false;
}
@@ -349,7 +350,8 @@
*out_alert = SSL_AD_INTERNAL_ERROR;
Array<uint8_t> secret;
- if (!secret.Init(MLKEM_SHARED_SECRET_BYTES + X25519_SHARED_KEY_LEN)) {
+ if (!secret.InitForOverwrite(MLKEM_SHARED_SECRET_BYTES +
+ X25519_SHARED_KEY_LEN)) {
return false;
}
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index cbb5cbf..ef4b7d5 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -1976,7 +1976,7 @@
static bool ssl_nids_to_group_ids(Array<uint16_t> *out_group_ids,
Span<const int> nids) {
Array<uint16_t> group_ids;
- if (!group_ids.Init(nids.size())) {
+ if (!group_ids.InitForOverwrite(nids.size())) {
return false;
}
@@ -2018,7 +2018,7 @@
} while (col);
Array<uint16_t> group_ids;
- if (!group_ids.Init(count)) {
+ if (!group_ids.InitForOverwrite(count)) {
return false;
}
diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index 76ba084..14a8b67 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -603,7 +603,7 @@
// Check for invalid algorithms, and filter out |SSL_SIGN_RSA_PKCS1_MD5_SHA1|.
Array<uint16_t> filtered;
- if (!filtered.Init(prefs.size())) {
+ if (!filtered.InitForOverwrite(prefs.size())) {
return false;
}
size_t added = 0;
@@ -695,7 +695,7 @@
}
const size_t num_pairs = num_values / 2;
- if (!out->Init(num_pairs)) {
+ if (!out->InitForOverwrite(num_pairs)) {
return false;
}
@@ -771,7 +771,7 @@
}
}
- if (!out->Init(num_elements)) {
+ if (!out->InitForOverwrite(num_elements)) {
return false;
}
size_t out_i = 0;
diff --git a/ssl/t1_enc.cc b/ssl/t1_enc.cc
index a13629e..fb9ae89 100644
--- a/ssl/t1_enc.cc
+++ b/ssl/t1_enc.cc
@@ -214,7 +214,7 @@
// Ensure that |key_block_cache| is set up.
const size_t key_block_size = 2 * (mac_secret_len + key_len + iv_len);
if (key_block_cache->empty()) {
- if (!key_block_cache->Init(key_block_size) ||
+ if (!key_block_cache->InitForOverwrite(key_block_size) ||
!generate_key_block(ssl, MakeSpan(*key_block_cache), session)) {
return false;
}
@@ -362,7 +362,7 @@
seed_len += 2 + context_len;
}
Array<uint8_t> seed;
- if (!seed.Init(seed_len)) {
+ if (!seed.InitForOverwrite(seed_len)) {
return 0;
}
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index 24da90a..7082d16 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -1166,7 +1166,7 @@
// Historically, OpenSSL filled in fake session IDs for ticket-based sessions.
// Envoy's tests depend on this, although perhaps they shouldn't.
- session->session_id.ResizeMaybeUninit(SHA256_DIGEST_LENGTH);
+ session->session_id.ResizeForOverwrite(SHA256_DIGEST_LENGTH);
SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id.data());
session->ticket_age_add_valid = true;
diff --git a/ssl/tls13_enc.cc b/ssl/tls13_enc.cc
index db59f71..29f07ea 100644
--- a/ssl/tls13_enc.cc
+++ b/ssl/tls13_enc.cc
@@ -168,7 +168,7 @@
return false;
}
- out->ResizeMaybeUninit(transcript.DigestLen());
+ out->ResizeForOverwrite(transcript.DigestLen());
return hkdf_expand_label(MakeSpan(*out), transcript.Digest(), hs->secret,
label, MakeConstSpan(context_hash, context_hash_len),
SSL_is_dtls(hs->ssl));
