Fix extraneous call to ContentVerifyJob::DispatchFailureCallback.

If ContentVerifyJob receives hashes (ContentVerifyJob::OnHashesReady)
after the content is read (ContentVerifyJob::DoneReading), then it
was possible for DispatchFailureCallback to be called more than once.
This causes a DCHECK failure in DispatchFailureCallback
(DCHECK(!failed_)).

Bail out early in OnHashesReady if call to ContentVerifyJob::BytesRead
leads to a ContentVerifyJob failure.

Note that this isn't harmful in Release builds as
DispatchFailureCallback tests ContentVerifyJob::failure_callback_
before calling the callback, which is reset once it is called.
Nevertheless, this is incorrect behavior and this CL fixes that.


Bug: 804630
Test: See bug description for repro steps.
Change-Id: Icf3a5c8b4c0d01cb20e02de14b11aca4aeff03e5
Reviewed-on: https://chromium-review.googlesource.com/879961Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531755}

extensions/browser/content_verifier/content_hash.h

@@ -12,6 +12,7 @@
 #include "extensions/browser/computed_hashes.h"
 #include "extensions/browser/content_verifier_delegate.h"
 #include "extensions/browser/verified_contents.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension_id.h"
 
 namespace extensions {
@@ -137,7 +138,7 @@ class ContentHash {
   // The block size to use for hashing.
   // TODO(asargent) - use the value from verified_contents.json for each
   // file, instead of using a constant.
-  int block_size_ = 4096;
+  int block_size_ = extension_misc::kContentVerificationDefaultBlockSize;
 
   DISALLOW_COPY_AND_ASSIGN(ContentHash);
 };

extensions/browser/content_verify_job.cc

@@ -138,6 +138,7 @@ void ContentVerifyJob::DoneReading() {
 }
 
 bool ContentVerifyJob::FinishBlock() {
+  DCHECK(!failed_);
   if (current_hash_byte_count_ == 0) {
     if (!done_reading_ ||
         // If we have checked all blocks already, then nothing else to do here.
@@ -188,11 +189,15 @@ void ContentVerifyJob::OnHashesReady(bool success) {
     return;
   }
 
+  DCHECK(!failed_);
+
   hashes_ready_ = true;
   if (!queue_.empty()) {
     std::string tmp;
     queue_.swap(tmp);
     BytesRead(tmp.size(), base::string_as_array(&tmp));
+    if (failed_)
+      return;
   }
   if (done_reading_) {
     ScopedElapsedTimer timer(&time_spent_);

extensions/browser/content_verify_job_unittest.cc

@@ -128,6 +128,38 @@ class ContentVerifyJobUnittest : public ExtensionsTest {
     return observer.WaitAndGetFailureReason();
   }
 
+  // Runs test to verify that a modified extension resource (background.js)
+  // causes ContentVerifyJob to fail with HASH_MISMATCH. The string
+  // |content_to_append_for_mismatch| is appended to the resource for
+  // modification.
+  void RunContentMismatchTest(
+      const std::string& content_to_append_for_mismatch) {
+    base::FilePath unzipped_path;
+    base::FilePath test_dir_base = GetTestPath(
+        base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
+    scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
+        test_dir_base.AppendASCII("source_all.zip"), &unzipped_path);
+    ASSERT_TRUE(extension.get());
+    // Make sure there is a verified_contents.json file there as this test
+    // cannot fetch it.
+    EXPECT_TRUE(base::PathExists(
+        file_util::GetVerifiedContentsPath(extension->path())));
+
+    const base::FilePath::CharType kResource[] =
+        FILE_PATH_LITERAL("background.js");
+    base::FilePath existent_resource_path(kResource);
+    {
+      // Make sure modified background.js fails content verification.
+      std::string modified_contents;
+      base::ReadFileToString(unzipped_path.Append(base::FilePath(kResource)),
+                             &modified_contents);
+      modified_contents.append(content_to_append_for_mismatch);
+      EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
+                RunContentVerifyJob(*extension.get(), existent_resource_path,
+                                    modified_contents));
+    }
+  }
+
  private:
   base::ScopedTempDir temp_dir_;
 
@@ -222,30 +254,15 @@ TEST_F(ContentVerifyJobUnittest, DeletedAndMissingFiles) {
 
 // Tests that content modification causes content verification failure.
 TEST_F(ContentVerifyJobUnittest, ContentMismatch) {
-  base::FilePath unzipped_path;
-  base::FilePath test_dir_base =
-      GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
-  scoped_refptr<Extension> extension = UnzipToTempDirAndLoad(
-      test_dir_base.AppendASCII("source_all.zip"), &unzipped_path);
-  ASSERT_TRUE(extension.get());
-  // Make sure there is a verified_contents.json file there as this test cannot
-  // fetch it.
-  EXPECT_TRUE(
-      base::PathExists(file_util::GetVerifiedContentsPath(extension->path())));
+  RunContentMismatchTest("console.log('modified');");
+}
 
-  const base::FilePath::CharType kResource[] =
-      FILE_PATH_LITERAL("background.js");
-  base::FilePath existent_resource_path(kResource);
-  {
-    // Make sure modified background.js fails content verification.
-    std::string modified_contents;
-    base::ReadFileToString(unzipped_path.Append(base::FilePath(kResource)),
-                           &modified_contents);
-    modified_contents.append("console.log('modified');");
-    EXPECT_EQ(ContentVerifyJob::HASH_MISMATCH,
-              RunContentVerifyJob(*extension.get(), existent_resource_path,
-                                  modified_contents));
-  }
+// Similar to ContentMismatch, but uses a file size > 4k.
+// Regression test for https://crbug.com/804630.
+TEST_F(ContentVerifyJobUnittest, ContentMismatchWithLargeFile) {
+  std::string content_larger_than_block_size(
+      extension_misc::kContentVerificationDefaultBlockSize + 1, ';');
+  RunContentMismatchTest(content_larger_than_block_size);
 }
 
 // Tests that extension resources that are originally 0 byte behave correctly

extensions/common/constants.cc

@@ -119,4 +119,6 @@ const char* const kHangoutsExtensionIds[6] = {
 const char kPolicyBlockedScripting[] =
     "This page cannot be scripted due to an ExtensionsSettings policy.";
 
+const int kContentVerificationDefaultBlockSize = 4096;
+
 }  // namespace extension_misc

extensions/common/constants.h

@@ -237,6 +237,9 @@ extern const char* const kHangoutsExtensionIds[6];
 // Error message when enterprise policy blocks scripting of webpage.
 extern const char kPolicyBlockedScripting[];
 
+// The default block size for hashing used in content verification.
+extern const int kContentVerificationDefaultBlockSize;
+
 }  // namespace extension_misc
 
 #endif  // EXTENSIONS_COMMON_CONSTANTS_H_