Avoid throwing exceptions in HashValidator. (#1753)

In general, we prefer not to throw exceptions to signal an hash mismatch
error. This makes the code easier to use if the application has disabled
exceptions.

There is one case where we must throw an exception though: if an error is
detected in the ObjectReadStreambuf the only mechanism to signal error is
exceptions (these classes must obey the protocol defined by the standard
library, and the library is designed with exceptions in mind). When
exceptions are disabled and we detect such errors we set the `status()`,
and the application is expected to check it. This is not ideal, but the
best that can be done in that case.

Author: coryan

google/cloud/storage/client_write_object_test.cc

@@ -60,7 +60,7 @@ class MockStreambuf : public internal::ObjectWriteStreambuf {
  public:
   MOCK_CONST_METHOD0(IsOpen, bool());
   MOCK_METHOD0(DoClose, StatusOr<internal::HttpResponse>());
-  MOCK_METHOD1(ValidateHash, void(ObjectMetadata const&));
+  MOCK_METHOD1(ValidateHash, bool(ObjectMetadata const&));
   MOCK_CONST_METHOD0(received_hash, std::string const&());
   MOCK_CONST_METHOD0(computed_hash, std::string const&());
   MOCK_CONST_METHOD0(resumable_session_id, std::string const&());

google/cloud/storage/internal/curl_resumable_streambuf.cc

@@ -35,10 +35,10 @@ bool CurlResumableStreambuf::IsOpen() const {
   return static_cast<bool>(upload_session_);
 }
 
-void CurlResumableStreambuf::ValidateHash(ObjectMetadata const& meta) {
+bool CurlResumableStreambuf::ValidateHash(ObjectMetadata const& meta) {
   hash_validator_->ProcessMetadata(meta);
-  hash_validator_result_ =
-      HashValidator::FinishAndCheck(__func__, std::move(*hash_validator_));
+  hash_validator_result_ = std::move(*hash_validator_).Finish();
+  return not hash_validator_result_.is_mismatch;
 }
 
 CurlResumableStreambuf::int_type CurlResumableStreambuf::overflow(int_type ch) {

google/cloud/storage/internal/curl_resumable_streambuf.h

@@ -39,7 +39,7 @@ class CurlResumableStreambuf : public ObjectWriteStreambuf {
   ~CurlResumableStreambuf() override = default;
 
   bool IsOpen() const override;
-  void ValidateHash(ObjectMetadata const& meta) override;
+  bool ValidateHash(ObjectMetadata const& meta) override;
   std::string const& received_hash() const override {
     return hash_validator_result_.received;
   }

google/cloud/storage/internal/curl_streambuf.cc

@@ -44,14 +44,34 @@ void CurlReadStreambuf::Close() {
 }
 
 CurlReadStreambuf::int_type CurlReadStreambuf::underflow() {
+  char const* function_name = __func__;
+  auto report_hash_mismatch = [this,
+                               function_name]() -> CurlReadStreambuf::int_type {
+    std::string msg;
+    msg += function_name;
+    msg += "() - mismatched hashes in download";
+#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
+    throw HashMismatchError(msg, hash_validator_result_.received,
+                            hash_validator_result_.computed);
+#else
+    msg += ", expected=";
+    msg += hash_validator_result_.computed;
+    msg += ", received=";
+    msg += hash_validator_result_.received;
+    status_ = Status(StatusCode::DATA_LOSS, std::move(msg));
+    return traits_type::eof();
+#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
+  };
+
   if (not IsOpen()) {
     // The stream is closed, reading from a closed stream can happen if there is
     // no object to read from, or the object is empty. In that case just setup
     // an empty (but valid) region and verify the checksums.
     SetEmptyRegion();
-    hash_validator_result_ = HashValidator::FinishAndCheck(
-        __func__ + std::string(" mismatched hashes reading from closed stream"),
-        std::move(*hash_validator_));
+    hash_validator_result_ = std::move(*hash_validator_).Finish();
+    if (hash_validator_result_.is_mismatch) {
+      return report_hash_mismatch();
+    }
     return traits_type::eof();
   }
 
@@ -80,9 +100,10 @@ CurlReadStreambuf::int_type CurlReadStreambuf::underflow() {
   // empty (but valid) region:
   SetEmptyRegion();
   // Verify the checksums, and return the EOF character.
-  hash_validator_result_ = HashValidator::FinishAndCheck(
-      __func__ + std::string(" mismatched hashes at end of download"),
-      std::move(*hash_validator_));
+  hash_validator_result_ = std::move(*hash_validator_).Finish();
+  if (hash_validator_result_.is_mismatch) {
+    return report_hash_mismatch();
+  }
   return traits_type::eof();
 }
 
@@ -121,10 +142,10 @@ CurlWriteStreambuf::CurlWriteStreambuf(
 
 bool CurlWriteStreambuf::IsOpen() const { return upload_.IsOpen(); }
 
-void CurlWriteStreambuf::ValidateHash(ObjectMetadata const& meta) {
+bool CurlWriteStreambuf::ValidateHash(ObjectMetadata const& meta) {
   hash_validator_->ProcessMetadata(meta);
-  hash_validator_result_ =
-      HashValidator::FinishAndCheck(__func__, std::move(*hash_validator_));
+  hash_validator_result_ = std::move(*hash_validator_).Finish();
+  return not hash_validator_result_.is_mismatch;
 }
 
 CurlWriteStreambuf::int_type CurlWriteStreambuf::overflow(int_type ch) {

google/cloud/storage/internal/curl_streambuf.h

@@ -79,7 +79,7 @@ class CurlWriteStreambuf : public ObjectWriteStreambuf {
   ~CurlWriteStreambuf() override = default;
 
   bool IsOpen() const override;
-  void ValidateHash(ObjectMetadata const& meta) override;
+  bool ValidateHash(ObjectMetadata const& meta) override;
   std::string const& received_hash() const override {
     return hash_validator_result_.received;
   }

google/cloud/storage/internal/hash_validator.cc

@@ -26,32 +26,6 @@ namespace storage {
 inline namespace STORAGE_CLIENT_NS {
 namespace internal {
 
-void HashValidator::CheckResult(std::string const& msg,
-                                HashValidator::Result const& result) {
-  if (not result.is_mismatch) {
-    return;
-  }
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  // Sometimes the server simply does not have a MD5 hash to send us, the most
-  // common case is a composed object, particularly one formed from encrypted
-  // components, where computing the MD5 would require decrypting and re-reading
-  // all the components. In that case we just do not raise an exception even if
-  // there is a mismatch.
-  throw HashMismatchError(msg, result.received, result.computed);
-#else
-  GCP_LOG(ERROR) << "Mismatched hashes: "
-                 << HashMismatchError(
-                     msg, result.received, result.computed).what();
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-}
-
-HashValidator::Result HashValidator::FinishAndCheck(std::string const& msg,
-                                                    HashValidator&& validator) {
-  auto result = std::move(validator).Finish();
-  CheckResult(msg, result);
-  return result;
-}
-
 void CompositeValidator::Update(std::string const& payload) {
   left_->Update(payload);
   right_->Update(payload);

google/cloud/storage/internal/hash_validator.h

@@ -65,19 +65,6 @@ class HashValidator {
    *   validators that disable validation.
    */
   virtual Result Finish() && = 0;
-
-  /**
-   * Raise an exception if the hashes do not match.
-   *
-   * @throws google::cloud::storage::HashValidation
-   */
-  static void CheckResult(std::string const& msg, Result const& result);
-
-  /**
-   * Call Finish() on the validator and check the result.
-   */
-  static Result FinishAndCheck(std::string const& msg,
-                               HashValidator&& validator);
 };
 
 /**

google/cloud/storage/internal/hash_validator_test.cc

@@ -36,79 +36,6 @@ const std::string EMPTY_STRING_MD5_HASH = "1B2M2Y8AsgTpgAmY7PhCfg==";
 const std::string QUICK_FOX_CRC32C_CHECKSUM = "ImIEBA==";
 const std::string QUICK_FOX_MD5_HASH = "nhB9nTcrtoJr2B01QqQZ1g==";
 
-TEST(HashValidator, CheckResultMismatch) {
-  HashValidator::Result result{"received-hash", "computed-hash", true};
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_THROW(try { HashValidator::CheckResult("test-msg", result); } catch (
-                   google::cloud::storage::HashMismatchError const& ex) {
-    EXPECT_EQ("received-hash", ex.received_hash());
-    EXPECT_EQ("computed-hash", ex.computed_hash());
-    EXPECT_THAT(ex.what(), HasSubstr("test-msg"));
-    throw;
-  },
-               std::ios::failure);
-#else
-  // The program should not be terminated in this case.
-  HashValidator::CheckResult("test-msg", result);
-  SUCCEED();
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-}
-
-TEST(HashValidator, CheckResultMatch) {
-  HashValidator::Result result{"received-hash", "computed-hash", false};
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_NO_THROW(HashValidator::CheckResult("test-msg", result));
-#else
-  // The program should not be terminated in this case.
-  HashValidator::CheckResult("test-msg", result);
-  SUCCEED();
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-}
-
-TEST(HashValidator, CheckResultEmptyReceived) {
-  // Sometimes the service does not send the hashes back, we do not treat that
-  // as a hash mismatch.
-  HashValidator::Result result{"", "test-hash"};
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_NO_THROW(HashValidator::CheckResult("test-msg", result));
-#else
-  // The program should not be terminated in this case.
-  HashValidator::CheckResult("test-msg", result);
-  SUCCEED();
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-}
-
-TEST(HashValidator, FinishAndCheck) {
-  NullHashValidator validator;
-  validator.Update("");
-  validator.ProcessHeader("x-goog-hash", "md5=" + EMPTY_STRING_MD5_HASH);
-  auto result = HashValidator::FinishAndCheck("test-msg", std::move(validator));
-  EXPECT_TRUE(result.received.empty());
-  EXPECT_TRUE(result.computed.empty());
-}
-
-TEST(HashValidator, FinishAndCheckMismatch) {
-  MD5HashValidator validator;
-  validator.Update("The quick brown fox jumps over the lazy dog");
-  validator.ProcessHeader("x-goog-hash", "md5=" + EMPTY_STRING_MD5_HASH);
-#if GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-  EXPECT_THROW(
-      try {
-        HashValidator::FinishAndCheck("test-msg", std::move(validator));
-      } catch (google::cloud::storage::HashMismatchError const& ex) {
-        EXPECT_EQ(EMPTY_STRING_MD5_HASH, ex.received_hash());
-        EXPECT_EQ(QUICK_FOX_MD5_HASH, ex.computed_hash());
-        EXPECT_THAT(ex.what(), HasSubstr("test-msg"));
-        throw;
-      },
-      std::ios::failure);
-#else
-  auto result = HashValidator::FinishAndCheck("test-msg", std::move(validator));
-  EXPECT_EQ(EMPTY_STRING_MD5_HASH, result.received);
-  EXPECT_EQ(QUICK_FOX_MD5_HASH, result.computed);
-#endif  // GOOGLE_CLOUD_CPP_HAVE_EXCEPTIONS
-}
-
 TEST(NullHashValidatorTest, Simple) {
   NullHashValidator validator;
   validator.ProcessHeader("x-goog-hash", "md5=<placeholder-for-test>");

google/cloud/storage/internal/object_streambuf.h

@@ -69,7 +69,7 @@ class ObjectWriteStreambuf : public std::basic_streambuf<char> {
 
   StatusOr<HttpResponse> Close();
   virtual bool IsOpen() const = 0;
-  virtual void ValidateHash(ObjectMetadata const& meta) = 0;
+  virtual bool ValidateHash(ObjectMetadata const& meta) = 0;
   virtual std::string const& received_hash() const = 0;
   virtual std::string const& computed_hash() const = 0;
 

google/cloud/storage/object_stream.cc

@@ -94,8 +94,9 @@ void ObjectWriteStream::Close() {
     }
   }
 
-  // TODO(#1747) - do not throw exceptions here.
-  buf_->ValidateHash(*metadata_);
+  if (not buf_->ValidateHash(*metadata_)) {
+    setstate(std::ios_base::badbit);
+  }
 }
 
 void ObjectWriteStream::Suspend() && { buf_.reset(); }