Truncate CdmFileIO temporary file before writing to it

As CdmFileIO writing is now asynchronous, it is possible for a write()
call that is cancelled by calling close() before the write callback is
called to actually succeed (or partially succeed). Normally that is not
a problem as the implementation of write creates a temporary file,
writes to it, and then renames the file to the actual file name as a
final step. However, if the write() is part way through the process
when it is cancelled, it may leave a temporary file around. Next time
write() is called, it will overwrite the existing temporary file. If
the new contents are shorter than what is in the file, then extra data
will be returned when the file is read later.

The fix is to truncate the temporary file if it exists. So in the rare
case that a temporary file is left around, it will be reset to 0 bytes
before writing to it.

This CL also updates all the FileIO tests that do this (call close()
immediately after calling write() without waiting for write() to
succeed). They will now check for one of the two buffers to be returned
on the next read(), as the second write() may have actually succeeded
before close() was called.

This should fix the code, so re-enabling the browser_tests that were
flaky.

Bug: 999421,997953
Test: Re-enabled tests succeed
Change-Id: Ib1772b678c7e89f588e81cc40055650478e6a9e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1793722Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: John Rummell <jrummell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699144}

chrome/browser/media/encrypted_media_browsertest.cc

@@ -801,8 +801,7 @@ IN_PROC_BROWSER_TEST_P(ECKEncryptedMediaTest, DISABLED_CDMCrashDuringDecode) {
                        kEmeSessionClosedAndError);
 }
 
