Update chrome_Cleaner's zip_archiver subdir to support quarantine feature

Includes a fix to ComputeSHA256DigestOfPath required by the unit test

R=csharp@chromium.org

Bug: 830892
Change-Id: I72e6c1a4fcf21565357e5e6043ee93b480931a4f
Reviewed-on: https://chromium-review.googlesource.com/c/1338659Reviewed-by: Chris Sharp <csharp@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#608794}

chrome/chrome_cleaner/os/disk_util.cc

@@ -590,7 +590,8 @@ bool ComputeSHA256DigestOfPath(const base::FilePath& path,
                                std::string* digest) {
   DCHECK(digest);
 
-  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                            base::File::FLAG_SHARE_DELETE);
   if (!file.IsValid())
     return false;
 

chrome/chrome_cleaner/os/file_removal_status_updater.h

@@ -31,7 +31,7 @@ enum RemovalStatusOverridePermission {
   // when previous knowledge was FAILED_TO_REMOVE.
   kOkToOverride,
   // This should never happen in the code, and we should raise an error.
-  // TODO: Currently there is no error, and kNotAllowed is
+  // TODO(joenotcharles): Currently there is no error, and kNotAllowed is
   // implemented as kSkip. This is because DCHECK writes an error message to
   // the log, and until recently this took the logging lock which might already
   // be held while checking this permission. Now that it's safe to DCHECK while

chrome/chrome_cleaner/zip_archiver/BUILD.gn

@@ -9,8 +9,10 @@ static_library("common") {
   ]
 
   deps = [
+    "//chrome/chrome_cleaner/constants:quarantine_constants",
     "//chrome/chrome_cleaner/interfaces:zip_archiver_interface",
     "//chrome/chrome_cleaner/ipc:mojo_task_runner",
+    "//chrome/chrome_cleaner/ipc:sandbox",
     "//chrome/chrome_cleaner/os:common_os",
     "//chrome/chrome_cleaner/zip_archiver/broker:common",
     "//mojo/public/cpp/system:system",
@@ -41,6 +43,7 @@ source_set("unittest_sources") {
 
   deps = [
     "//base/test:test_support",
+    "//chrome/chrome_cleaner/constants:quarantine_constants",
     "//chrome/chrome_cleaner/interfaces:zip_archiver_interface",
     "//chrome/chrome_cleaner/ipc:mojo_task_runner",
     "//chrome/chrome_cleaner/os:common_os",

chrome/chrome_cleaner/zip_archiver/DEPS

 include_rules = [
   "+third_party/zlib",
 ]
-

chrome/chrome_cleaner/zip_archiver/sandboxed_zip_archiver.cc

@@ -12,8 +12,9 @@
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/synchronization/waitable_event.h"
+#include "base/system/sys_info.h"
 #include "base/win/scoped_handle.h"
+#include "chrome/chrome_cleaner/constants/quarantine_constants.h"
 #include "chrome/chrome_cleaner/os/disk_util.h"
 #include "mojo/public/cpp/system/platform_handle.h"
 
@@ -23,6 +24,10 @@ namespace {
 
 using mojom::ZipArchiverResultCode;
 
+// According to the zip structure and tests, zipping one file with STORE
+// compression level should not increase the file size more than 1KB.
+constexpr int64_t kZipAdditionalSize = 1024;
+
 constexpr wchar_t kDefaultFileStreamSuffix[] = L"::$DATA";
 constexpr uint32_t kMinimizedReadAccess =
     SYNCHRONIZE | FILE_READ_DATA | FILE_READ_ATTRIBUTES;
@@ -66,15 +71,33 @@ void RunArchiver(mojom::ZipArchiverPtr* zip_archiver_ptr,
                 filename, password, std::move(callback));
 }
 
-void OnArchiveDone(ZipArchiverResultCode* return_code,
-                   base::WaitableEvent* waitable_event,
+void OnArchiveDone(const base::FilePath& zip_file_path,
+                   SandboxedZipArchiver::ArchiveResultCallback result_callback,
                    ZipArchiverResultCode result_code) {
-  *return_code = static_cast<ZipArchiverResultCode>(result_code);
-  waitable_event->Signal();
+  if (result_code != ZipArchiverResultCode::kSuccess) {
+    // The zip file handle has been closed by mojo. Delete the incomplete zip
+    // file directly.
+    if (!base::DeleteFile(zip_file_path, /*recursive=*/false))
+      LOG(ERROR) << "Failed to delete the incomplete zip file.";
+  }
+  // Call |result_callback| for SandboxedZipArchiver::Archive.
+  std::move(result_callback).Run(result_code);
 }
 
 }  // namespace
 
+namespace internal {
+
+// Zip file name format: "|filename|_|file_hash|.zip"
+base::string16 ConstructZipArchiveFileName(const base::string16& filename,
+                                           const std::string& file_hash) {
+  const std::string normalized_file_hash = base::ToUpperASCII(file_hash);
+  return base::StrCat(
+      {filename, L"_", base::UTF8ToUTF16(normalized_file_hash), L".zip"});
+}
+
+}  // namespace internal
+
 SandboxedZipArchiver::SandboxedZipArchiver(
     scoped_refptr<MojoTaskRunner> mojo_task_runner,
     UniqueZipArchiverPtr zip_archiver_ptr,
@@ -94,66 +117,76 @@ SandboxedZipArchiver::~SandboxedZipArchiver() = default;
 // password-protected zip file stored in the |dst_archive_folder|. The format of
 // zip file name is "|basename of the source file|_|hexdigest of the source file
 // hash|.zip".
-ZipArchiverResultCode SandboxedZipArchiver::Archive(
-    const base::FilePath& src_file_path,
-    base::FilePath* output_zip_file_path) {
-  DCHECK(output_zip_file_path);
-
+void SandboxedZipArchiver::Archive(const base::FilePath& src_file_path,
+                                   ArchiveResultCallback result_callback) {
   // Open the source file with minimized rights for reading.
-  // Without |FILE_SHARE_WRITE| and |FILE_SHARE_DELETE|, |src_file_path| cannot
-  // be manipulated or replaced until |DoArchive| returns. This prevents the
-  // following checks from TOCTTOU. Because |base::IsLink| doesn't work on
-  // Windows, use |FILE_FLAG_OPEN_REPARSE_POINT| to open a symbolic link then
-  // check. To eliminate any TOCTTOU, use |FILE_FLAG_BACKUP_SEMANTICS| to open a
-  // directory then check.
+  // Allowing all sharing accesses increases the chances of being able to open
+  // and archive the file. Because |base::IsLink| doesn't work on Windows, use
+  // |FILE_FLAG_OPEN_REPARSE_POINT| to open a symbolic link then check. To
+  // eliminate TOCTTOU, use |FILE_FLAG_BACKUP_SEMANTICS| to open a directory
+  // then check.
   base::File src_file(::CreateFile(
       src_file_path.AsUTF16Unsafe().c_str(), kMinimizedReadAccess,
-      FILE_SHARE_READ, nullptr, OPEN_EXISTING,
-      FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, nullptr));
+      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nullptr,
+      OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS,
+      nullptr));
   if (!src_file.IsValid()) {
     LOG(ERROR) << "Unable to open the source file.";
-    return ZipArchiverResultCode::kErrorCannotOpenSourceFile;
+    std::move(result_callback)
+        .Run(ZipArchiverResultCode::kErrorCannotOpenSourceFile);
+    return;
   }
 
   BY_HANDLE_FILE_INFORMATION src_file_info;
   if (!::GetFileInformationByHandle(src_file.GetPlatformFile(),
                                     &src_file_info)) {
     LOG(ERROR) << "Unable to get the source file information.";
-    return ZipArchiverResultCode::kErrorIO;
+    std::move(result_callback).Run(ZipArchiverResultCode::kErrorIO);
+    return;
   }
 
   // Don't archive symbolic links.
-  if (src_file_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)
-    return ZipArchiverResultCode::kIgnoredSourceFile;
+  if (src_file_info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
+    std::move(result_callback).Run(ZipArchiverResultCode::kIgnoredSourceFile);
+    return;
+  }
 
   // Don't archive directories. And |ZipArchiver| shouldn't get called with a
   // directory path.
   if (src_file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
     LOG(ERROR) << "Tried to archive a directory.";
-    return ZipArchiverResultCode::kIgnoredSourceFile;
+    std::move(result_callback).Run(ZipArchiverResultCode::kIgnoredSourceFile);
+    return;
   }
 
   base::string16 sanitized_src_filename;
-  if (!GetSanitizedFileName(src_file_path, &sanitized_src_filename))
-    return ZipArchiverResultCode::kIgnoredSourceFile;
+  if (!GetSanitizedFileName(src_file_path, &sanitized_src_filename)) {
+    std::move(result_callback).Run(ZipArchiverResultCode::kIgnoredSourceFile);
+    return;
+  }
 
-  // TODO(veranika): Check the source file size once the limit is determined.
+  const ZipArchiverResultCode result_code = CheckFileSize(&src_file);
+  if (result_code != ZipArchiverResultCode::kSuccess) {
+    std::move(result_callback).Run(result_code);
+    return;
+  }
 
   std::string src_file_hash;
   if (!ComputeSHA256DigestOfPath(src_file_path, &src_file_hash)) {
     LOG(ERROR) << "Unable to hash the source file.";
-    return ZipArchiverResultCode::kErrorIO;
+    std::move(result_callback).Run(ZipArchiverResultCode::kErrorIO);
+    return;
   }
 
-  // Zip file name format: "|source basename|_|src_file_hash|.zip"
-  const base::FilePath zip_filename(
-      base::StrCat({sanitized_src_filename, L"_",
-                    base::UTF8ToUTF16(src_file_hash), L".zip"}));
+  const base::string16 zip_filename = internal::ConstructZipArchiveFileName(
+      sanitized_src_filename, src_file_hash);
   const base::FilePath zip_file_path = dst_archive_folder_.Append(zip_filename);
 
   // Fail if the zip file exists.
-  if (base::PathExists(zip_file_path))
-    return ZipArchiverResultCode::kZipFileExists;
+  if (base::PathExists(zip_file_path)) {
+    std::move(result_callback).Run(ZipArchiverResultCode::kZipFileExists);
+    return;
+  }
 
   // Create and open the zip file with minimized rights for writing.
   base::File zip_file(::CreateFile(zip_file_path.AsUTF16Unsafe().c_str(),
@@ -161,60 +194,63 @@ ZipArchiverResultCode SandboxedZipArchiver::Archive(
                                    CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr));
   if (!zip_file.IsValid()) {
     LOG(ERROR) << "Unable to create the zip file.";
-    return ZipArchiverResultCode::kErrorCannotCreateZipFile;
+    std::move(result_callback)
+        .Run(ZipArchiverResultCode::kErrorCannotCreateZipFile);
+    return;
   }
 
   const std::string filename_in_zip = base::UTF16ToUTF8(sanitized_src_filename);
-  ZipArchiverResultCode result_code =
-      DoArchive(std::move(src_file), std::move(zip_file), filename_in_zip);
-  if (result_code != ZipArchiverResultCode::kSuccess) {
-    // The |zip_file| has been closed when returned from the scope of
-    // |DoArchive|. Delete the incomplete zip file directly.
-    if (!base::DeleteFile(zip_file_path, /*recursive=*/false))
-      LOG(ERROR) << "Failed to delete the incomplete zip file.";
-
-    return result_code;
-  }
-
-  *output_zip_file_path = zip_file_path;
-  return ZipArchiverResultCode::kSuccess;
-}
-
-ZipArchiverResultCode SandboxedZipArchiver::DoArchive(
-    base::File src_file,
-    base::File zip_file,
-    const std::string& filename_in_zip) {
-  ZipArchiverResultCode result_code;
-  base::WaitableEvent waitable_event(
-      base::WaitableEvent::ResetPolicy::MANUAL,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
+  // Do archive.
   // Unretained pointer of |zip_archiver_ptr_| is safe because its deleter
   // is run on the same task runner. If |zip_archiver_ptr_| is destructed later,
   // the deleter will be scheduled after this task.
+  auto done_callback =
+      base::BindOnce(OnArchiveDone, zip_file_path, std::move(result_callback));
   mojo_task_runner_->PostTask(
       FROM_HERE,
-      base::BindOnce(
-          RunArchiver, base::Unretained(zip_archiver_ptr_.get()),
-          mojo::WrapPlatformFile(src_file.TakePlatformFile()),
-          mojo::WrapPlatformFile(zip_file.TakePlatformFile()), filename_in_zip,
-          zip_password_,
-          base::BindOnce(OnArchiveDone, &result_code, &waitable_event)));
-
-  waitable_event.Wait();
-  return result_code;
+      base::BindOnce(RunArchiver, base::Unretained(zip_archiver_ptr_.get()),
+                     mojo::WrapPlatformFile(src_file.TakePlatformFile()),
+                     mojo::WrapPlatformFile(zip_file.TakePlatformFile()),
+                     filename_in_zip, zip_password_, std::move(done_callback)));
+}
+
+ZipArchiverResultCode SandboxedZipArchiver::CheckFileSize(base::File* file) {
+  const int64_t file_size = file->GetLength();
+  if (file_size == -1) {
+    LOG(ERROR) << "Unable to get the file size.";
+    return ZipArchiverResultCode::kErrorIO;
+  }
+  if (file_size > kQuarantineSourceSizeLimit) {
+    LOG(ERROR) << "Source file is too big.";
+    return ZipArchiverResultCode::kErrorSourceFileTooBig;
+  }
+
+  const int64_t dst_disk_space =
+      base::SysInfo::AmountOfFreeDiskSpace(dst_archive_folder_);
+  if (dst_disk_space == -1) {
+    LOG(ERROR) << "Unable to get the free disk space.";
+    return ZipArchiverResultCode::kErrorIO;
+  }
+  if (file_size + kZipAdditionalSize > dst_disk_space) {
+    LOG(ERROR) << "Not enough disk space.";
+    return ZipArchiverResultCode::kErrorNotEnoughDiskSpace;
+  }
+
+  return ZipArchiverResultCode::kSuccess;
 }
 
 ResultCode SpawnZipArchiverSandbox(
     const base::FilePath& dst_archive_folder,
     const std::string& zip_password,
     scoped_refptr<MojoTaskRunner> mojo_task_runner,
-    base::OnceClosure connection_error_handler,
+    const SandboxConnectionErrorCallback& connection_error_callback,
     std::unique_ptr<SandboxedZipArchiver>* sandboxed_zip_archiver) {
-  ZipArchiverSandboxSetupHooks setup_hooks(mojo_task_runner,
-                                           std::move(connection_error_handler));
   DCHECK(sandboxed_zip_archiver);
 
+  auto error_handler =
+      base::BindOnce(connection_error_callback, SandboxType::kZipArchiver);
+  ZipArchiverSandboxSetupHooks setup_hooks(mojo_task_runner,
+                                           std::move(error_handler));
   ResultCode result_code =
       SpawnSandbox(&setup_hooks, SandboxType::kZipArchiver);
   if (result_code == RESULT_CODE_SUCCESS) {

chrome/chrome_cleaner/zip_archiver/sandboxed_zip_archiver.h

@@ -15,25 +15,34 @@
 #include "base/sequenced_task_runner.h"
 #include "chrome/chrome_cleaner/interfaces/zip_archiver.mojom.h"
 #include "chrome/chrome_cleaner/ipc/mojo_task_runner.h"
+#include "chrome/chrome_cleaner/ipc/sandbox.h"
 #include "chrome/chrome_cleaner/zip_archiver/broker/sandbox_setup.h"
 
 namespace chrome_cleaner {
 
+namespace internal {
+
+base::string16 ConstructZipArchiveFileName(const base::string16& filename,
+                                           const std::string& file_hash);
+
+}  // namespace internal
+
 class SandboxedZipArchiver {
  public:
+  using ArchiveResultCallback =
+      base::OnceCallback<void(mojom::ZipArchiverResultCode)>;
+
   SandboxedZipArchiver(scoped_refptr<MojoTaskRunner> mojo_task_runner,
                        UniqueZipArchiverPtr zip_archiver_ptr,
                        const base::FilePath& dst_archive_folder,
                        const std::string& zip_password);
   ~SandboxedZipArchiver();
 
-  mojom::ZipArchiverResultCode Archive(const base::FilePath& src_file_path,
-                                       base::FilePath* output_zip_file_path);
+  void Archive(const base::FilePath& src_file_path,
+               ArchiveResultCallback result_callback);
 
  private:
-  mojom::ZipArchiverResultCode DoArchive(base::File src_file,
-                                         base::File zip_file,
-                                         const std::string& filename_in_zip);
+  mojom::ZipArchiverResultCode CheckFileSize(base::File* file);
 
   scoped_refptr<MojoTaskRunner> mojo_task_runner_;
   UniqueZipArchiverPtr zip_archiver_ptr_;
@@ -45,7 +54,7 @@ ResultCode SpawnZipArchiverSandbox(
     const base::FilePath& dst_archive_folder,
     const std::string& zip_password,
     scoped_refptr<MojoTaskRunner> mojo_task_runner,
-    base::OnceClosure connection_error_handler,
+    const SandboxConnectionErrorCallback& connection_error_callback,
     std::unique_ptr<SandboxedZipArchiver>* sandboxed_zip_archiver);
 
 }  // namespace chrome_cleaner

chrome/chrome_cleaner/zip_archiver/sandboxed_zip_archiver_unittest.cc

@@ -9,12 +9,13 @@
 #include "base/files/file.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/strcat.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/test/multiprocess_test.h"
 #include "base/test/scoped_task_environment.h"
 #include "base/win/scoped_handle.h"
+#include "chrome/chrome_cleaner/constants/quarantine_constants.h"
 #include "chrome/chrome_cleaner/interfaces/zip_archiver.mojom.h"
 #include "chrome/chrome_cleaner/ipc/mojo_task_runner.h"
 #include "chrome/chrome_cleaner/os/disk_util.h"
@@ -42,6 +43,13 @@ constexpr uint32_t kProhibitedAccessPermissions[] = {
     WRITE_OWNER,  FILE_APPEND_DATA, FILE_EXECUTE,
     FILE_READ_EA, FILE_WRITE_EA,    FILE_WRITE_ATTRIBUTES};
 
+void OnArchiveDone(ZipArchiverResultCode* test_result_code,
+                   base::OnceClosure done_callback,
+                   ZipArchiverResultCode result_code) {
+  *test_result_code = result_code;
+  std::move(done_callback).Run();
+}
+
 class ZipArchiverSandboxedArchiverTest : public base::MultiProcessTest {
  public:
   void SetUp() override {
@@ -61,11 +69,10 @@ class ZipArchiverSandboxedArchiverTest : public base::MultiProcessTest {
     std::string src_file_hash;
     ComputeSHA256DigestOfPath(src_file_path, &src_file_hash);
 
-    const base::FilePath& dst_archive_folder = test_file_.GetTempDirPath();
-    base::FilePath zip_filename(
-        base::StrCat({test_file_.GetSourceFilePath().BaseName().AsUTF16Unsafe(),
-                      L"_", base::UTF8ToUTF16(src_file_hash), L".zip"}));
+    const base::string16 zip_filename = internal::ConstructZipArchiveFileName(
+        src_file_path.BaseName().value(), src_file_hash);
 
+    const base::FilePath& dst_archive_folder = test_file_.GetTempDirPath();
     expect_zip_file_path_ = dst_archive_folder.Append(zip_filename);
 
     zip_archiver_ = std::make_unique<SandboxedZipArchiver>(
@@ -74,30 +81,42 @@ class ZipArchiverSandboxedArchiverTest : public base::MultiProcessTest {
   }
 
  protected:
-  std::unique_ptr<SandboxedZipArchiver> zip_archiver_;
+  ZipArchiverResultCode Archive(const base::FilePath& path) {
+    ZipArchiverResultCode result_code;
+    base::RunLoop loop;
+    // Unretained pointer is safe here since we will wait for the task.
+    zip_archiver_->Archive(
+        path, base::BindOnce(OnArchiveDone, base::Unretained(&result_code),
+                             loop.QuitClosure()));
+    loop.Run();
+    return result_code;
+  }
+
   ZipArchiverTestFile test_file_;
   base::FilePath expect_zip_file_path_;
 
  private:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<SandboxedZipArchiver> zip_archiver_;
 };
 
-// |ArchiverPermissionCheckerImpl| runs and handles |Archive| requests in the
+// ArgumentVerifyingFakeArchiver runs and handles Archive requests in the
 // sandbox child process. It checks if the parameters passed in the sandbox are
-// configured correctly. It doesn't do real archiving.
-class ArchiverPermissionCheckerImpl : public mojom::ZipArchiver {
+// configured correctly and if some checks are done in SandboxedZipArchiver
+// before sending requests to the sandbox. It doesn't do real archiving.
+class ArgumentVerifyingFakeArchiver : public mojom::ZipArchiver {
  public:
-  explicit ArchiverPermissionCheckerImpl(mojom::ZipArchiverRequest request)
+  explicit ArgumentVerifyingFakeArchiver(mojom::ZipArchiverRequest request)
       : binding_(this, std::move(request)) {
     binding_.set_connection_error_handler(base::BindOnce(
         [] { FAIL() << "ZipArchiver sandbox connection error"; }));
   }
 
-  ~ArchiverPermissionCheckerImpl() override = default;
+  ~ArgumentVerifyingFakeArchiver() override = default;
 
   void Archive(mojo::ScopedHandle src_file_handle,
                mojo::ScopedHandle zip_file_handle,
-               const std::string& filename,
+               const std::string& filename_in_zip,
                const std::string& password,
                ArchiveCallback callback) override {
     HANDLE raw_src_file_handle;
@@ -112,6 +131,12 @@ class ArchiverPermissionCheckerImpl : public mojom::ZipArchiver {
       return;
     }
 
+    // Test if the size of the source file has been checked.
+    if (src_file.GetLength() > kQuarantineSourceSizeLimit) {
+      std::move(callback).Run(ZipArchiverResultCode::kErrorInvalidParameter);
+      return;
+    }
+
     HANDLE raw_zip_file_handle;
     if (mojo::UnwrapPlatformFile(std::move(zip_file_handle),
                                  &raw_zip_file_handle) != MOJO_RESULT_OK) {
@@ -158,7 +183,7 @@ class ArchiverPermissionCheckerImpl : public mojom::ZipArchiver {
 
   mojo::Binding<mojom::ZipArchiver> binding_;
 
-  DISALLOW_COPY_AND_ASSIGN(ArchiverPermissionCheckerImpl);
+  DISALLOW_COPY_AND_ASSIGN(ArgumentVerifyingFakeArchiver);
 };
 
 }  // namespace
@@ -177,12 +202,9 @@ MULTIPROCESS_TEST_MAIN(SandboxedZipArchiverTargetMain) {
 }
 
 TEST_F(ZipArchiverSandboxedArchiverTest, Archive) {
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(test_file_.GetSourceFilePath(),
-                                   &output_zip_file_path),
-            ZipArchiverResultCode::kSuccess);
+  EXPECT_EQ(ZipArchiverResultCode::kSuccess,
+            Archive(test_file_.GetSourceFilePath()));
 
-  EXPECT_EQ(output_zip_file_path, expect_zip_file_path_);
   test_file_.ExpectValidZipFile(
       expect_zip_file_path_,
       test_file_.GetSourceFilePath().BaseName().AsUTF8Unsafe(), kTestPassword);
@@ -191,20 +213,16 @@ TEST_F(ZipArchiverSandboxedArchiverTest, Archive) {
 TEST_F(ZipArchiverSandboxedArchiverTest, SourceFileNotFound) {
   ASSERT_TRUE(base::DeleteFile(test_file_.GetSourceFilePath(), false));
 
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(test_file_.GetSourceFilePath(),
-                                   &output_zip_file_path),
-            ZipArchiverResultCode::kErrorCannotOpenSourceFile);
+  EXPECT_EQ(ZipArchiverResultCode::kErrorCannotOpenSourceFile,
+            Archive(test_file_.GetSourceFilePath()));
 }
 
 TEST_F(ZipArchiverSandboxedArchiverTest, ZipFileExists) {
   base::File zip_file(expect_zip_file_path_, base::File::FLAG_CREATE);
   ASSERT_TRUE(zip_file.IsValid());
 
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(test_file_.GetSourceFilePath(),
-                                   &output_zip_file_path),
-            ZipArchiverResultCode::kZipFileExists);
+  EXPECT_EQ(ZipArchiverResultCode::kZipFileExists,
+            Archive(test_file_.GetSourceFilePath()));
 }
 
 TEST_F(ZipArchiverSandboxedArchiverTest, SourceIsSymbolicLink) {
@@ -214,27 +232,20 @@ TEST_F(ZipArchiverSandboxedArchiverTest, SourceIsSymbolicLink) {
       symlink_path.AsUTF16Unsafe().c_str(),
       test_file_.GetSourceFilePath().AsUTF16Unsafe().c_str(), 0));
 
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(symlink_path, &output_zip_file_path),
-            ZipArchiverResultCode::kIgnoredSourceFile);
+  EXPECT_EQ(ZipArchiverResultCode::kIgnoredSourceFile, Archive(symlink_path));
 }
 
 TEST_F(ZipArchiverSandboxedArchiverTest, SourceIsDirectory) {
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(test_file_.GetTempDirPath(),
-                                   &output_zip_file_path),
-            ZipArchiverResultCode::kIgnoredSourceFile);
+  EXPECT_EQ(ZipArchiverResultCode::kIgnoredSourceFile,
+            Archive(test_file_.GetTempDirPath()));
 }
 
 TEST_F(ZipArchiverSandboxedArchiverTest, SourceIsDefaultFileStream) {
   base::FilePath stream_path(base::StrCat(
       {test_file_.GetSourceFilePath().AsUTF16Unsafe(), L"::$data"}));
 
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(stream_path, &output_zip_file_path),
-            ZipArchiverResultCode::kSuccess);
+  EXPECT_EQ(ZipArchiverResultCode::kSuccess, Archive(stream_path));
 
-  EXPECT_EQ(output_zip_file_path, expect_zip_file_path_);
   test_file_.ExpectValidZipFile(
       expect_zip_file_path_,
       test_file_.GetSourceFilePath().BaseName().AsUTF8Unsafe(), kTestPassword);
@@ -246,18 +257,30 @@ TEST_F(ZipArchiverSandboxedArchiverTest, SourceIsNonDefaultFileStream) {
   base::File stream_file(stream_path, base::File::FLAG_CREATE);
   ASSERT_TRUE(stream_file.IsValid());
 
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(stream_path, &output_zip_file_path),
-            ZipArchiverResultCode::kIgnoredSourceFile);
+  EXPECT_EQ(ZipArchiverResultCode::kIgnoredSourceFile, Archive(stream_path));
+}
+
+TEST_F(ZipArchiverSandboxedArchiverTest, ArchiveOpenedFileWithSharingAccess) {
+  const base::FilePath path = test_file_.GetSourceFilePath();
+  base::File opened_file(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                                   base::File::FLAG_WRITE |
+                                   base::File::FLAG_DELETE_ON_CLOSE);
+  ASSERT_TRUE(opened_file.IsValid());
+
+  EXPECT_EQ(ZipArchiverResultCode::kSuccess, Archive(path));
+
+  test_file_.ExpectValidZipFile(expect_zip_file_path_,
+                                path.BaseName().AsUTF8Unsafe(), kTestPassword);
 }
 
 namespace {
 
-// |ZipArchiverIsolationTest| uses |ArchiverPermissionCheckerImpl| to check the
-// sandbox configuration.
-class ZipArchiverIsolationTest : public base::MultiProcessTest {
+// ZipArchiverSandboxCheckTest uses ArgumentVerifyingFakeArchiver to check the
+// sandbox configuration and if some checks are done in SandboxedZipArchiver
+// before sending requests to the sandbox.
+class ZipArchiverSandboxCheckTest : public base::MultiProcessTest {
  public:
-  ZipArchiverIsolationTest()
+  ZipArchiverSandboxCheckTest()
       : mojo_task_runner_(MojoTaskRunner::Create()),
         impl_ptr_(nullptr, base::OnTaskRunnerDeleter(mojo_task_runner_)) {
     UniqueZipArchiverPtr zip_archiver_ptr(
@@ -267,11 +290,10 @@ class ZipArchiverIsolationTest : public base::MultiProcessTest {
     // Initialize the |impl_ptr_| in the mojo task and wait until it completed.
     base::RunLoop loop;
     mojo_task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &ZipArchiverIsolationTest::InitializeArchiverPermissionCheckerImpl,
-            base::Unretained(this), zip_archiver_ptr.get(),
-            loop.QuitClosure()));
+        FROM_HERE, base::BindOnce(&ZipArchiverSandboxCheckTest::
+                                      InitializeArgumentVerifyingFakeArchiver,
+                                  base::Unretained(this),
+                                  zip_archiver_ptr.get(), loop.QuitClosure()));
     loop.Run();
 
     test_file_.Initialize();
@@ -282,31 +304,56 @@ class ZipArchiverIsolationTest : public base::MultiProcessTest {
   }
 
  protected:
-  std::unique_ptr<SandboxedZipArchiver> zip_archiver_;
+  ZipArchiverResultCode Archive(const base::FilePath& path) {
+    ZipArchiverResultCode result_code;
+    base::RunLoop loop;
+    // Unretained pointer is safe here since we will wait for the task.
+    zip_archiver_->Archive(
+        path, base::BindOnce(OnArchiveDone, base::Unretained(&result_code),
+                             loop.QuitClosure()));
+    loop.Run();
+    return result_code;
+  }
+
   ZipArchiverTestFile test_file_;
 
  private:
-  void InitializeArchiverPermissionCheckerImpl(
+  void InitializeArgumentVerifyingFakeArchiver(
       mojom::ZipArchiverPtr* zip_archiver_ptr,
       base::OnceClosure callback) {
     impl_ptr_.reset(
-        new ArchiverPermissionCheckerImpl(mojo::MakeRequest(zip_archiver_ptr)));
+        new ArgumentVerifyingFakeArchiver(mojo::MakeRequest(zip_archiver_ptr)));
     std::move(callback).Run();
   }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   scoped_refptr<MojoTaskRunner> mojo_task_runner_;
-  std::unique_ptr<ArchiverPermissionCheckerImpl, base::OnTaskRunnerDeleter>
+  std::unique_ptr<SandboxedZipArchiver> zip_archiver_;
+  std::unique_ptr<ArgumentVerifyingFakeArchiver, base::OnTaskRunnerDeleter>
       impl_ptr_;
 };
 
 }  // namespace
 
-TEST_F(ZipArchiverIsolationTest, CheckPermission) {
-  base::FilePath output_zip_file_path;
-  EXPECT_EQ(zip_archiver_->Archive(test_file_.GetSourceFilePath(),
-                                   &output_zip_file_path),
-            ZipArchiverResultCode::kSuccess);
+TEST_F(ZipArchiverSandboxCheckTest, CheckPermission) {
+  // Let ArgumentVerifyingFakeArchiver check if file handles are opened with
+  // correct permissions.
+  EXPECT_EQ(ZipArchiverResultCode::kSuccess,
+            Archive(test_file_.GetSourceFilePath()));
+}
+
+TEST_F(ZipArchiverSandboxCheckTest, SourceIsTooBig) {
+  const base::FilePath path = test_file_.GetSourceFilePath();
+  base::File src_file(path, base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+  ASSERT_TRUE(src_file.IsValid());
+  // Increase the size of source file to one byte more than limit.
+  ASSERT_TRUE(src_file.SetLength(kQuarantineSourceSizeLimit + 1));
+  src_file.Close();
+
+  // Expect SandboxedZipArchiver to check file size before sending a request to
+  // ArgumentVerifyingFakeArchiver. ArgumentVerifyingFakeArchiver will return
+  // kErrorInvalidParameter if the file size isn't checked first.
+  EXPECT_EQ(ZipArchiverResultCode::kErrorSourceFileTooBig, Archive(path));
 }
 
 }  // namespace chrome_cleaner