ImportantFileWriter: Flush the data before closing the

chrome/common/important_file_writer.cc

Index: chrome/common/important_file_writer.cc
===================================================================
--- chrome/common/important_file_writer.cc	(revision 97170)
+++ chrome/common/important_file_writer.cc	(working copy)
@@ -36,22 +36,33 @@
     // 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;
     }
@@ -102,6 +113,10 @@
 
 void ImportantFileWriter::WriteNow(const std::string& data) {
   DCHECK(CalledOnValidThread());
+  if (data.length() > static_cast<size_t>(kint32max)) {
+    NOTREACHED();
+    return;
+  }
 
   if (HasPendingWrite())
     timer_.Stop();