Convert utility process out-of-process file patching IPC to mojo

chrome/browser/component_updater/component_patcher_operation_out_of_process.cc

Index: chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
diff --git a/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc b/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
index 27d2795f0d86827290a38ae56d3349d3a9b9efb9..5f3b42b076edc6d7ac1bd7971b94190825bd7e26 100644
--- a/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
+++ b/chrome/browser/component_updater/component_patcher_operation_out_of_process.cc
@@ -4,132 +4,92 @@
 
 #include "chrome/browser/component_updater/component_patcher_operation_out_of_process.h"
 
-#include <memory>
-#include <vector>
-
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/files/file.h"
 #include "base/files/file_path.h"
-#include "base/location.h"
-#include "base/single_thread_task_runner.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "chrome/common/chrome_utility_messages.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/string16.h"
 #include "chrome/grit/generated_resources.h"
-#include "components/component_updater/component_updater_service.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/utility_process_host.h"
-#include "content/public/browser/utility_process_host_client.h"
-#include "courgette/courgette.h"
-#include "courgette/third_party/bsdiff/bsdiff.h"
-#include "ipc/ipc_message_macros.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace component_updater {
 
-class PatchHost : public content::UtilityProcessHostClient {
- public:
-  PatchHost(base::Callback<void(int result)> callback,
-            scoped_refptr<base::SequencedTaskRunner> task_runner);
-
-  void StartProcess(std::unique_ptr<IPC::Message> message);
-
- private:
-  ~PatchHost() override;
-
-  void OnPatchFinished(int result);
-
-  // Overrides of content::UtilityProcessHostClient.
-  bool OnMessageReceived(const IPC::Message& message) override;
-
-  void OnProcessCrashed(int exit_code) override;
-
-  base::Callback<void(int result)> callback_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
-};
-
-PatchHost::PatchHost(base::Callback<void(int result)> callback,
-                     scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : callback_(callback), task_runner_(task_runner) {
-}
+ChromeOutOfProcessPatcher::ChromeOutOfProcessPatcher() = default;
 
-PatchHost::~PatchHost() {
-}
+ChromeOutOfProcessPatcher::~ChromeOutOfProcessPatcher() = default;
 
-void PatchHost::StartProcess(std::unique_ptr<IPC::Message> message) {
-  // The DeltaUpdateOpPatchHost is not responsible for deleting the
-  // UtilityProcessHost object.
-  content::UtilityProcessHost* host = content::UtilityProcessHost::Create(
-      this, base::ThreadTaskRunnerHandle::Get().get());
-  host->SetName(l10n_util::GetStringUTF16(
-      IDS_UTILITY_PROCESS_COMPONENT_PATCHER_NAME));
-  host->Send(message.release());
-}
+void ChromeOutOfProcessPatcher::Patch(
+    const std::string& operation,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    const base::FilePath& input_path,
+    const base::FilePath& patch_path,
+    const base::FilePath& output_path,
+    base::Callback<void(int result)> callback) {
+  DCHECK(task_runner);
+  DCHECK(!callback.is_null());
+
+  task_runner_ = std::move(task_runner);
+  callback_ = callback;
+
+  base::File input_file(input_path,
+                        base::File::FLAG_OPEN | base::File::FLAG_READ);
+  base::File patch_file(patch_path,
+                        base::File::FLAG_OPEN | base::File::FLAG_READ);
+  base::File output_file(output_path, base::File::FLAG_CREATE |
+                                          base::File::FLAG_WRITE |
+                                          base::File::FLAG_EXCLUSIVE_WRITE);
+
+  if (!input_file.IsValid() || !patch_file.IsValid() ||
+      !output_file.IsValid()) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback_, -1));
+    return;
+  }
 
-bool PatchHost::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(PatchHost, message)
-  IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_PatchFile_Finished, OnPatchFinished)
-  IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&ChromeOutOfProcessPatcher::PatchOnIOThread, this, operation,
+                 base::Passed(&input_file), base::Passed(&patch_file),
+                 base::Passed(&output_file)));
 }
 
-void PatchHost::OnPatchFinished(int result) {
-  if (task_runner_.get()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback_, result));
-    task_runner_ = NULL;
-  }
-}
+void ChromeOutOfProcessPatcher::PatchOnIOThread(const std::string& operation,
+                                                base::File input_file,
+                                                base::File patch_file,
+                                                base::File output_file) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK(!utility_process_mojo_client_);
 
-void PatchHost::OnProcessCrashed(int exit_code) {
-  if (task_runner_.get()) {
-    task_runner_->PostTask(FROM_HERE, base::Bind(callback_, -1));
-    task_runner_ = NULL;
-  }
-}
+  const base::string16 utility_process_name =
+      l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_COMPONENT_PATCHER_NAME);
 