-// Flaky: crbug.com/997953
-IN_PROC_BROWSER_TEST_P(ECKEncryptedMediaTest, DISABLED_FileIOTest) {
+IN_PROC_BROWSER_TEST_P(ECKEncryptedMediaTest, FileIOTest) {
   TestNonPlaybackCases(kExternalClearKeyFileIOTestKeySystem, kUnitTestSuccess);
 }
 
@@ -964,8 +963,7 @@ IN_PROC_BROWSER_TEST_P(ECKEncryptedMediaTest, CdmProxy) {
 // save anything to disk. Instead we are only running the tests that actually
 // have the CDM do file access.
 
-// TODO(crbug.com/999421) Disabled due to flake on all platforms
-IN_PROC_BROWSER_TEST_F(ECKIncognitoEncryptedMediaTest, DISABLED_FileIO) {
+IN_PROC_BROWSER_TEST_F(ECKIncognitoEncryptedMediaTest, FileIO) {
   // Try the FileIO test using the default CDM API while running in incognito.
   TestNonPlaybackCases(kExternalClearKeyFileIOTestKeySystem, kUnitTestSuccess);
 }

content/browser/media/cdm_file_impl.cc

@@ -522,6 +522,8 @@ void CdmFileImpl::Write(const std::vector<uint8_t>& data,
 
   // FileStreamWriter only works on existing files. |temp_file_name_| should not
   // exist, so create an empty one if necessary.
+  // We can not use AsyncFileUtil::CreateOrOpen() as it does not work with the
+  // incognito filesystem (http://crbug.com/958294).
   auto url = CreateFileSystemURL(temp_file_name_);
   auto* file_util = file_system_context_->GetAsyncFileUtil(
       storage::kFileSystemTypePluginPrivate);
@@ -529,17 +531,17 @@ void CdmFileImpl::Write(const std::vector<uint8_t>& data,
       std::make_unique<storage::FileSystemOperationContext>(
           file_system_context_.get());
   operation_context->set_allowed_bytes_growth(storage::QuotaManager::kNoLimit);
-  file_util->EnsureFileExists(
-      std::move(operation_context), url,
-      base::Bind(&CdmFileImpl::OnEnsureFileExists, weak_factory_.GetWeakPtr(),
-                 std::move(buffer), bytes_to_write));
+  file_util->EnsureFileExists(std::move(operation_context), url,
+                              base::Bind(&CdmFileImpl::OnEnsureTempFileExists,
+                                         weak_factory_.GetWeakPtr(),
+                                         std::move(buffer), bytes_to_write));
 }
 
-void CdmFileImpl::OnEnsureFileExists(scoped_refptr<net::IOBuffer> buffer,
-                                     int bytes_to_write,
-                                     base::File::Error result,
-                                     bool created) {
-  DVLOG(3) << __func__ << " file: " << file_name_
+void CdmFileImpl::OnEnsureTempFileExists(scoped_refptr<net::IOBuffer> buffer,
+                                         int bytes_to_write,
+                                         base::File::Error result,
+                                         bool created) {
+  DVLOG(3) << __func__ << " file: " << temp_file_name_
            << ", result: " << base::File::ErrorToString(result);
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(file_locked_);
@@ -554,6 +556,44 @@ void CdmFileImpl::OnEnsureFileExists(scoped_refptr<net::IOBuffer> buffer,
     return;
   }
 
+  // If the temp file has just been created, we know it is empty and can simply
+  // proceed with writing to it. However, if the file exists, truncate it in
+  // case it is longer than the number of bytes we want to write.
+  if (created) {
+    OnTempFileIsEmpty(std::move(buffer), bytes_to_write, result);
+    return;
+  }
+
+  auto url = CreateFileSystemURL(temp_file_name_);
+  auto* file_util = file_system_context_->GetAsyncFileUtil(
+      storage::kFileSystemTypePluginPrivate);
+  auto operation_context =
+      std::make_unique<storage::FileSystemOperationContext>(
+          file_system_context_.get());
+  operation_context->set_allowed_bytes_growth(storage::QuotaManager::kNoLimit);
+  file_util->Truncate(
+      std::move(operation_context), url, 0,
+      base::Bind(&CdmFileImpl::OnTempFileIsEmpty, weak_factory_.GetWeakPtr(),
+                 std::move(buffer), bytes_to_write));
+}
+
+void CdmFileImpl::OnTempFileIsEmpty(scoped_refptr<net::IOBuffer> buffer,
+                                    int bytes_to_write,
+                                    base::File::Error result) {
+  DVLOG(3) << __func__ << " file: " << temp_file_name_
+           << ", result: " << base::File::ErrorToString(result);
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(file_locked_);
+  DCHECK(write_callback_);
+  DCHECK(!file_writer_);
+
+  if (result != base::File::FILE_OK) {
+    DLOG(WARNING) << "Failed to truncate temporary file, result: "
+                  << base::File::ErrorToString(result);
+    std::move(write_callback_).Run(Status::kFailure);
+    return;
+  }
+
   // As writing is done on the IO thread, when it's done WriteDone() needs to be
   // called on this thread.
   auto write_done_cb = media::BindToCurrentLoop(

content/browser/media/cdm_file_impl.h

@@ -63,10 +63,13 @@ class CdmFileImpl final : public media::mojom::CdmFile {
   // written to the file, |bytes_to_write| is the length. Uses |file_writer_|,
   // which is cleared when no longer needed. |write_callback_| will always be
   // called with the result.
-  void OnEnsureFileExists(scoped_refptr<net::IOBuffer> buffer,
-                          int bytes_to_write,
-                          base::File::Error result,
-                          bool created);
+  void OnEnsureTempFileExists(scoped_refptr<net::IOBuffer> buffer,
+                              int bytes_to_write,
+                              base::File::Error result,
+                              bool created);
+  void OnTempFileIsEmpty(scoped_refptr<net::IOBuffer> buffer,
+                         int bytes_to_write,
+                         base::File::Error result);
   void WriteDone(bool success);
   void OnFileRenamed(base::File::Error move_result);
 

media/cdm/library_cdm/clear_key_cdm/cdm_file_io_test.cc

@@ -10,6 +10,7 @@
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/string_number_conversions.h"
 
 namespace media {
 
@@ -434,12 +435,17 @@ void FileIOTestRunner::AddTests() {
     EXPECT_FILE_WRITTEN(kSuccess)
     WRITE_FILE(kBigData, kBigDataSize)
     CLOSE_FILE
-    // Write() didn't finish and the content of the file is not modified.
+    // Write() is async, so it may or may not modify the content of the file.
     CREATE_FILE_IO
     OPEN_FILE
     EXPECT_FILE_OPENED(kSuccess)
     READ_FILE
-    EXPECT_FILE_READ(kSuccess, kData, kDataSize)
+    // As Write() is async, it is possible that the second write above
+    // succeeds before the file is closed. So check that the contents
+    // is either data set.
+    EXPECT_FILE_READ_EITHER(kSuccess,
+                            kData, kDataSize,
+                            kBigData, kBigDataSize)
   END_TEST_CASE
 
   START_TEST_CASE("CloseDuringPendingOverwriteWithSmallerData")
@@ -449,12 +455,17 @@ void FileIOTestRunner::AddTests() {
     EXPECT_FILE_WRITTEN(kSuccess)
     WRITE_FILE(kData, kDataSize)
     CLOSE_FILE
-    // Write() didn't finish and the content of the file is not modified.
+    // Write() is async, so it may or may not modify the content of the file.
     CREATE_FILE_IO
     OPEN_FILE
     EXPECT_FILE_OPENED(kSuccess)
     READ_FILE
-    EXPECT_FILE_READ(kSuccess, kBigData, kBigDataSize)
+    // As Write() is async, it is possible that the second write above
+    // succeeds before the file is closed. So check that the contents
+    // is either data set.
+    EXPECT_FILE_READ_EITHER(kSuccess,
+                            kBigData, kBigDataSize,
+                            kData, kDataSize)
   END_TEST_CASE
 
   START_TEST_CASE("CloseDuringPendingRead")
@@ -656,7 +667,10 @@ void FileIOTest::OnResult(const TestStep& result) {
   if (!CheckResult(result)) {
     LOG(WARNING) << test_name_ << " got unexpected result. type=" << result.type
                  << ", status=" << (uint32_t)result.status
-                 << ", data_size=" << result.data_size;
+                 << ", data_size=" << result.data_size << ", received data="
+                 << (result.data
+                         ? base::HexEncode(result.data, result.data_size)
+                         : "<null>");
     for (const auto& step : test_steps_) {
       if (IsResult(step)) {
         LOG(WARNING) << test_name_ << " expected type=" << step.type