Enabling 64-bit mojo shell to launch 32-bit child to handle nonsfi content.

shell/out_of_process_native_runner.cc

 // Copyright 2014 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 "shell/out_of_process_native_runner.h"
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/strings/string_util.h"
+#include "elf.h"

[Mark Seaborn, 2015/09/15 19:42:58]

Should be <elf.h> since this is a system header

[Sean Klein, 2015/09/15 21:35:40]

Done.

 #include "shell/child_controller.mojom.h"
 #include "shell/child_process_host.h"
 #include "shell/in_process_native_runner.h"
 
 namespace shell {
 
 OutOfProcessNativeRunner::OutOfProcessNativeRunner(Context* context)
     : context_(context) {
 }
 
 OutOfProcessNativeRunner::~OutOfProcessNativeRunner() {
   if (child_process_host_) {
     // TODO(vtl): Race condition: If |ChildProcessHost::DidStart()| hasn't been
     // called yet, we shouldn't call |Join()| here. (Until |DidStart()|, we may
     // not have a child process to wait on.) Probably we should fix |Join()|.
     child_process_host_->Join();
   }
 }
 
 void OutOfProcessNativeRunner::Start(
     const base::FilePath& app_path,
     mojo::InterfaceRequest<mojo::Application> application_request,
     const base::Closure& app_completed_callback) {
   app_path_ = app_path;
 
   DCHECK(app_completed_callback_.is_null());
   app_completed_callback_ = app_completed_callback;
 
   child_process_host_.reset(new ChildProcessHost(context_));
-  child_process_host_->Start();
+
+  bool require_32_bit = false;
+  char data[EI_NIDENT];

[Mark Seaborn, 2015/09/15 19:42:58]

This new code could also be conditionalised on "#if defined(ARCH_CPU_X86_64)".

[Sean Klein, 2015/09/15 21:35:40]

It seems a bit misleading to use a conditional here and return a unique value
for 32 bit architectures just to ignore it later.

I will include the shortcut for 32 bit systems regardless.

+  if (EI_NIDENT != ReadFile(app_path, data, EI_NIDENT))

[Mark Seaborn, 2015/09/15 19:42:58]

How about ReadFile(app_path, data, sizeof(data)), to better ensure the size is
in sync?

[Sean Klein, 2015/09/15 21:35:40]

Done.

+    assert(false);

[Mark Seaborn, 2015/09/15 19:42:58]

Chromium style is to avoid assert() and use CHECK() instead.

However, if the file is too small, I'm not sure you want to abort here.  Maybe
it would be better to assume require_32_bit = false and just let the process
startup fail later.

[Sean Klein, 2015/09/15 21:35:40]

This seems misleading to me (I am not a fan of silent errors), but I have
updated it to this version.

[Mark Seaborn, 2015/09/15 22:25:43]

Maybe:

size_t bytes_read = ReadFile(...);
DCHECK_EQ(bytes_read, sizeof(data));
if (bytes_read != sizeof(data))
  return false;

+  if (data[EI_CLASS] == ELFCLASS32)
+    require_32_bit = true;

[Mark Seaborn, 2015/09/15 19:42:58]

How about splitting this new code into a Require32Bit(filename) function?

[Sean Klein, 2015/09/15 21:35:40]

Done.

+  child_process_host_->Start(require_32_bit);
 
   // TODO(vtl): |app_path.AsUTF8Unsafe()| is unsafe.
   child_process_host_->StartApp(
       app_path.AsUTF8Unsafe(), application_request.Pass(),
       base::Bind(&OutOfProcessNativeRunner::AppCompleted,
                  base::Unretained(this)));
 }
 
 void OutOfProcessNativeRunner::AppCompleted(int32_t result) {
   DVLOG(2) << "OutOfProcessNativeRunner::AppCompleted(" << result << ")";
@@ skipping 10 matching lines @@
 
 scoped_ptr<NativeRunner> OutOfProcessNativeRunnerFactory::Create(
     const Options& options) {
   if (options.force_in_process)
     return make_scoped_ptr(new InProcessNativeRunner(context_));
 
   return make_scoped_ptr(new OutOfProcessNativeRunner(context_));
 }
 
 }  // namespace shell