Modernize p_hkdf.cc a bit Add a bssl::Vector::Append. That was carrying a CBB just to implement an appendable buffer. Also a couple of minor bits elsewhere in crypto/evp. Change-Id: Ieccd77887ee5fc9f9c0f66358903524411d90d49 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90727 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Rudolf Polzer <rpolzer@google.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com>
diff --git a/crypto/evp/p_dh.cc b/crypto/evp/p_dh.cc index 4d91e15..b2420ab 100644 --- a/crypto/evp/p_dh.cc +++ b/crypto/evp/p_dh.cc
@@ -138,7 +138,7 @@ }; struct DH_PKEY_CTX { - int pad = 0; + bool pad = false; }; static int pkey_dh_init(EvpPkeyCtx *ctx, const EVP_PKEY_ALG *) {
diff --git a/crypto/evp/p_ec.cc b/crypto/evp/p_ec.cc index ed6f830..2e4704a 100644 --- a/crypto/evp/p_ec.cc +++ b/crypto/evp/p_ec.cc
@@ -312,11 +312,11 @@ int_ec_free, }; -typedef struct { +struct EC_PKEY_CTX { // message digest - const EVP_MD *md; - const EC_GROUP *gen_group; -} EC_PKEY_CTX; + const EVP_MD *md = nullptr; + const EC_GROUP *gen_group = nullptr; +}; static int pkey_ec_init(EvpPkeyCtx *ctx, const EVP_PKEY_ALG *alg) { EC_PKEY_CTX *dctx = New<EC_PKEY_CTX>();
diff --git a/crypto/evp/p_hkdf.cc b/crypto/evp/p_hkdf.cc index fe58105..d124b85 100644 --- a/crypto/evp/p_hkdf.cc +++ b/crypto/evp/p_hkdf.cc
@@ -14,11 +14,11 @@ #include <openssl/evp.h> -#include <openssl/bytestring.h> #include <openssl/err.h> #include <openssl/hkdf.h> #include <openssl/kdf.h> #include <openssl/mem.h> +#include <openssl/span.h> #include "../internal.h" #include "../mem_internal.h" @@ -29,28 +29,16 @@ namespace { -typedef struct { - int mode; - const EVP_MD *md; - uint8_t *key; - size_t key_len; - uint8_t *salt; - size_t salt_len; - CBB info; -} HKDF_PKEY_CTX; +struct HKDF_PKEY_CTX { + int mode = 0; + const EVP_MD *md = nullptr; + Array<uint8_t> key; + Array<uint8_t> salt; + Vector<uint8_t> info; +}; static int pkey_hkdf_init(EvpPkeyCtx *ctx, const EVP_PKEY_ALG *) { - HKDF_PKEY_CTX *hctx = New<HKDF_PKEY_CTX>(); - if (hctx == nullptr) { - return 0; - } - - if (!CBB_init(&hctx->info, 0)) { - Delete(hctx); - return 0; - } - - ctx->data = hctx; + ctx->data = New<HKDF_PKEY_CTX>(); return 1; } @@ -65,26 +53,9 @@ hctx_dst->mode = hctx_src->mode; hctx_dst->md = hctx_src->md; - if (hctx_src->key_len != 0) { - hctx_dst->key = reinterpret_cast<uint8_t *>( - OPENSSL_memdup(hctx_src->key, hctx_src->key_len)); - if (hctx_dst->key == nullptr) { - return 0; - } - hctx_dst->key_len = hctx_src->key_len; - } - - if (hctx_src->salt_len != 0) { - hctx_dst->salt = reinterpret_cast<uint8_t *>( - OPENSSL_memdup(hctx_src->salt, hctx_src->salt_len)); - if (hctx_dst->salt == nullptr) { - return 0; - } - hctx_dst->salt_len = hctx_src->salt_len; - } - - if (!CBB_add_bytes(&hctx_dst->info, CBB_data(&hctx_src->info), - CBB_len(&hctx_src->info))) { + if (!hctx_dst->key.CopyFrom(hctx_src->key) || + !hctx_dst->salt.CopyFrom(hctx_src->salt) || + !hctx_dst->info.CopyFrom(hctx_src->info)) { return 0; } @@ -93,13 +64,8 @@ static void pkey_hkdf_cleanup(EvpPkeyCtx *ctx) { HKDF_PKEY_CTX *hctx = reinterpret_cast<HKDF_PKEY_CTX *>(ctx->data); - if (hctx != nullptr) { - OPENSSL_free(hctx->key); - OPENSSL_free(hctx->salt); - CBB_cleanup(&hctx->info); - Delete(hctx); - ctx->data = nullptr; - } + Delete(hctx); + ctx->data = nullptr; } static int pkey_hkdf_derive(EvpPkeyCtx *ctx, uint8_t *out, size_t *out_len) { @@ -108,7 +74,7 @@ OPENSSL_PUT_ERROR(EVP, EVP_R_MISSING_PARAMETERS); return 0; } - if (hctx->key_len == 0) { + if (hctx->key.empty()) { OPENSSL_PUT_ERROR(EVP, EVP_R_NO_KEY_SET); return 0; } @@ -124,20 +90,23 @@ switch (hctx->mode) { case EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND: - return HKDF(out, *out_len, hctx->md, hctx->key, hctx->key_len, hctx->salt, - hctx->salt_len, CBB_data(&hctx->info), CBB_len(&hctx->info)); + return HKDF(out, *out_len, hctx->md, hctx->key.data(), hctx->key.size(), + hctx->salt.data(), hctx->salt.size(), hctx->info.data(), + hctx->info.size()); case EVP_PKEY_HKDEF_MODE_EXTRACT_ONLY: if (*out_len < EVP_MD_size(hctx->md)) { OPENSSL_PUT_ERROR(EVP, EVP_R_BUFFER_TOO_SMALL); return 0; } - return HKDF_extract(out, out_len, hctx->md, hctx->key, hctx->key_len, - hctx->salt, hctx->salt_len); + return HKDF_extract(out, out_len, hctx->md, hctx->key.data(), + hctx->key.size(), hctx->salt.data(), + hctx->salt.size()); case EVP_PKEY_HKDEF_MODE_EXPAND_ONLY: - return HKDF_expand(out, *out_len, hctx->md, hctx->key, hctx->key_len, - CBB_data(&hctx->info), CBB_len(&hctx->info)); + return HKDF_expand(out, *out_len, hctx->md, hctx->key.data(), + hctx->key.size(), hctx->info.data(), + hctx->info.size()); } OPENSSL_PUT_ERROR(EVP, ERR_R_INTERNAL_ERROR); return 0; @@ -159,27 +128,18 @@ hctx->md = reinterpret_cast<const EVP_MD *>(p2); return 1; case EVP_PKEY_CTRL_HKDF_KEY: { - const CBS *key = reinterpret_cast<const CBS *>(p2); - if (!CBS_stow(key, &hctx->key, &hctx->key_len)) { - return 0; - } - return 1; + const auto *key = reinterpret_cast<const Span<const uint8_t> *>(p2); + return hctx->key.CopyFrom(*key); } case EVP_PKEY_CTRL_HKDF_SALT: { - const CBS *salt = reinterpret_cast<const CBS *>(p2); - if (!CBS_stow(salt, &hctx->salt, &hctx->salt_len)) { - return 0; - } - return 1; + const auto *salt = reinterpret_cast<const Span<const uint8_t> *>(p2); + return hctx->salt.CopyFrom(*salt); } case EVP_PKEY_CTRL_HKDF_INFO: { - const CBS *info = reinterpret_cast<const CBS *>(p2); + const auto *info = reinterpret_cast<const Span<const uint8_t> *>(p2); // |EVP_PKEY_CTX_add1_hkdf_info| appends to the info string, rather than // replacing it. - if (!CBB_add_bytes(&hctx->info, CBS_data(info), CBS_len(info))) { - return 0; - } - return 1; + return hctx->info.Append(*info); } default: OPENSSL_PUT_ERROR(EVP, EVP_R_COMMAND_NOT_SUPPORTED); @@ -224,24 +184,21 @@ int EVP_PKEY_CTX_set1_hkdf_key(EVP_PKEY_CTX *ctx, const uint8_t *key, size_t key_len) { - CBS cbs; - CBS_init(&cbs, key, key_len); + Span<const uint8_t> span(key, key_len); return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_HKDF, EVP_PKEY_OP_DERIVE, - EVP_PKEY_CTRL_HKDF_KEY, 0, &cbs); + EVP_PKEY_CTRL_HKDF_KEY, 0, &span); } int EVP_PKEY_CTX_set1_hkdf_salt(EVP_PKEY_CTX *ctx, const uint8_t *salt, size_t salt_len) { - CBS cbs; - CBS_init(&cbs, salt, salt_len); + Span<const uint8_t> span(salt, salt_len); return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_HKDF, EVP_PKEY_OP_DERIVE, - EVP_PKEY_CTRL_HKDF_SALT, 0, &cbs); + EVP_PKEY_CTRL_HKDF_SALT, 0, &span); } int EVP_PKEY_CTX_add1_hkdf_info(EVP_PKEY_CTX *ctx, const uint8_t *info, size_t info_len) { - CBS cbs; - CBS_init(&cbs, info, info_len); + Span<const uint8_t> span(info, info_len); return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_HKDF, EVP_PKEY_OP_DERIVE, - EVP_PKEY_CTRL_HKDF_INFO, 0, &cbs); + EVP_PKEY_CTRL_HKDF_INFO, 0, &span); }
diff --git a/crypto/mem_internal.h b/crypto/mem_internal.h index cb42b24..bb815d7 100644 --- a/crypto/mem_internal.h +++ b/crypto/mem_internal.h
@@ -380,7 +380,7 @@ // Push adds |elem| at the end of the internal array, growing if necessary. It // returns false when allocation fails. [[nodiscard]] bool Push(T elem) { - if (!MaybeGrow()) { + if (!MaybeGrow(1)) { return false; } new (&data_[size_]) T(std::move(elem)); @@ -402,6 +402,17 @@ return true; } + // Append appends the contents of |in| to the array. It returns true on + // success and false on allocation error. + [[nodiscard]] bool Append(Span<const T> in) { + if (!MaybeGrow(in.size())) { + return false; + } + std::uninitialized_copy(in.begin(), in.end(), data_ + size_); + size_ += in.size(); + return true; + } + // EraseIf removes all elements that satisfy the predicate |pred|. template <typename Pred> void EraseIf(Pred pred) { @@ -411,26 +422,27 @@ } private: - // If there is no room for one more element, creates a new backing array with + // If there is no room for |num| elements, creates a new backing array with // double the size of the old one and copies elements over. - bool MaybeGrow() { - // No need to grow if we have room for one more T. - if (size_ < capacity_) { - return true; - } - size_t new_capacity = kDefaultSize; - if (capacity_ > 0) { - // Double the array's size if it's safe to do so. - if (capacity_ > SIZE_MAX / 2) { - OPENSSL_PUT_ERROR(CRYPTO, ERR_R_OVERFLOW); - return false; - } - new_capacity = capacity_ * 2; - } - if (new_capacity > SIZE_MAX / sizeof(T)) { + [[nodiscard]] bool MaybeGrow(size_t num) { + constexpr size_t kDefaultSize = 16; + constexpr size_t kMaxCapacity = SIZE_MAX / sizeof(T); + if (num > kMaxCapacity - size_) { OPENSSL_PUT_ERROR(CRYPTO, ERR_R_OVERFLOW); return false; } + size_t new_capacity = size_ + num; + // No need to grow if we have room. + if (capacity_ >= new_capacity) { + return true; + } + // Always grow to at least kDefaultSize to avoid several small mallocs at + // the start. + new_capacity = std::max(new_capacity, std::min(kDefaultSize, kMaxCapacity)); + // At least double the old capacity for linear amortized behavior. + if (capacity_ <= kMaxCapacity / 2) { + new_capacity = std::max(new_capacity, capacity_ * 2); + } T *new_data = reinterpret_cast<T *>(OPENSSL_malloc(new_capacity * sizeof(T))); if (new_data == nullptr) { @@ -452,8 +464,6 @@ size_t size_ = 0; // |capacity_| is the number of elements allocated in this Vector. size_t capacity_ = 0; - // |kDefaultSize| is the default initial size of the backing array. - static constexpr size_t kDefaultSize = 16; }; // A PackedSize is an integer that can store values from 0 to N, represented as
diff --git a/crypto/mem_test.cc b/crypto/mem_test.cc index eaa58bf..c443e47 100644 --- a/crypto/mem_test.cc +++ b/crypto/mem_test.cc
@@ -201,6 +201,59 @@ EXPECT_EQ(vec.back(), 42u); } +TEST(VectorTest, AppendSimpleType) { + Vector<size_t> vec; + ASSERT_TRUE(vec.empty()); + + // Append through the initial capacity and small growth cases. + for (size_t i = 0; i < 32; i++) { + size_t in[2] = {2 * i, 2 * i + 1}; + ASSERT_TRUE(vec.Append(in)); + } + EXPECT_EQ(vec.size(), 64u); + for (size_t i = 0; i < 64; i++) { + EXPECT_EQ(vec[i], i); + } + + // Append a large buffer to test when we more than double the capacity. + size_t buf[512]; + for (size_t i = 0; i < std::size(buf); i++) { + buf[i] = 64 + i; + } + ASSERT_TRUE(vec.Append(buf)); + EXPECT_EQ(vec.size(), 512u + 64u); + for (size_t i = 0; i < 512u + 64u; i++) { + EXPECT_EQ(vec[i], i); + } +} + +TEST(VectorTest, AppendComplexType) { + Vector<std::shared_ptr<size_t>> vec; + ASSERT_TRUE(vec.empty()); + + // Append through the initial capacity and small growth cases. + for (size_t i = 0; i < 32; i++) { + std::shared_ptr<size_t> in[2] = {std::make_shared<size_t>(2 * i), + std::make_shared<size_t>(2 * i + 1)}; + ASSERT_TRUE(vec.Append(in)); + } + EXPECT_EQ(vec.size(), 64u); + for (size_t i = 0; i < 64; i++) { + EXPECT_EQ(*vec[i], i); + } + + // Append a large buffer to test when we more than double the capacity. + std::shared_ptr<size_t> buf[512]; + for (size_t i = 0; i < std::size(buf); i++) { + buf[i] = std::make_shared<size_t>(64 + i); + } + ASSERT_TRUE(vec.Append(buf)); + EXPECT_EQ(vec.size(), 512u + 64u); + for (size_t i = 0; i < 512u + 64u; i++) { + EXPECT_EQ(*vec[i], i); + } +} + TEST(VectorTest, MoveConstructor) { Vector<size_t> vec; for (size_t i = 0; i < 100; i++) {