Sandbox the component updater's patcher utility process.

courgette/third_party/bsdiff/bsdiff_apply.cc

 // Copyright 2003, 2004 Colin Percival
 // All rights reserved
 //
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted providing that the following conditions
 // are met:
 // 1. Redistributions of source code must retain the above copyright
 //    notice, this list of conditions and the following disclaimer.
 // 2. Redistributions in binary form must reproduce the above copyright
 //    notice, this list of conditions and the following disclaimer in the
@@ skipping 22 matching lines @@
 
 // Copyright (c) 2009 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 "courgette/third_party/bsdiff/bsdiff.h"
 
 #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"
 
 namespace {
 
 using courgette::CalculateCrc;
 using courgette::SinkStream;
 using courgette::SinkStreamSet;
 using courgette::SourceStream;
@@ skipping 132 matching lines @@
     return CRC_ERROR;
 
   MBS_ApplyPatch(&header, patch_stream, old_start, old_size, new_stream);
 
   return OK;
 }
 
 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;
   BSDiffStatus status =
       ApplyBinaryPatch(&old_file_stream, &patch_file_stream, &new_sink_stream);
   if (status != OK) {
     return status;
   }
 
   // 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;
   return OK;
 }
 
 }  // namespace bsdiff