Sandbox the component updater's patcher utility process.

courgette/third_party/bsdiff/bsdiff_apply.cc

Index: courgette/third_party/bsdiff/bsdiff_apply.cc
diff --git a/courgette/third_party/bsdiff/bsdiff_apply.cc b/courgette/third_party/bsdiff/bsdiff_apply.cc
index fca9616a8ab0bf62a29ef6f1a8dc62ab59a0988b..f5c105a9088d5eeae726c25181d96f3b07e141f5 100644
--- a/courgette/third_party/bsdiff/bsdiff_apply.cc
+++ b/courgette/third_party/bsdiff/bsdiff_apply.cc
@@ -40,6 +40,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include "base/files/file.h"
 #include "base/files/memory_mapped_file.h"
 #include "courgette/crc.h"
 #include "courgette/streams.h"
@@ -192,21 +193,38 @@ BSDiffStatus ApplyBinaryPatch(SourceStream* old_stream,
 BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path,
                               const base::FilePath& patch_file_path,
                               const base::FilePath& new_file_path) {
+  return ApplyBinaryPatch(
+      base::File(
+          base::FilePath(old_file_path),
+          base::File::FLAG_OPEN | base::File::FLAG_READ),
+      base::File(
+          base::FilePath(patch_file_path),
+          base::File::FLAG_OPEN | base::File::FLAG_READ),
+      base::File(
+          base::FilePath(new_file_path),
+          base::File::FLAG_CREATE_ALWAYS |
+              base::File::FLAG_WRITE |
+              base::File::FLAG_EXCLUSIVE_WRITE));
+}
+
+BSDiffStatus ApplyBinaryPatch(base::File old_file,
+                              base::File patch_file,
+                              base::File new_file) {
   // Set up the old stream.
-  base::MemoryMappedFile old_file;
-  if (!old_file.Initialize(old_file_path)) {
+  base::MemoryMappedFile old_file_mem;
+  if (!old_file_mem.Initialize(std::move(old_file))) {
     return READ_ERROR;
   }
   SourceStream old_file_stream;
-  old_file_stream.Init(old_file.data(), old_file.length());
+  old_file_stream.Init(old_file_mem.data(), old_file_mem.length());
 
   // Set up the patch stream.
-  base::MemoryMappedFile patch_file;
-  if (!patch_file.Initialize(patch_file_path)) {
+  base::MemoryMappedFile patch_file_mem;
+  if (!patch_file_mem.Initialize(std::move(patch_file))) {
     return READ_ERROR;
   }
   SourceStream patch_file_stream;
-  patch_file_stream.Init(patch_file.data(), patch_file.length());
+  patch_file_stream.Init(patch_file_mem.data(), patch_file_mem.length());
 
   // Set up the new stream and apply the patch.
   SinkStream new_sink_stream;
@@ -217,8 +235,9 @@ BSDiffStatus ApplyBinaryPatch(const base::FilePath& old_file_path,
   }
 
   // Write the stream to disk.

[huangs, 2016/12/06 18:23:22]

There seems to be a change in behavior: Previously if failure occurs before this
point then |new_file_path| does not get written. But with the change,
|new_file_path| always gets written, if failure happens then we'll have a size-0
file lying around.

[waffles, 2016/12/06 19:18:45]

Yes, that is correct. This can't be avoided in the case where we create the file
in the browser process and pass the handle to the utility process. It could be
avoided for the in-process patching case (i.e. on ios or from the courgette
executable), at the cost of some code duplication.

Alternatively, the path-based ApplyBinaryPatch could be removed and its only
call site updated. There are a few more call sites for ApplyEnsemblePatch.

Let me know if this is a concern, and if so how you want to proceed.

[huangs, 2016/12/06 19:40:07]

This is not an issue for Component Updator, but it's a change for stand-alone
Courgette and usage by Installer.

- For stand-alone Courgette: This departs from what I'm used to for
Courgette-apply -- but I can live with that. There's a slight risk of having
validation script that checks "existence of output file after running
Courgette". I don't believe anyone's doing that.

- For installer: On patch failure there will be a size-0 file lying around. I
believe entire directories get wiped out on failure anyway, so we probably won't
accumulate permanent artifacts on user's in this case.

The "beliefs" above are not verified by fact. To not waste time, I propose:
- Mention the behavioral change in CL comment, commit as usual.
- Ping Chrome build folks to see if, after they generate patch, whether do they
verify by applying the patch. If things fail, there will be size-0 file lying
around and hopefully that's not an issue.
- Ping grt@ re. installer, and note that now size-0 file will be created if
patching fails, and ask whether things will be cleaned up.

[waffles, 2016/12/06 19:46:22]

Sounds good, will do.

-  int written = base::WriteFile(
-      new_file_path, reinterpret_cast<const char*>(new_sink_stream.Buffer()),
+  int written = new_file.Write(
+      0,
+      reinterpret_cast<const char*>(new_sink_stream.Buffer()),
       static_cast<int>(new_sink_stream.Length()));
   if (written != static_cast<int>(new_sink_stream.Length()))
     return WRITE_ERROR;