From c25d7e463bd16180c398a38f517724d488bf55ce Mon Sep 17 00:00:00 2001
From: Anna Tsvirchkova <[email protected]>
Date: Tue, 11 Aug 2026 00:53:17 -0700
Subject: [PATCH] [Gardener] Revert "blob: Harden timestamp validation in BlobTransport"

This reverts commit 68f2bf3d56f5f7883153ec00c9992fff298be425.

Reason for revert: BlobStorageContextMojoTest#WriteSingleFileBlobNoFileSize and 1 more test have become extremely flaky. See b/544855147

Failure Link: https://ci.chromium.org/ui/p/chromium/builders/ci/android-14-x64-rel/12455/overview

Original change's description:
> blob: Harden timestamp validation in BlobTransport
>
> Revert commit 966e352a7bb4759ac48058acf014c6d77ca1305c which caused
> a severe P1 performance regression by streaming bytes over a data
> pipe instead of passing the file handle.
>
> Instead, implement an alternative lightweight security fix:
> 1. FileTransportStrategy::OnReply explicitly rejects null timestamps.
> 2. FileStreamReader::VerifySnapshotTime treats null expected
>    modification times as validation failures.
>
> The diff can be seen between PS 1 and 8.
>
> Bug: 497574154, 542909707
> Change-Id: I9e0ea30b1836c88991fed0ba33a3ec48f8cf0dba
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8213659
> Reviewed-by: Rakina Zata Amni <[email protected]>
> Commit-Queue: Eriko Kurimoto <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1676348}

Bug: 497574154, 542909707, 544855147
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 497574154, 542909707
Change-Id: If6e7a1693ee01bf0d43c4cc6bb3944bc7f96f1e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8235080
Bot-Commit: [email protected] <[email protected]>
Owners-Override: Anna Tsvirchkova <[email protected]>
Auto-Submit: Anna Tsvirchkova <[email protected]>
Commit-Queue: [email protected] <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1677023}
---

diff --git a/storage/browser/blob/blob_memory_controller.cc b/storage/browser/blob/blob_memory_controller.cc
index 08004eb..575f84d2 100644
--- a/storage/browser/blob/blob_memory_controller.cc
+++ b/storage/browser/blob/blob_memory_controller.cc
@@ -176,11 +176,7 @@
   for (const base::FilePath& file_path : file_paths) {
     FileCreationInfo creation_info;
     // Try to open our file.
-    uint32_t flags = File::FLAG_CREATE_ALWAYS | File::FLAG_WRITE;
-
-    // This File may be passed to an untrusted process.
-    flags = base::File::AddFlagsForPassingToUntrustedProcess(flags);
-    File file(file_path, flags);
+    File file(file_path, File::FLAG_CREATE_ALWAYS | File::FLAG_WRITE);
     creation_info.path = std::move(file_path);
     creation_info.file_deletion_runner = file_task_runner;
     creation_info.error = file.error_details();
diff --git a/storage/browser/blob/blob_registry_impl_unittest.cc b/storage/browser/blob/blob_registry_impl_unittest.cc
index 208fd1e..b2d4fca4 100644
--- a/storage/browser/blob/blob_registry_impl_unittest.cc
+++ b/storage/browser/blob/blob_registry_impl_unittest.cc
@@ -903,8 +903,8 @@
   size_t expected_file_count =
       1 + kData.size() / kTestBlobStorageMaxFileSizeBytes;
   EXPECT_EQ(0u, reply_request_count_);
-  EXPECT_EQ(0u, stream_request_count_);
-  EXPECT_EQ(expected_file_count, file_request_count_);
+  EXPECT_EQ(1u, stream_request_count_);
+  EXPECT_EQ(0u, file_request_count_);
 
   auto snapshot = handle->CreateSnapshot();
   EXPECT_EQ(expected_file_count, snapshot->items().size());
diff --git a/storage/browser/blob/blob_transport_strategy.cc b/storage/browser/blob/blob_transport_strategy.cc
index 1728d317..1ed415ac 100644
--- a/storage/browser/blob/blob_transport_strategy.cc
+++ b/storage/browser/blob/blob_transport_strategy.cc
@@ -9,12 +9,16 @@
 #include "base/check.h"
 #include "base/check_op.h"
 #include "base/containers/circular_deque.h"
+#include "base/files/file.h"
 #include "base/functional/bind.h"
 #include "base/logging.h"
 #include "base/memory/raw_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/notreached.h"
 #include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
 #include "mojo/public/cpp/system/data_pipe.h"
+#include "mojo/public/cpp/system/simple_watcher.h"
 #include "storage/browser/blob/blob_data_builder.h"
 #include "third_party/blink/public/mojom/blob/data_element.mojom.h"
 
@@ -255,116 +259,297 @@
   size_t current_source_offset_ = 0;
 };
 
