[FSA] Allow seeking past the end of a file

Currently, we reject all attempts to write past the end of a file. This
change makes the API more consistent with the behavior of POSIX.

If writing at an offset past the end of a file, the null bytes between
the old end of the file and the offset will count towards the file's
quota.

Adds Quota tests to the SandboxFSW.

Bug: 1153385
Change-Id: Iec54acbccecb728f9375931825525ba2fbafd1ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593810
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/master@{#850838}

content/browser/file_system_access/file_system_access_file_writer_impl_unittest.cc

@@ -504,15 +504,15 @@ TEST_P(FileSystemAccessFileWriterImplWriteTest, WriteWithOffsetInFile) {
 TEST_P(FileSystemAccessFileWriterImplWriteTest, WriteWithOffsetPastFile) {
   uint64_t bytes_written;
   FileSystemAccessStatus result = WriteSync(4, "abc", &bytes_written);
-  EXPECT_EQ(result, FileSystemAccessStatus::kFileError);
-  EXPECT_EQ(bytes_written, 0u);
+  EXPECT_EQ(result, FileSystemAccessStatus::kOk);
+  EXPECT_EQ(bytes_written, 3u);
 
   result = CloseSync();
   EXPECT_EQ(result, FileSystemAccessStatus::kOk);
   EXPECT_TRUE(base::Contains(quarantine_.paths, test_file_url_.path()));
 
   using std::string_literals::operator""s;
-  EXPECT_EQ(""s, ReadFile(test_file_url_));
+  EXPECT_EQ("\0\0\0\0abc"s, ReadFile(test_file_url_));
 }
 
 TEST_F(FileSystemAccessFileWriterImplTest, TruncateShrink) {

storage/browser/file_system/file_stream_writer_test.h

@@ -41,8 +41,8 @@ class FileStreamWriterTest : public testing::Test {
   static void NeverCalled(int unused) { ADD_FAILURE(); }
 
  private:
-  base::test::SingleThreadTaskEnvironment task_environment_{
-      base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
+  base::test::TaskEnvironment task_environment_{
+      base::test::TaskEnvironment::MainThreadType::IO};
 };
 
 template <class SubClass>
@@ -109,11 +109,11 @@ TYPED_TEST_P(FileStreamWriterTypedTest, WriteAfterEnd) {
 
   std::unique_ptr<FileStreamWriter> writer(
       this->CreateWriter(std::string(this->kTestFileName), 7));
-  EXPECT_EQ(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE,
-            WriteStringToWriter(writer.get(), "xxx"));
+  EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "xxx"));
 
   EXPECT_TRUE(this->FilePathExists(std::string(this->kTestFileName)));
-  EXPECT_EQ("foobar", this->GetFileContent(std::string(this->kTestFileName)));
+  EXPECT_EQ(std::string("foobar\0xxx", 10),
+            this->GetFileContent(std::string(this->kTestFileName)));
 }
 
 TYPED_TEST_P(FileStreamWriterTypedTest, WriteFailForNonexistingFile) {

storage/browser/file_system/local_file_stream_writer.cc

@@ -141,46 +141,19 @@ void LocalFileStreamWriter::DidOpen(base::OnceClosure main_operation,
     return;
   }
 
-  auto file_info = std::make_unique<base::File::Info>();
-  base::File::Info* raw_file_info = file_info.get();
-  result = stream_impl_->GetFileInfo(
-      raw_file_info,
-      base::BindOnce(&LocalFileStreamWriter::InitiateSeek,
-                     weak_factory_.GetWeakPtr(), std::move(main_operation),
-                     std::move(file_info)));
-  DCHECK_NE(result, net::OK);
-  if (result != net::ERR_IO_PENDING) {
-    has_pending_operation_ = false;
-    std::move(write_callback_).Run(result);
-  }
+  InitiateSeek(std::move(main_operation));
 }
 
-void LocalFileStreamWriter::InitiateSeek(
-    base::OnceClosure main_operation,
-    std::unique_ptr<base::File::Info> file_info,
-    int file_info_result) {
+void LocalFileStreamWriter::InitiateSeek(base::OnceClosure main_operation) {
   DCHECK(has_pending_operation_);
   DCHECK(stream_impl_.get());
 
-  if (file_info_result != net::OK) {
-    has_pending_operation_ = false;
-    std::move(write_callback_).Run(file_info_result);
-    return;
-  }
-
   if (initial_offset_ == 0) {
     // No need to seek.
     std::move(main_operation).Run();
     return;
   }
 
-  if (initial_offset_ > file_info->size) {
-    // We should not be writing past the end of the file.
-    has_pending_operation_ = false;
-    std::move(write_callback_).Run(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
-    return;
-  }
-
   int result = stream_impl_->Seek(
       initial_offset_,
       base::BindOnce(&LocalFileStreamWriter::DidSeek,

storage/browser/file_system/local_file_stream_writer.h

@@ -13,7 +13,6 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/component_export.h"
-#include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -55,9 +54,7 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) LocalFileStreamWriter
 
   // Seeks to |initial_offset_| and proceeds to |main_operation| if it succeeds.
   // If failed, the error code is returned by calling |error_callback|.
-  void InitiateSeek(base::OnceClosure main_operation,
-                    std::unique_ptr<base::File::Info> file_info,
-                    int file_info_result);
+  void InitiateSeek(base::OnceClosure main_operation);
   void DidSeek(base::OnceClosure main_operation, int64_t result);
 
   // Passed as the |main_operation| of InitiateOpen() function.

storage/browser/file_system/obfuscated_file_util_memory_delegate.cc

@@ -485,7 +485,7 @@ int ObfuscatedFileUtilMemoryDelegate::WriteFile(
 
   size_t offset_u = static_cast<size_t>(offset);
   // Fail if |offset| or |buf_len| not valid.
-  if (offset < 0 || buf_len < 0 || offset_u > dp->entry->file_content.size())
+  if (offset < 0 || buf_len < 0)
     return net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
 
   // Fail if result doesn't fit in a std::vector.
@@ -500,6 +500,8 @@ int ObfuscatedFileUtilMemoryDelegate::WriteFile(
     if (offset_u + buf_len > dp->entry->file_content.size())
       dp->entry->file_content.resize(offset_u + buf_len);
 
+    // if |offset_u| is larger than the original file size, there will be null
+    // bytes between the end of the file and |offset_u|.
     memcpy(dp->entry->file_content.data() + offset, buf->data(), buf_len);
   }
   return buf_len;

storage/browser/file_system/sandbox_file_stream_writer.cc

@@ -29,18 +29,18 @@ namespace storage {
 
 namespace {
 
-// Adjust the |quota| value in overwriting case (i.e. |file_size| > 0 and
-// |file_offset| < |file_size|) to make the remaining quota calculation easier.
-// Specifically this widens the quota for overlapping range (so that we can
-// simply compare written bytes against the adjusted quota).
+// Adjust the |quota| value to make the remaining quota calculation easier. This
+// allows us to simply compare written bytes against the adjusted quota.
 int64_t AdjustQuotaForOverlap(int64_t quota,
                               int64_t file_offset,
                               int64_t file_size) {
-  DCHECK_LE(file_offset, file_size);
   if (quota < 0)
     quota = 0;
+  // |overlap| can be negative if |file_offset| is past the end of the file.
+  // Negative |overlap| ensures null bytes between the end of the file and the
+  // |file_offset| are counted towards the file's quota.
   int64_t overlap = file_size - file_offset;
-  if (std::numeric_limits<int64_t>::max() - overlap > quota)
+  if (overlap < 0 || std::numeric_limits<int64_t>::max() - overlap > quota)
     quota += overlap;
   return quota;
 }
@@ -141,11 +141,7 @@ void SandboxFileStreamWriter::DidCreateSnapshotFile(
     return;
   }
   file_size_ = file_info.size;
-  if (initial_offset_ > file_size_) {
-    // We should not be writing past the end of the file.
-    std::move(callback).Run(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
-    return;
-  }
+
   DCHECK(!file_writer_.get());
 
   if (file_system_context_->is_incognito()) {
@@ -243,7 +239,10 @@ void SandboxFileStreamWriter::DidWrite(int write_response) {
 
   if (total_bytes_written_ + write_response + initial_offset_ > file_size_) {
     int overlapped = file_size_ - total_bytes_written_ - initial_offset_;
-    if (overlapped < 0)
+    // If writing past the end of a file, the distance seeked past the file
+    // needs to be accounted for. This adjustment should only be made for the
+    // first such write (when |total_bytes_written_| is 0).
+    if (overlapped < 0 && total_bytes_written_ != 0)
       overlapped = 0;
     observers_.Notify(&FileUpdateObserver::OnUpdate, url_,
                       write_response - overlapped);

storage/browser/file_system/sandbox_file_stream_writer_unittest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "storage/browser/file_system/sandbox_file_stream_writer.h"
+#include "base/test/bind.h"
 #include "storage/browser/file_system/file_stream_writer_test.h"
 
 #include <stdint.h>
@@ -22,6 +23,8 @@
 #include "storage/browser/file_system/file_stream_writer.h"
 #include "storage/browser/file_system/file_system_context.h"
 #include "storage/browser/test/async_file_test_helper.h"
+#include "storage/browser/test/mock_quota_manager_proxy.h"
+#include "storage/browser/test/mock_special_storage_policy.h"
 #include "storage/browser/test/test_file_system_context.h"
 #include "storage/common/file_system/file_system_types.h"
 
@@ -38,7 +41,14 @@ class SandboxFileStreamWriterTest : public FileStreamWriterTest {
   void SetUp() override {
     ASSERT_TRUE(dir_.CreateUniqueTempDir());
 
-    file_system_context_ = CreateFileSystemContext(dir_);
+    quota_manager_ = base::MakeRefCounted<storage::MockQuotaManager>(
+        is_incognito(), dir_.GetPath(), base::ThreadTaskRunnerHandle::Get(),
+        nullptr);
+    quota_manager_proxy_ = base::MakeRefCounted<storage::MockQuotaManagerProxy>(
+        quota_manager_.get(), base::ThreadTaskRunnerHandle::Get().get());
+
+    file_system_context_ =
+        CreateFileSystemContext(quota_manager_proxy_.get(), dir_);
 
     file_system_context_->OpenFileSystem(
         url::Origin::Create(GURL(kURLOrigin)), kFileSystemTypeTemporary,
@@ -47,20 +57,38 @@ class SandboxFileStreamWriterTest : public FileStreamWriterTest {
                           base::File::Error result) {
           ASSERT_EQ(base::File::FILE_OK, result);
         }));
+
+    SetQuota(1024 * 1024 * 100);
     base::RunLoop().RunUntilIdle();
   }
 
-  void TearDown() override { base::RunLoop().RunUntilIdle(); }
+  void TearDown() override {
+    quota_manager_proxy_->SimulateQuotaManagerDestroyed();
+    quota_manager_.reset();
+    base::RunLoop().RunUntilIdle();
+  }
 
  protected:
   base::ScopedTempDir dir_;
   scoped_refptr<FileSystemContext> file_system_context_;
+  scoped_refptr<MockQuotaManager> quota_manager_;
+  scoped_refptr<MockQuotaManagerProxy> quota_manager_proxy_;
+
+  struct quota_usage_and_info {
+    blink::mojom::QuotaStatusCode status;
+    int64_t usage;
+    int64_t quota;
+  };
 
   virtual FileSystemContext* CreateFileSystemContext(
+      QuotaManagerProxy* quota_manager_proxy,
       const base::ScopedTempDir& dir) {
-    return CreateFileSystemContextForTesting(nullptr, dir.GetPath());
+    return CreateFileSystemContextForTesting(quota_manager_proxy,
+                                             dir.GetPath());
   }
 
+  virtual bool is_incognito() { return false; }
+
   FileSystemURL GetFileSystemURL(const std::string& file_name) {
     return file_system_context_->CreateCrackedFileSystemURL(
         url::Origin::Create(GURL(kURLOrigin)), kFileSystemTypeTemporary,
@@ -107,8 +135,181 @@ class SandboxFileStreamWriterTest : public FileStreamWriterTest {
 
     return content;
   }
+
+  std::unique_ptr<SandboxFileStreamWriter> CreateSandboxWriter(
+      const std::string& name,
+      int64_t offset) {
+    auto writer = std::make_unique<SandboxFileStreamWriter>(
+        file_system_context_.get(), GetFileSystemURL(name), offset,
+        *file_system_context_->GetUpdateObservers(kFileSystemTypeTemporary));
+    return writer;
+  }
+
+  quota_usage_and_info GetUsageAndQuotaSync() {
+    quota_usage_and_info info;
+    quota_manager_->GetUsageAndQuota(
+        url::Origin::Create(GURL(kURLOrigin)),
+        blink::mojom::StorageType::kTemporary,
+        base::BindLambdaForTesting([&](blink::mojom::QuotaStatusCode status,
+                                       int64_t usage, int64_t quota) {
+          info.status = status;
+          info.usage = usage;
+          info.quota = quota;
+        }));
+    return info;
+  }
+
+  void SetQuota(int64_t quota) {
+    quota_manager_->SetQuota(url::Origin::Create(GURL(kURLOrigin)),
+                             blink::mojom::StorageType::kTemporary, quota);
+  }
+
+  int64_t GetFreeQuota() {
+    auto info = GetUsageAndQuotaSync();
+    return info.quota - info.usage;
+  }
+
+  void SetFreeQuota(int64_t free_quota) {
+    auto info = GetUsageAndQuotaSync();
+    SetQuota(info.usage + free_quota);
+  }
+
+  void Test_Quota_OK() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(7);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 3));
+    EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "xxx"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("fooxxx", 6), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 4);
+  }
+
+  void Test_Quota_WritePastEnd() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(6);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 6));
+    EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "xxx"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo\0\0\0xxx", 9), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 0);
+  }
+
+  void Test_Quota_NoSpace() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(0);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 3));
+    EXPECT_EQ(net::ERR_FILE_NO_SPACE, WriteStringToWriter(writer.get(), "xxx"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo", 3), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 0);
+  }
+
+  void Test_Quota_NoSpace_PartialWrite() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(5);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 6));
+    EXPECT_EQ(net::ERR_FILE_NO_SPACE, WriteStringToWriter(writer.get(), "xxx"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo\0\0\0xx", 8), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 0);
+  }
+
+  void Test_Quota_Negative() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(-1);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 3));
+    EXPECT_EQ(net::ERR_FILE_NO_SPACE, WriteStringToWriter(writer.get(), "xxx"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo", 3), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), -1);
+  }
+
+  void Test_Quota_WritePastEndTwice_OK() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(9);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 6));
+    EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "xxx"));
+    EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "yyy"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo\0\0\0xxxyyy", 12), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 0);
+  }
+
+  void Test_Quota_WritePastEndTwice_NoSpace() {
+    std::string name = "file_a";
+    EXPECT_TRUE(CreateFileWithContent(name, "foo"));
+
+    SetFreeQuota(7);
+    std::unique_ptr<SandboxFileStreamWriter> writer(
+        CreateSandboxWriter(name, 6));
+    EXPECT_EQ(net::OK, WriteStringToWriter(writer.get(), "xxx"));
+    EXPECT_EQ(net::ERR_FILE_NO_SPACE, WriteStringToWriter(writer.get(), "yyy"));
+    writer.reset();
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(FilePathExists(name));
+    EXPECT_EQ(std::string("foo\0\0\0xxxy", 10), GetFileContent(name));
+    EXPECT_EQ(GetFreeQuota(), 0);
+  }
 };
 