-ChromeOutOfProcessPatcher::ChromeOutOfProcessPatcher() {
-}
+  utility_process_mojo_client_.reset(
+      new content::UtilityProcessMojoClient<chrome::mojom::FilePatcher>(
+          utility_process_name));
+  utility_process_mojo_client_->set_error_callback(
+      base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this, -1));
 
-ChromeOutOfProcessPatcher::~ChromeOutOfProcessPatcher() {
-}
+  utility_process_mojo_client_->Start();  // Start the utility process.
 
-void ChromeOutOfProcessPatcher::Patch(
-    const std::string& operation,
-    scoped_refptr<base::SequencedTaskRunner> task_runner,
-    const base::FilePath& input_abs_path,
-    const base::FilePath& patch_abs_path,
-    const base::FilePath& output_abs_path,
-    base::Callback<void(int result)> callback) {
-  host_ = new PatchHost(callback, task_runner);
-  IPC::PlatformFileForTransit input = IPC::TakePlatformFileForTransit(
-      base::File(
-          input_abs_path, base::File::FLAG_OPEN | base::File::FLAG_READ));
-  IPC::PlatformFileForTransit patch = IPC::TakePlatformFileForTransit(
-      base::File(
-          patch_abs_path, base::File::FLAG_OPEN | base::File::FLAG_READ));
-  IPC::PlatformFileForTransit output = IPC::TakePlatformFileForTransit(
-      base::File(
-          output_abs_path,
-          base::File::FLAG_CREATE |
-              base::File::FLAG_WRITE |
-              base::File::FLAG_EXCLUSIVE_WRITE));
-  std::unique_ptr<IPC::Message> patch_message;
   if (operation == update_client::kBsdiff) {
-    patch_message.reset(new ChromeUtilityMsg_PatchFileBsdiff(
-        input, patch, output));
+    utility_process_mojo_client_->service()->PatchFileBsdiff(
+        std::move(input_file), std::move(patch_file), std::move(output_file),
+        base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this));
   } else if (operation == update_client::kCourgette) {
-    patch_message.reset(new ChromeUtilityMsg_PatchFileCourgette(
-        input, patch, output));
+    utility_process_mojo_client_->service()->PatchFileCourgette(
+        std::move(input_file), std::move(patch_file), std::move(output_file),
+        base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this));
   } else {
     NOTREACHED();
   }
+}
 
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(
-          &PatchHost::StartProcess, host_, base::Passed(&patch_message)));
+void ChromeOutOfProcessPatcher::PatchDone(int result) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  utility_process_mojo_client_.reset();  // Terminate the utility process.
+  task_runner_->PostTask(FROM_HERE, base::Bind(callback_, result));

[waffles, 2017/01/18 20:34:03]

I see you removed the

 if (task_runner_.get()) {
   ...
   task_runner_ = NULL
 }

wrapper.

We found that in practice sometimes the utility process could call us back
multiple times, later causing a crash when callback_ is run (because certain
objects could already be destructed). In particular, this could happen if
OnPatchFinished() but then OnProcessCrashed() (or vice versa).

Are we guaranteed that mojo (or an OOP attacker) cannot cause PatchDone to run
multiple times?

What is the behavior if the mojo process crashes? Will PatchDone ever be called?
(The purpose of running this patching out-of-process is to isolate ourselves
from crashes in the patcher, and it's not OK for such a crash to put the update
system into an unrecoverable state.)

[Noel Gordon, 2017/01/19 15:55:20]

Right, it could be racy.
Yes.  And I talked over this issue in detail with Sam today.  When PatchDone is
called, note the utility_process_mojo_client_.reset() code line - this
termination instantly disconnects the mojo channel from the utility process (the
channel enters state disconnected), and also schedules the destruction of the
utility process.

If during this disconnect phase, a message arrived from the utility process, it
would be queued for delivery as normal.  Or if the utility process crashed, a
message would be queued for that.  However, delivery of these queued message(s)
(PatchDone callbacks in our case) gets blocked at mojo level: it
drops them the floor because the channel state is disconnected.  So we see a
PatchDone exactly once.
Yes, PatchDone will be called with the error code you were using (-1).  This
handling must be setup before starting the utility process ...

70   utility_process_mojo_client_->set_error_callback(
71       base::Bind(&ChromeOutOfProcessPatcher::PatchDone, this, -1));
72
73   utility_process_mojo_client_->Start();

by setting the error callback.  utility_process_mojo_client_ DCHECK's that an
error callback has been setup during Start().  If the utility process crashes at
any time, the remote end of our mojo channel to the utility process breaks,
which queues the error callback on our side of the mojo channel. Our side sees a
PatchDone(-1).

In summary, here are the queued messages that we could arrive on the mojo
channel

1) Normal case
   PatchDone(result)

2) Normal case and then the utility process crashed
   PatchDone(result), PatchDone(-1)

3) Utility process crashed case
   PatchDone(-1)

Again, we only service the first message queued and then disconnect the mojo
channel.  Hope I have explained well, but let me know.

 }
 
 }  // namespace component_updater