fix self-assignment for OpenSSLHash::Digest, throw if context allocation...

fix self-assignment for OpenSSLHash::Digest, throw if context allocation fails, implement move support

Summary:
The copy assignment operator was not checking for self-assignment, which means the code was wrong when an object was being copied into itself.
Also, allocation failure in the constructor would be silently ignored and result in a segfault crash later. Throw std::runtime_error if OpenSSL context allocation fails.
Also, move constructor and move assignment operator were not implemented - they are now.

Reviewed By: yfeldblum

Differential Revision: D30879545

fbshipit-source-id: 8b06f6fe97912a03ec5480a3b7c69aebf3a7f2ca

folly/ssl/OpenSSLHash.h

@@ -16,6 +16,7 @@
 
 #pragma once
 
+#include <stdexcept>
 #include <folly/Range.h>
 #include <folly/io/IOBuf.h>
 #include <folly/portability/OpenSSL.h>
@@ -30,35 +31,49 @@ class OpenSSLHash {
  public:
   class Digest {
    public:
-    Digest() : ctx_(EVP_MD_CTX_new()) {}
+    Digest() { check_context_notnull(); }
 
-    Digest(const Digest& other) {
-      ctx_ = EvpMdCtxUniquePtr(EVP_MD_CTX_new());
-      if (other.md_ != nullptr) {
-        hash_init(other.md_);
-        check_libssl_result(
-            1, EVP_MD_CTX_copy_ex(ctx_.get(), other.ctx_.get()));
+    Digest(const Digest& that) {
+      check_context_notnull();
+      copy_impl(that);
+    }
+
+    Digest(Digest&& that) noexcept(false) {
+      check_context_notnull();
+      move_impl(std::move(that));
     }
+
+    Digest& operator=(const Digest& that) {
+      if (this != &that) {
+        copy_impl(that);
+      }
+      return *this;
     }
 
-    Digest& operator=(const Digest& other) {
-      this->~Digest();
-      return *new (this) Digest(other);
+    Digest& operator=(Digest&& that) noexcept(false) {
+      if (this != &that) {
+        move_impl(std::move(that));
+        that.hash_reset();
+      }
+      return *this;
     }
 
     void hash_init(const EVP_MD* md) {
       md_ = md;
       check_libssl_result(1, EVP_DigestInit_ex(ctx_.get(), md, nullptr));
     }
+
     void hash_update(ByteRange data) {
       check_libssl_result(
           1, EVP_DigestUpdate(ctx_.get(), data.data(), data.size()));
     }
+
     void hash_update(const IOBuf& data) {
       for (auto r : data) {
         hash_update(r);
       }
     }
+
     void hash_final(MutableByteRange out) {
       const auto size = EVP_MD_size(md_);
       check_out_size(size_t(size), out);
@@ -69,8 +84,34 @@ class OpenSSLHash {
     }
 
    private:
-    const EVP_MD* md_ = nullptr;
-    EvpMdCtxUniquePtr ctx_{nullptr};
+    const EVP_MD* md_{nullptr};
+    EvpMdCtxUniquePtr ctx_{EVP_MD_CTX_new()};
+
+    void hash_reset() {
+      md_ = nullptr;
+      check_libssl_result(1, EVP_MD_CTX_reset(ctx_.get()));
+    }
+
+    void check_context_notnull() {
+      if (nullptr == ctx_) {
+        throw_exception<std::runtime_error>(
+            "EVP_MD_CTX_new() returned nullptr");
+      }
+    }
+
+    void copy_impl(const Digest& that) {
+      if (that.md_ != nullptr) {
+        hash_init(that.md_);
+        check_libssl_result(1, EVP_MD_CTX_copy_ex(ctx_.get(), that.ctx_.get()));
+      } else {
+        this->hash_reset();
+      }
+    }
+
+    void move_impl(Digest&& that) noexcept {
+      std::swap(this->md_, that.md_);
+      std::swap(this->ctx_, that.ctx_);
+    }
   };
 
   static void hash(MutableByteRange out, const EVP_MD* md, ByteRange data) {

folly/ssl/test/OpenSSLHashTest.cpp

@@ -45,36 +45,131 @@ TEST_F(OpenSSLHashTest, sha256) {
 }
 
 TEST_F(OpenSSLHashTest, sha256_hashcopy) {
+  std::array<uint8_t, 32> expected, actual1, actual2;
+  constexpr StringPiece data{"foobar"};
+
+  OpenSSLHash::hash(range(expected), EVP_sha256(), data);
+
+  OpenSSLHash::Digest digest;
+  digest.hash_init(EVP_sha256());
+  digest.hash_update(ByteRange(data));
+
+  OpenSSLHash::Digest copy1(digest); // copy constructor
+  OpenSSLHash::Digest copy2 = digest; // copy assignment operator
+
+  copy1.hash_final(range(actual1));
+  copy2.hash_final(range(actual2));
+
+  EXPECT_EQ(expected, actual1);
+  EXPECT_EQ(expected, actual2);
+}
+
+TEST_F(OpenSSLHashTest, sha256_hashcopy_self) {
   std::array<uint8_t, 32> expected, actual;
+  constexpr StringPiece data{"foobar"};
+
+  OpenSSLHash::hash(range(expected), EVP_sha256(), data);
 
   OpenSSLHash::Digest digest;
   digest.hash_init(EVP_sha256());
-  digest.hash_update(ByteRange(StringPiece("foobar")));
+  digest.hash_update(ByteRange(data));
 
-  OpenSSLHash::Digest copy(digest);
+  OpenSSLHash::Digest* ptr = &digest;
+  digest = *ptr; // test copy of an object to itself
 
-  digest.hash_final(range(expected));
-  copy.hash_final(range(actual));
+  digest.hash_final(range(actual));
 
   EXPECT_EQ(expected, actual);
 }
 
-TEST_F(OpenSSLHashTest, sha256_hashcopy_intermediate) {
+TEST_F(OpenSSLHashTest, sha256_hashmove) {
+  std::array<uint8_t, 32> expected, actual1, actual2;
+  constexpr StringPiece data{"foobar"};
+
+  OpenSSLHash::hash(range(expected), EVP_sha256(), data);
+
+  OpenSSLHash::Digest digest;
+  digest.hash_init(EVP_sha256());
+  digest.hash_update(ByteRange(data));
+  OpenSSLHash::Digest copy1(std::move(digest)); // move constructor
+  copy1.hash_final(range(actual1));
+  EXPECT_EQ(expected, actual1);
+
+  digest = OpenSSLHash::Digest{}; // should be safe to reassign to moved object
+  digest.hash_init(EVP_sha256());
+  digest.hash_update(ByteRange(data));
+  OpenSSLHash::Digest copy2 = std::move(digest); // move assignment operator
+  copy2.hash_final(range(actual2));
+  EXPECT_EQ(expected, actual2);
+}
+
+TEST_F(OpenSSLHashTest, sha256_hashmove_self) {
   std::array<uint8_t, 32> expected, actual;
+  constexpr StringPiece data{"foobar"};
+
+  OpenSSLHash::hash(range(expected), EVP_sha256(), data);
 
   OpenSSLHash::Digest digest;
   digest.hash_init(EVP_sha256());
-  digest.hash_update(ByteRange(StringPiece("foo")));
+  digest.hash_update(ByteRange(data));
+
+  OpenSSLHash::Digest* ptr = &digest;
+  digest = std::move(*ptr); // test move of an object to itself
+
+  digest.hash_final(range(actual));
+
+  EXPECT_EQ(expected, actual);
+}
+
+TEST_F(OpenSSLHashTest, sha256_hashcopy_intermediate) {
+  std::array<uint8_t, 32> expected, actual1, actual2;
+  constexpr StringPiece data1("foo");
+  constexpr StringPiece data2("bar");
 
-  OpenSSLHash::Digest copy(digest);
+  OpenSSLHash::Digest digest;
+  digest.hash_init(EVP_sha256());
+  digest.hash_update(ByteRange(data1));
 
-  digest.hash_update(ByteRange(StringPiece("bar")));
-  copy.hash_update(ByteRange(StringPiece("bar")));
+  OpenSSLHash::Digest copy1(digest); // copy constructor
+  OpenSSLHash::Digest copy2 = digest; // copy assignment operator
 
+  digest.hash_update(ByteRange(data2));
   digest.hash_final(range(expected));
-  copy.hash_final(range(actual));
 
-  EXPECT_EQ(expected, actual);
+  copy1.hash_update(ByteRange(data2));
+  copy1.hash_final(range(actual1));
+  EXPECT_EQ(expected, actual1);
+
+  copy2.hash_update(ByteRange(data2));
+  copy2.hash_final(range(actual2));
+  EXPECT_EQ(expected, actual2);
+}
+
+TEST_F(OpenSSLHashTest, sha256_hashmove_intermediate) {
+  std::array<uint8_t, 32> expected, actual1, actual2;
+  constexpr StringPiece fulldata("foobar");
+  constexpr StringPiece data1("foo");
+  constexpr StringPiece data2("bar");
+
+  OpenSSLHash::hash(range(expected), EVP_sha256(), fulldata);
+
+  OpenSSLHash::Digest digest;
+  digest.hash_init(EVP_sha256());
+  digest.hash_update(ByteRange(data1));
+  OpenSSLHash::Digest copy1(std::move(digest)); // move constructor
+  copy1.hash_update(ByteRange(data2));
+  copy1.hash_final(range(actual1));
+  EXPECT_EQ(expected, actual1);
+
+  digest.hash_init(EVP_sha256()); // should be safe to re-init moved object
+  digest.hash_update(ByteRange(data1));
+  OpenSSLHash::Digest copy2 = std::move(digest); // move assignment operator
+  copy2.hash_update(ByteRange(data2));
+  copy2.hash_final(range(actual2));
+  EXPECT_EQ(expected, actual2);
+
+  // Make sure it's safe to re-init moved object after move operator=()
+  digest.hash_init(EVP_sha256());
 }
 
 TEST_F(OpenSSLHashTest, hmac_sha256) {