+TEST_F(SandboxFileStreamWriterTest, Quota_OK) {
+  Test_Quota_OK();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_WritePastEnd) {
+  Test_Quota_WritePastEnd();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_NoSpace) {
+  Test_Quota_NoSpace();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_NoSpace_PartialWrite) {
+  Test_Quota_NoSpace_PartialWrite();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_Negative) {
+  Test_Quota_Negative();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_WritePastEndTwice_OK) {
+  Test_Quota_WritePastEndTwice_OK();
+}
+
+TEST_F(SandboxFileStreamWriterTest, Quota_WritePastEndTwice_NoSpace) {
+  Test_Quota_WritePastEndTwice_NoSpace();
+}
+
 INSTANTIATE_TYPED_TEST_SUITE_P(Sandbox,
                                FileStreamWriterTypedTest,
                                SandboxFileStreamWriterTest);
@@ -120,13 +321,45 @@ class SandboxFileStreamWriterIncognitoTest
 
  protected:
   FileSystemContext* CreateFileSystemContext(
+      QuotaManagerProxy* quota_manager_proxy,
       const base::ScopedTempDir& dir) override {
     return CreateIncognitoFileSystemContextForTesting(
         base::ThreadTaskRunnerHandle::Get(),
-        base::ThreadTaskRunnerHandle::Get(), nullptr, dir.GetPath());
+        base::ThreadTaskRunnerHandle::Get(), quota_manager_proxy,
+        dir.GetPath());
   }
