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

shell/out_of_process_native_runner.cc

Index: shell/out_of_process_native_runner.cc
diff --git a/shell/out_of_process_native_runner.cc b/shell/out_of_process_native_runner.cc
index 39cf1f7003abfef747ed59f21afdebf9d08b04ab..b5c24c7de7a705e7e9cee45b53304b137831b0ed 100644
--- a/shell/out_of_process_native_runner.cc
+++ b/shell/out_of_process_native_runner.cc
@@ -4,10 +4,14 @@
 
 #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"
@@ -37,7 +41,14 @@ void OutOfProcessNativeRunner::Start(
   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(