Make str2bn return a bssl::UniquePtr instead of a raw pointer, in ord…

…er to avoid easy to miss memory leaks. This also should fix the existing potential leak in rsa_ssa_pss_verify_boringssl.cc. PiperOrigin-RevId: 206331453 GitOrigin-RevId: 8fd6f92082c695ca6e390065ab689138cdbaf1a6
raipc · Jul 27, 2018 · 2561fc3 · 2561fc3
1 parent f8ebc9f
commit 2561fc3
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 6 deletions.
diff --git a/cc/subtle/rsa_ssa_pss_verify_boringssl.cc b/cc/subtle/rsa_ssa_pss_verify_boringssl.cc
@@ -64,7 +64,7 @@ RsaSsaPssVerifyBoringSsl::New(
   if (!status_or_n.ok()) return status_or_n.status();
   auto status_or_e = SubtleUtilBoringSSL::str2bn(pub_key.e);
   if (!status_or_e.ok()) return status_or_e.status();
-  size_t modulus_size = BN_num_bits(status_or_n.ValueOrDie());
+  size_t modulus_size = BN_num_bits(status_or_n.ValueOrDie().get());
   if (modulus_size < kMinModulusSizeInBits) {
     return ToStatusF(
         util::error::INVALID_ARGUMENT,
@@ -77,10 +77,12 @@ RsaSsaPssVerifyBoringSsl::New(
                         "BoringSsl RSA allocation error");
   }
   // Set RSA public key and hence d is nullptr.
-  if (1 != RSA_set0_key(rsa.get(), status_or_n.ValueOrDie(),
-                        status_or_e.ValueOrDie(), nullptr /* d */)) {
+  if (1 != RSA_set0_key(rsa.get(), status_or_n.ValueOrDie().get(),
+                        status_or_e.ValueOrDie().get(), nullptr /* d */)) {
     return util::Status(util::error::INTERNAL, "Could not set RSA key.");
   }
+  status_or_n.ValueOrDie().release();
+  status_or_e.ValueOrDie().release();
   std::unique_ptr<RsaSsaPssVerifyBoringSsl> verify(new RsaSsaPssVerifyBoringSsl(
       std::move(rsa), sig_hash_result.ValueOrDie(),
       mgf1_hash_result.ValueOrDie(), params.salt_length));

diff --git a/cc/subtle/subtle_util_boringssl.cc b/cc/subtle/subtle_util_boringssl.cc
@@ -47,14 +47,15 @@ size_t FieldElementSizeInBytes(const EC_GROUP* group) {
 }  // namespace
 
 // static
-util::StatusOr<BIGNUM *> SubtleUtilBoringSSL::str2bn(absl::string_view s) {
+util::StatusOr<bssl::UniquePtr<BIGNUM>> SubtleUtilBoringSSL::str2bn(
+    absl::string_view s) {
   bssl::UniquePtr<BIGNUM> bn(
       BN_bin2bn(reinterpret_cast<const unsigned char *>(s.data()), s.length(),
                 nullptr /* ret */));
   if (bn.get() == nullptr) {
     return util::Status(util::error::INTERNAL, "BIGNUM allocation failed");
   }
-  return bn.release();
+  return std::move(bn);
 }
 
 // static

diff --git a/cc/subtle/subtle_util_boringssl.h b/cc/subtle/subtle_util_boringssl.h
@@ -63,7 +63,7 @@ class SubtleUtilBoringSSL {
 
   // Returns BoringSSL's BIGNUM constructed from bigendian std::string
   // representation.
-  static util::StatusOr<BIGNUM *> str2bn(absl::string_view s);
+  static util::StatusOr<bssl::UniquePtr<BIGNUM>> str2bn(absl::string_view s);
 
   // Returns BoringSSL error strings accumulated in the error queue,
   // thus emptying the queue.