+
+  bool is_incognito() override { return true; }
 };
 
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_OK) {
+  Test_Quota_OK();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_WritePastEnd) {
+  Test_Quota_WritePastEnd();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_NoSpace) {
+  Test_Quota_NoSpace();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_NoSpace_PartialWrite) {
+  Test_Quota_NoSpace_PartialWrite();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_Negative) {
+  Test_Quota_Negative();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_WritePastEndTwice_OK) {
+  Test_Quota_WritePastEndTwice_OK();
+}
+
+TEST_F(SandboxFileStreamWriterIncognitoTest, Quota_WritePastEndTwice_NoSpace) {
+  Test_Quota_WritePastEndTwice_NoSpace();
+}
+
 INSTANTIATE_TYPED_TEST_SUITE_P(SandboxIncognito,
                                FileStreamWriterTypedTest,
                                SandboxFileStreamWriterIncognitoTest);

third_party/blink/web_tests/external/wpt/file-system-access/script-tests/FileSystemWritableFileStream-write.js

@@ -117,14 +117,12 @@ directory_test(async (t, root) => {
   const handle = await createEmptyFile(t, 'bad_offset', root);
   const stream = await handle.createWritable();
 
-  await promise_rejects_dom(
-      t, 'InvalidStateError', stream.write({type: 'write', position: 4, data: new Blob(['abc'])}));
-  await promise_rejects_js(
-      t, TypeError, stream.close(), 'stream is already closed');
+  await stream.write({type: 'write', position: 4, data: new Blob(['abc'])});
+  await stream.close();
 
-  assert_equals(await getFileContents(handle), '');
-  assert_equals(await getFileSize(handle), 0);
-}, 'write() called with an invalid offset');
+  assert_equals(await getFileContents(handle), '\0\0\0\0abc');
+  assert_equals(await getFileSize(handle), 7);
+}, 'write() called with an offset beyond the end of the file');
 
 directory_test(async (t, root) => {
   const handle = await createEmptyFile(t, 'empty_string', root);