-// Transport strategy that requests all data through files.
+// Transport strategy that stores all data in page files. Bytes are streamed
+// over data pipes and written to the files locally so that the page files
+// remain private to this process.
 class FileTransportStrategy : public BlobTransportStrategy {
  public:
   FileTransportStrategy(BlobDataBuilder* builder,
                         ResultCallback result_callback,
                         const BlobStorageLimits& limits)
       : BlobTransportStrategy(builder, std::move(result_callback)),
-        limits_(limits) {}
+        limits_(limits),
+        reply_runner_(base::SequencedTaskRunner::GetCurrentDefault()),
+        file_runner_(base::ThreadPool::CreateSequencedTaskRunner(
+            {base::MayBlock(), base::TaskPriority::USER_VISIBLE})) {}
+
+  ~FileTransportStrategy() override {
+    if (!files_.empty()) {
+      file_runner_->PostTask(
+          FROM_HERE,
+          base::BindOnce([](std::vector<base::File>) {}, std::move(files_)));
+    }
+  }
 
   void AddBytesElement(
       blink::mojom::DataElementBytes* bytes,
       const mojo::Remote<blink::mojom::BytesProvider>& data) override {
+    if (bytes->length == 0) {
+      return;
+    }
+    Element element;
+    element.provider = data.get();
+    element.length = bytes->length;
     uint64_t source_offset = 0;
     while (source_offset < bytes->length) {
-      if (current_file_size_ >= limits_.max_file_size ||
-          file_requests_.empty()) {
+      if (current_file_size_ >= limits_.max_file_size || file_count_ == 0) {
         current_file_size_ = 0;
-        current_file_index_++;
-        file_requests_.push_back(std::vector<Request>());
+        file_count_++;
       }
 
       // Make sure no single file gets too big, but do use up all the available
       // space in all but the last file.
-      uint64_t element_size =
+      uint64_t segment_size =
           std::min(bytes->length - source_offset,
                    limits_.max_file_size - current_file_size_);
-      BlobDataBuilder::FutureFile future_file = builder_->AppendFutureFile(
-          current_file_size_, element_size, file_requests_.size() - 1);
+      element.future_files.push_back(builder_->AppendFutureFile(
+          current_file_size_, segment_size, file_count_ - 1));
+      element.segments.push_back(
+          Segment{file_count_ - 1, current_file_size_, segment_size});
 
-      num_unresolved_requests_++;
-      file_requests_.back().push_back(Request{
-          data.get(), source_offset, element_size, std::move(future_file)});
-
-      source_offset += element_size;
-      current_file_size_ += element_size;
+      source_offset += segment_size;
+      current_file_size_ += segment_size;
     }
+    elements_.push_back(std::move(element));
   }
 
   void BeginTransport(
       std::vector<BlobMemoryController::FileCreationInfo> file_infos) override {
-    if (file_requests_.empty()) {
+    if (elements_.empty()) {
       std::move(result_callback_).Run(BlobStatus::DONE);
       return;
     }
-    DCHECK_EQ(file_infos.size(), file_requests_.size());
-    for (size_t file_index = 0; file_index < file_requests_.size();
-         ++file_index) {
-      auto& requests = file_requests_[file_index];
-      uint64_t file_offset = 0;
-      for (size_t i = 0; i < requests.size(); ++i) {
-        auto& request = requests[i];
-        base::File file = i == requests.size() - 1
-                              ? std::move(file_infos[file_index].file)
-                              : file_infos[file_index].file.Duplicate();
-        // base::Unretained is safe because |this| is guaranteed (by the
-        // contract that code using BlobTransportStrategy should adhere to) to
-        // outlive the BytesProvider.
-        request.provider->RequestAsFile(
-            request.source_offset, request.source_size, std::move(file),
-            file_offset,
-            base::BindOnce(&FileTransportStrategy::OnReply,
-                           base::Unretained(this),
-                           std::move(request.future_file),
-                           file_infos[file_index].file_reference));
-        file_offset += request.source_size;

diff --git a/storage/browser/blob/blob_registry_impl_unittest.cc b/storage/browser/blob/blob_registry_impl_unittest.cc
index 208fd1e..b2d4fca4 100644
--- a/storage/browser/blob/blob_registry_impl_unittest.cc
+++ b/storage/browser/blob/blob_registry_impl_unittest.cc
@@ -903,8 +903,8 @@
   size_t expected_file_count =
       1 + kData.size() / kTestBlobStorageMaxFileSizeBytes;
   EXPECT_EQ(0u, reply_request_count_);
-  EXPECT_EQ(0u, stream_request_count_);
-  EXPECT_EQ(expected_file_count, file_request_count_);
+  EXPECT_EQ(1u, stream_request_count_);
+  EXPECT_EQ(0u, file_request_count_);
 
   auto snapshot = handle->CreateSnapshot();
   EXPECT_EQ(expected_file_count, snapshot->items().size());
diff --git a/storage/browser/blob/blob_transport_strategy_unittest.cc b/storage/browser/blob/blob_transport_strategy_unittest.cc
index 1626140c..e2698c7 100644
--- a/storage/browser/blob/blob_transport_strategy_unittest.cc
+++ b/storage/browser/blob/blob_transport_strategy_unittest.cc
@@ -366,6 +366,64 @@
   EXPECT_TRUE(bad_messages_.empty());
 }
 
+TEST_F(BlobTransportStrategyTest, Files_PageFileNotSharedWithProvider) {
+  BlobDataBuilder builder(kId);
+
+  std::string data = base::RandBytesAsString(kTestBlobStorageMaxFileSizeBytes);
+  blink::mojom::DataElementBytes bytes(data.size(), std::nullopt,
+                                       mojo::NullRemote());
+
+  // Must outlive `strategy`.
+  mojo::Remote<blink::mojom::BytesProvider> bytes_provider(
+      CreateBytesProvider(data, mock_time_));
+
+  base::RunLoop loop;
+  BlobStatus status = BlobStatus::PENDING_TRANSPORT;
+  auto strategy = BlobTransportStrategy::Create(
+      MemoryStrategy::FILE, &builder,
+      base::BindOnce(
+          [](BlobStatus* result_out, base::OnceClosure closure,
+             BlobStatus result) {
+            *result_out = result;
+            std::move(closure).Run();
+          },
+          &status, loop.QuitClosure()),
+      limits_);
+
+  strategy->AddBytesElement(&bytes, bytes_provider);
+
+  base::FilePath path;
+  FileInfoVector files(1);
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::CreateTemporaryFileInDir(data_dir_.GetPath(), &path));
+    files[0].file =
+        base::File(path, base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+    files[0].file_deletion_runner =
+        base::SingleThreadTaskRunner::GetCurrentDefault();
+    files[0].file_reference = ShareableFileReference::GetOrCreate(
+        path, ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+        bytes_provider_runner_.get());
+  }
+
+  strategy->BeginTransport(std::move(files));
+  loop.Run();
+
+  EXPECT_EQ(BlobStatus::DONE, status);
+  EXPECT_TRUE(bad_messages_.empty());
+
+  // Page files back blob data that must remain immutable once transport
+  // completes, so the provider must never be given a handle to them.
+  EXPECT_EQ(0u, file_request_count_);
+
+  std::string file_contents;
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::ReadFileToString(path, &file_contents));
+  }
+  EXPECT_EQ(data, file_contents);
+}
+
 TEST_F(BlobTransportStrategyTest, Files_WriteFailed) {
   BlobDataBuilder builder(kId);
 
@@ -374,9 +432,10 @@
   blink::mojom::DataElementBytes bytes(data.size(), std::nullopt,
                                        mojo::NullRemote());
 
-  // Must outlive `strategy`.
+  // Must outlive `strategy`. Supplies fewer bytes than the element declares so
+  // that the page file cannot be fully written.
   mojo::Remote<blink::mojom::BytesProvider> bytes_provider(
-      CreateBytesProvider(data, std::nullopt));
+      CreateBytesProvider(data.substr(0, data.size() - 1), mock_time_));
 
   BlobStatus status = BlobStatus::PENDING_TRANSPORT;
   auto strategy = BlobTransportStrategy::Create(
@@ -443,32 +502,41 @@
   size_t expected_file_count =
       1 + data.size() / kTestBlobStorageMaxFileSizeBytes;
   FileInfoVector files(expected_file_count);
+  std::vector<base::FilePath> paths(expected_file_count);
   for (size_t i = 0; i < expected_file_count; ++i) {
     base::ScopedAllowBlockingForTesting allow_blocking;
-    base::FilePath path;
-    ASSERT_TRUE(base::CreateTemporaryFileInDir(data_dir_.GetPath(), &path));
+    ASSERT_TRUE(base::CreateTemporaryFileInDir(data_dir_.GetPath(), &paths[i]));
     files[i].file =
-        base::File(path, base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+        base::File(paths[i], base::File::FLAG_OPEN | base::File::FLAG_WRITE);
     files[i].file_deletion_runner =
         base::SingleThreadTaskRunner::GetCurrentDefault();
     files[i].file_reference = ShareableFileReference::GetOrCreate(
-        path, ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+        paths[i], ShareableFileReference::DELETE_ON_FINAL_RELEASE,
         bytes_provider_runner_.get());
-    size_t offset = i * kTestBlobStorageMaxFileSizeBytes;
-    size_t length = std::min<uint64_t>(kTestBlobStorageMaxFileSizeBytes,
-                                       data.size() - offset);
-    expected.AppendFile(path, 0, length, mock_time_);
   }
 
   strategy->BeginTransport(std::move(files));
   loop.Run();
 
+  for (size_t i = 0; i < expected_file_count; ++i) {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    size_t offset = i * kTestBlobStorageMaxFileSizeBytes;
+    size_t length = std::min<uint64_t>(kTestBlobStorageMaxFileSizeBytes,
+                                       data.size() - offset);
+    std::string file_contents;
+    ASSERT_TRUE(base::ReadFileToString(paths[i], &file_contents));
+    EXPECT_EQ(data.substr(offset, length), file_contents);
+    base::File::Info info;
+    ASSERT_TRUE(base::GetFileInfo(paths[i], &info));
+    expected.AppendFile(paths[i], 0, length, info.last_modified);
+  }
+
   EXPECT_EQ(BlobStatus::DONE, status);
   EXPECT_EQ(expected, builder);
   EXPECT_TRUE(bad_messages_.empty());
   EXPECT_EQ(0u, reply_request_count_);
-  EXPECT_EQ(0u, stream_request_count_);
-  EXPECT_EQ(expected_file_count, file_request_count_);
+  EXPECT_EQ(1u, stream_request_count_);
+  EXPECT_EQ(0u, file_request_count_);
 }
 
 TEST_F(BlobTransportStrategyTest, Files_ValidBytesMultipleElements) {
@@ -516,31 +584,40 @@
   size_t expected_file_count =
       1 + 4 * data.size() / kTestBlobStorageMaxFileSizeBytes;
   FileInfoVector files(expected_file_count);
+  std::vector<base::FilePath> paths(expected_file_count);
   for (size_t i = 0; i < expected_file_count; ++i) {
     base::ScopedAllowBlockingForTesting allow_blocking;
-    base::FilePath path;
-    ASSERT_TRUE(base::CreateTemporaryFileInDir(data_dir_.GetPath(), &path));
+    ASSERT_TRUE(base::CreateTemporaryFileInDir(data_dir_.GetPath(), &paths[i]));
     files[i].file =
-        base::File(path, base::File::FLAG_OPEN | base::File::FLAG_WRITE);
-    files[i].path = path;
+        base::File(paths[i], base::File::FLAG_OPEN | base::File::FLAG_WRITE);
+    files[i].path = paths[i];
     files[i].file_deletion_runner =
         base::SingleThreadTaskRunner::GetCurrentDefault();
     files[i].file_reference = ShareableFileReference::GetOrCreate(
-        path, ShareableFileReference::DELETE_ON_FINAL_RELEASE,
+        paths[i], ShareableFileReference::DELETE_ON_FINAL_RELEASE,
         bytes_provider_runner_.get());
   }
 
+  strategy->BeginTransport(std::move(files));
+  loop.Run();
+
+  std::vector<base::Time> mtimes(expected_file_count);
+  for (size_t i = 0; i < expected_file_count; ++i) {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    base::File::Info info;
+    ASSERT_TRUE(base::GetFileInfo(paths[i], &info));
+    mtimes[i] = info.last_modified;
+  }
+
   size_t file_offset = 0;
   size_t file_index = 0;
-  size_t expected_request_count = 0;
   for (size_t i = 0; i < 4; ++i) {
     size_t remaining_size = data.size();
     while (remaining_size > 0) {
       size_t block_size = std::min<uint64_t>(
           kTestBlobStorageMaxFileSizeBytes - file_offset, remaining_size);
-      expected.AppendFile(files[file_index].path, file_offset, block_size,
-                          mock_time_);
-      expected_request_count++;
+      expected.AppendFile(paths[file_index], file_offset, block_size,
+                          mtimes[file_index]);
       remaining_size -= block_size;
       file_offset += block_size;
       if (file_offset >= kTestBlobStorageMaxFileSizeBytes) {
@@ -550,15 +627,12 @@
     }
   }
 
-  strategy->BeginTransport(std::move(files));
-  loop.Run();
-
   EXPECT_EQ(BlobStatus::DONE, status);
   EXPECT_EQ(expected, builder);
   EXPECT_TRUE(bad_messages_.empty());
   EXPECT_EQ(0u, reply_request_count_);
-  EXPECT_EQ(0u, stream_request_count_);
-  EXPECT_EQ(expected_request_count, file_request_count_);
+  EXPECT_EQ(4u, stream_request_count_);
+  EXPECT_EQ(0u, file_request_count_);
 }
 
 }  // namespace
diff --git a/storage/browser/test/mock_bytes_provider.h b/storage/browser/test/mock_bytes_provider.h
index 41c7e216..ae27962 100644
--- a/storage/browser/test/mock_bytes_provider.h
+++ b/storage/browser/test/mock_bytes_provider.h
@@ -17,12 +17,12 @@
 // bytes are consumed.
 class MockBytesProvider : public blink::mojom::BytesProvider {
  public:
-  explicit MockBytesProvider(std::vector<uint8_t> data,
-                             size_t* reply_request_count = nullptr,
-                             size_t* stream_request_count = nullptr,
-                             size_t* file_request_count = nullptr,
-                             std::optional<base::Time> file_modification_time =
-                                 base::Time::FromSecondsSinceUnixEpoch(1));
+  explicit MockBytesProvider(
+      std::vector<uint8_t> data,
+      size_t* reply_request_count = nullptr,
+      size_t* stream_request_count = nullptr,
+      size_t* file_request_count = nullptr,
+      std::optional<base::Time> file_modification_time = base::Time());
   ~MockBytesProvider() override;
 
   // BytesProvider implementation: