[NativeFS] Change hash calculation code to go through FS backends.

This makes sure this code works for non-native file systems.

Bug: 1103076
Change-Id: I1579fd33cd40b6527504a89987ba8dbc9b3c8931
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2304958
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791250}

content/browser/native_file_system/native_file_system_file_writer_impl.cc

@@ -16,6 +16,7 @@
 #include "crypto/secure_hash.h"
 #include "mojo/public/cpp/bindings/callback_helpers.h"
 #include "storage/browser/blob/blob_storage_context.h"
+#include "storage/browser/file_system/file_stream_reader.h"
 #include "storage/browser/file_system/file_system_operation_runner.h"
 #include "third_party/blink/public/common/blob/blob_utils.h"
 #include "third_party/blink/public/mojom/blob/blob.mojom.h"
@@ -26,48 +27,107 @@ using storage::BlobDataHandle;
 using storage::FileSystemOperation;
 using storage::FileSystemOperationRunner;
 
+namespace content {
+
 namespace {
 
 // For after write checks we need the hash and size of the file. That data is
-// calculated on a worker thread, and this struct is used to pass it back.
-struct HashResult {
-  base::File::Error status;
-  // SHA256 hash of the file contents, an empty string if some error occurred.
-  std::string hash;
-  // Can be -1 to indicate an error calculating the hash and/or size.
-  int64_t file_size = -1;
-};
+// calculated on the IO thread by this class.
+// This class is ref-counted to make it easier to integrate with the
+// FileStreamReader API where methods either return synchronously or invoke
+// their callback asynchronously.
+class HashCalculator : public base::RefCounted<HashCalculator> {
+ public:
+  // Must be called on the FileSystemContext's IO runner.
+  static void CreateAndStart(
+      scoped_refptr<storage::FileSystemContext> context,
+      NativeFileSystemFileWriterImpl::HashCallback callback,
+      const storage::FileSystemURL& swap_url,
+      storage::FileSystemOperationRunner*) {
+    auto calculator = base::MakeRefCounted<HashCalculator>(std::move(context),
+                                                           std::move(callback));
+    calculator->Start(swap_url);
+  }
+
+  HashCalculator(scoped_refptr<storage::FileSystemContext> context,
+                 NativeFileSystemFileWriterImpl::HashCallback callback)
+      : context_(std::move(context)), callback_(std::move(callback)) {
+    DCHECK(context_);
+  }
+
+ private:
+  friend class base::RefCounted<HashCalculator>;
+  ~HashCalculator() = default;
+
+  void Start(const storage::FileSystemURL& swap_url) {
+    reader_ = context_->CreateFileStreamReader(
+        swap_url, 0, storage::kMaximumLength, base::Time());
+    int64_t length =
+        reader_->GetLength(base::BindOnce(&HashCalculator::GotLength, this));
+    if (length == net::ERR_IO_PENDING)
+      return;
+    GotLength(length);
+  }
 
-HashResult ReadAndComputeSHA256ChecksumAndSize(const base::FilePath& path) {
-  base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  void GotLength(int64_t length) {
+    if (length < 0) {
+      std::move(callback_).Run(storage::NetErrorToFileError(length),
+                               std::string(), -1);
+      return;
+    }
 
-  if (!file.IsValid())
-    return {file.error_details(), std::string(), -1};
+    file_size_ = length;
+    ReadMore();
+  }
 
-  std::unique_ptr<crypto::SecureHash> hash =
-      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
-  std::vector<char> buffer(8 * 1024);
-  int bytes_read = file.ReadAtCurrentPos(buffer.data(), buffer.size());
+  void ReadMore() {
+    DCHECK_GE(file_size_, 0);
+    int read_result =
+        reader_->Read(buffer_.get(), buffer_->size(),
+                      base::BindOnce(&HashCalculator::DidRead, this));
+    if (read_result == net::ERR_IO_PENDING)
+      return;
+    DidRead(read_result);
+  }
 
-  while (bytes_read > 0) {
-    hash->Update(buffer.data(), bytes_read);
-    bytes_read = file.ReadAtCurrentPos(buffer.data(), buffer.size());
+  void DidRead(int bytes_read) {
+    DCHECK_GE(file_size_, 0);
+    if (bytes_read < 0) {
+      std::move(callback_).Run(storage::NetErrorToFileError(bytes_read),
+                               std::string(), -1);
+      return;
+    }
+    if (bytes_read == 0) {
+      std::string hash_str(hash_->GetHashLength(), 0);
+      hash_->Finish(base::data(hash_str), hash_str.size());
+      std::move(callback_).Run(base::File::FILE_OK, hash_str, file_size_);
+      return;
+    }
+
+    hash_->Update(buffer_->data(), bytes_read);
+    ReadMore();
   }
 
-  // If bytes_read is -ve, it means there were issues reading from disk.
-  if (bytes_read < 0)
-    return {file.error_details(), std::string(), -1};
+  const scoped_refptr<storage::FileSystemContext> context_;
+  NativeFileSystemFileWriterImpl::HashCallback callback_;
+
+  const scoped_refptr<net::IOBufferWithSize> buffer_{
+      base::MakeRefCounted<net::IOBufferWithSize>(8 * 1024)};
 
-  std::string hash_str(hash->GetHashLength(), 0);
-  hash->Finish(base::data(hash_str), hash_str.size());
+  const std::unique_ptr<crypto::SecureHash> hash_{
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256)};
 
-  return {file.error_details(), hash_str, file.GetLength()};
+  std::unique_ptr<storage::FileStreamReader> reader_;
+  int64_t file_size_ = -1;
+};
+
+void RemoveSwapFile(const storage::FileSystemURL& swap_url,
+                    storage::FileSystemOperationRunner* runner) {
+  runner->Remove(swap_url, /*recursive=*/false, base::DoNothing());
 }
 
 }  // namespace
 
-namespace content {
-
 struct NativeFileSystemFileWriterImpl::WriteState {
   WriteCallback callback;
   uint64_t bytes_written = 0;
@@ -300,13 +360,15 @@ void NativeFileSystemFileWriterImpl::CloseImpl(CloseCallback callback) {
 
   ComputeHashForSwapFile(base::BindOnce(
       &NativeFileSystemFileWriterImpl::DoAfterWriteCheck,
-      weak_factory_.GetWeakPtr(), swap_url().path(), std::move(callback)));
+      weak_factory_.GetWeakPtr(), base::WrapRefCounted(manager()), swap_url(),
+      std::move(callback)));
 }
 
 // static
 void NativeFileSystemFileWriterImpl::DoAfterWriteCheck(
     base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-    const base::FilePath& swap_path,
+    scoped_refptr<NativeFileSystemManagerImpl> manager,
+    const storage::FileSystemURL& swap_url,
     NativeFileSystemFileWriterImpl::CloseCallback callback,
     base::File::Error hash_result,
     const std::string& hash,
@@ -314,9 +376,8 @@ void NativeFileSystemFileWriterImpl::DoAfterWriteCheck(
   if (!file_writer || hash_result != base::File::FILE_OK) {
     // If writer was deleted, or calculating the hash failed try deleting the
     // swap file and invoke the callback.
-    base::ThreadPool::PostTask(
-        FROM_HERE, {base::MayBlock()},
-        base::BindOnce(base::GetDeleteFileCallback(), swap_path));
+    manager->operation_runner().PostTaskWithThisObject(
+        FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url));
     std::move(callback).Run(native_file_system_error::FromStatus(
         NativeFileSystemStatus::kOperationAborted,
         "Failed to perform Safe Browsing check."));
@@ -335,13 +396,15 @@ void NativeFileSystemFileWriterImpl::DoAfterWriteCheck(
   file_writer->manager()->permission_context()->PerformAfterWriteChecks(
       std::move(item), file_writer->context().frame_id,
       base::BindOnce(&NativeFileSystemFileWriterImpl::DidAfterWriteCheck,
-                     file_writer, swap_path, std::move(callback)));
+                     file_writer, std::move(manager), swap_url,
+                     std::move(callback)));
 }
 
 // static
 void NativeFileSystemFileWriterImpl::DidAfterWriteCheck(
     base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-    const base::FilePath& swap_path,
+    scoped_refptr<NativeFileSystemManagerImpl> manager,
+    const storage::FileSystemURL& swap_url,
     NativeFileSystemFileWriterImpl::CloseCallback callback,
     NativeFileSystemPermissionContext::AfterWriteCheckResult result) {
   if (file_writer &&
@@ -354,9 +417,8 @@ void NativeFileSystemFileWriterImpl::DidAfterWriteCheck(
   // Writer is gone, or safe browsing check failed. In this case we should
   // try deleting the swap file and call the callback to report that close
   // failed.
-  base::ThreadPool::PostTask(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(base::GetDeleteFileCallback(), swap_path));
+  manager->operation_runner().PostTaskWithThisObject(
+      FROM_HERE, base::BindOnce(&RemoveSwapFile, swap_url));
   std::move(callback).Run(native_file_system_error::FromStatus(
       NativeFileSystemStatus::kOperationAborted,
       "Write operation blocked by Safe Browsing."));
@@ -397,6 +459,19 @@ void NativeFileSystemFileWriterImpl::DidSwapFileBeforeClose(
     return;
   }
 
+  // url().path() has to make sense for quarantine to work. On ChromeOS that
+  // means it has to not be in the sandboxed file system, while on other
+  // platforms it means it has to be a local or test file.
+#if defined(OS_CHROMEOS)
+  DCHECK(url().type() != storage::kFileSystemTypeTemporary &&
+         url().type() != storage::kFileSystemTypePersistent)
+      << url().type();
+#else
+  DCHECK(url().type() == storage::kFileSystemTypeNativeLocal ||
+         url().type() == storage::kFileSystemTypeTest)
+      << url().type();
+#endif
+
   GURL referrer_url = manager()->is_off_the_record() ? GURL() : context().url;
   GURL authority_url =
       referrer_url.is_valid() && referrer_url.SchemeIsHTTPOrHTTPS()
@@ -460,24 +535,18 @@ void NativeFileSystemFileWriterImpl::ComputeHashForSwapFile(
     HashCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-#if defined(OS_CHROMEOS)
-  // TOOD(crbug.com/1103076): Extend this check to non-native paths.
-  DCHECK(swap_url().type() == storage::kFileSystemTypeNativeLocal ||
-         swap_url().type() == storage::kFileSystemTypeNativeForPlatformApp)
-      << swap_url().type();
-#else
-  DCHECK_EQ(swap_url().type(), storage::kFileSystemTypeNativeLocal);
-#endif
-
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&ReadAndComputeSHA256ChecksumAndSize, swap_url().path()),
-      base::BindOnce(
-          [](HashCallback callback, HashResult result) {
-            std::move(callback).Run(result.status, result.hash,
-                                    result.file_size);
-          },
-          std::move(callback)));
+  auto wrapped_callback = base::BindOnce(
+      [](scoped_refptr<base::SequencedTaskRunner> runner, HashCallback callback,
+         base::File::Error error, const std::string& hash, int64_t size) {
+        runner->PostTask(
+            FROM_HERE, base::BindOnce(std::move(callback), error, hash, size));
+      },
+      base::SequencedTaskRunnerHandle::Get(), std::move(callback));
+
+  manager()->operation_runner().PostTaskWithThisObject(
+      FROM_HERE, base::BindOnce(&HashCalculator::CreateAndStart,
+                                base::WrapRefCounted(file_system_context()),
+                                std::move(wrapped_callback), swap_url()));
 }
 
 base::WeakPtr<NativeFileSystemHandleBase>

content/browser/native_file_system/native_file_system_file_writer_impl.h

@@ -85,14 +85,16 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   // perform cleanup even if the writer was deleted before they were invoked.
   static void DoAfterWriteCheck(
       base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-      const base::FilePath& swap_path,
+      scoped_refptr<NativeFileSystemManagerImpl> manager,
+      const storage::FileSystemURL& swap_url,
       NativeFileSystemFileWriterImpl::CloseCallback callback,
       base::File::Error hash_result,
       const std::string& hash,
       int64_t size);
   static void DidAfterWriteCheck(
       base::WeakPtr<NativeFileSystemFileWriterImpl> file_writer,
-      const base::FilePath& swap_path,
+      scoped_refptr<NativeFileSystemManagerImpl> manager,
+      const storage::FileSystemURL& swap_url,
       NativeFileSystemFileWriterImpl::CloseCallback callback,
       NativeFileSystemPermissionContext::AfterWriteCheckResult result);
   void DidPassAfterWriteCheck(CloseCallback callback);
@@ -106,8 +108,7 @@ class CONTENT_EXPORT NativeFileSystemFileWriterImpl
   // except temporary file systems.
   // TOOD(crbug.com/1103076): Extend this check to non-native paths.
   bool RequireSecurityChecks() const {
-    return url().type() == storage::kFileSystemTypeNativeLocal ||
-           url().type() == storage::kFileSystemTypeNativeForPlatformApp;
+    return url().type() != storage::kFileSystemTypeTemporary;
   }
 
   void ComputeHashForSwapFile(HashCallback callback);

content/browser/native_file_system/native_file_system_manager_impl_unittest.cc

@@ -290,8 +290,7 @@ TEST_F(NativeFileSystemManagerImplTest,
       storage::AsyncFileTestHelper::kDontCheckSize));
 }
 
-TEST_F(NativeFileSystemManagerImplTest,
-       FileWriterCloseAllowedToCompleteOnDestruct) {
+TEST_F(NativeFileSystemManagerImplTest, FileWriterCloseAbortsOnDestruct) {
   auto test_file_url = file_system_context_->CreateCrackedFileSystemURL(
       kTestOrigin, storage::kFileSystemTypeTest,
       base::FilePath::FromUTF8Unsafe("test"));
@@ -319,11 +318,14 @@ TEST_F(NativeFileSystemManagerImplTest,
   writer_remote.reset();
   base::RunLoop().RunUntilIdle();
 
+  // Since the writer was destroyed before close completed, the swap file should
+  // have been destroyed and the target file should have been left untouched.
   ASSERT_FALSE(storage::AsyncFileTestHelper::FileExists(
       file_system_context_.get(), test_swap_url,
       storage::AsyncFileTestHelper::kDontCheckSize));
-  ASSERT_TRUE(storage::AsyncFileTestHelper::FileExists(
-      file_system_context_.get(), test_file_url, 3));
+  ASSERT_FALSE(storage::AsyncFileTestHelper::FileExists(
+      file_system_context_.get(), test_file_url,
+      storage::AsyncFileTestHelper::kDontCheckSize));
 }
 
 TEST_F(NativeFileSystemManagerImplTest, SerializeHandle_SandboxedFile) {
@@ -750,4 +752,4 @@ TEST_F(NativeFileSystemManagerImplTest,
   EXPECT_FALSE(dir_remote.is_connected());
 }
 
-}  // namespace content
\ No newline at end of file
+}  // namespace content