ImportantFileWriter: Flush the data before closing the

chrome/common/important_file_writer.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/important_file_writer.h"
 
 #include <stdio.h>
 
 #include <string>
 
@@ skipping 18 matching lines @@
       : path_(path),
         data_(data) {
   }
 
   virtual void Run() {
     // Write the data to a temp file then rename to avoid data loss if we crash
     // while writing the file. Ensure that the temp file is on the same volume
     // as target file, so it can be moved in one step, and that the temp file
     // is securely created.
     FilePath tmp_file_path;
-    FILE* tmp_file = file_util::CreateAndOpenTemporaryFileInDir(
-        path_.DirName(), &tmp_file_path);
-    if (!tmp_file) {
+    if (!file_util::CreateTemporaryFileInDir(path_.DirName(), &tmp_file_path)) {
       LogFailure("could not create temporary file");
       return;
     }
 
-    size_t bytes_written = fwrite(data_.data(), 1, data_.length(), tmp_file);
-    if (!file_util::CloseFile(tmp_file)) {
+    int flags = base::PLATFORM_FILE_OPEN | base::PLATFORM_FILE_WRITE;
+    base::PlatformFile tmp_file =
+        base::CreatePlatformFile(tmp_file_path, flags, NULL, NULL);

[Scott Hess - ex-Googler, 2011/08/23 00:54:38]

I would prefer if you could not introduce a race condition on POSIX platforms
(if someone replaced the file with a symlink between the create and the open). 
That's probably why CreateAndOpenTemporaryFile*() was originally created.

[rvargas (doing something else), 2011/08/23 01:41:52]

I was writing the code with some posix specific version to keep the same
behavior but decided against that because a) the code that returns a a file
descriptor directly is not currently exported by file_util and b) if we really
care about someone doing something with the file when we don't hold a reference
to it, the code is still broken by the write/close -- rename sequence.

In other words, I don't think that is a use case that we should worry about.

[Scott Hess - ex-Googler, 2011/08/23 19:23:57]

The rename case is a very good point!  I won't dig deeper on this review :-).

[Anyhow, AFAICT, if someone can inject into path_, the user is already owned. 
So calling it "target name _suffix" rather than using a temp file would be
sufficient, and wouldn't leave garbage on crashes.]

+    if (tmp_file == base::kInvalidPlatformFileValue) {
+      LogFailure("could not open temporary file");
+      return;
+    }
+
+    CHECK_LE(data_.length(), static_cast<size_t>(kint32max));
+    int bytes_written = base::WritePlatformFile(
+        tmp_file, 0, data_.data(), static_cast<int>(data_.length()));

[Scott Hess - ex-Googler, 2011/08/23 19:27:17]

BTW ... AFAICT the POSIX version of this function doesn't handle short writes in
case of signal.

[rvargas (doing something else), 2011/08/23 23:36:58]

In that case, line 63 should detect the error and fail, right?

+    base::FlushPlatformFile(tmp_file);  // Ignore return value.

[Scott Hess - ex-Googler, 2011/08/23 00:54:38]

Would it be reasonable to wire up something like file_util::FlushFile()?  On
POSIX it would be something like fflush() followed by
FlushPlatformFile(fileno(file)), but I don't know if it would be so easy on
Windows.  Or possible file_util::FlushAndCloseFile() would make sense.

[rvargas (doing something else), 2011/08/23 01:41:52]

Not sure. I can make fflush do the right thing on Windows, but I don't think we
need two versions of the code around: one dealing with the CRT and another one
for native file implementations. The way I see this is that given that we have a
cross-platform native file abstraction, we should only use the CRT version for
really trivial stuff, so building FILE* helpers on top of fd seems wrong.

[Scott Hess - ex-Googler, 2011/08/23 19:23:57]

OK, that's all I wanted to hear - I hate having multiple half-assed file
abstractions, because I have no idea which one I'm supposed to champion in
code-reviews like this.  We should just build a full-assed abstraction of our
own and use that everywhere.  I'll get right on it.

+
+    if (!base::ClosePlatformFile(tmp_file)) {
       LogFailure("failed to close temporary file");
       file_util::Delete(tmp_file_path, false);
       return;
     }
-    if (bytes_written < data_.length()) {
+
+    if (bytes_written < static_cast<int>(data_.length())) {
       LogFailure("error writing, bytes_written=" +
-                 base::Uint64ToString(bytes_written));
+                 base::IntToString(bytes_written));
       file_util::Delete(tmp_file_path, false);
       return;
     }
 
     if (!file_util::ReplaceFile(tmp_file_path, path_)) {
       LogFailure("could not rename temporary file");
       file_util::Delete(tmp_file_path, false);
       return;
     }
   }
@@ skipping 30 matching lines @@
   DCHECK(!HasPendingWrite());
 }
 
 bool ImportantFileWriter::HasPendingWrite() const {
   DCHECK(CalledOnValidThread());
   return timer_.IsRunning();
 }
 
 void ImportantFileWriter::WriteNow(const std::string& data) {
   DCHECK(CalledOnValidThread());
+  if (data.length() > static_cast<size_t>(kint32max)) {
+    NOTREACHED();
+    return;
+  }
 
   if (HasPendingWrite())
     timer_.Stop();
 
   if (!file_message_loop_proxy_->PostTask(
       FROM_HERE, new WriteToDiskTask(path_, data))) {
     // Posting the task to background message loop is not expected
     // to fail, but if it does, avoid losing data and just hit the disk
     // on the current thread.
     NOTREACHED();
@@ skipping 25 matching lines @@
   DCHECK(serializer_);
   std::string data;
   if (serializer_->SerializeData(&data)) {
     WriteNow(data);
   } else {
     LOG(WARNING) << "failed to serialize data to be saved in "
                  << path_.value();
   }
   serializer_ = NULL;
 }