Enable --nacl-gdb flag on Linux.

chrome/browser/nacl_host/nacl_process_host.cc

 // Copyright (c) 2012 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 "chrome/browser/nacl_host/nacl_process_host.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/mru_cache.h"
 #include "base/memory/singleton.h"
+#include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/path_service.h"
+#include "base/string_number_conversions.h"
+#include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/windows_version.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/renderer_host/chrome_render_message_filter.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
@@ skipping 236 matching lines @@
   return (base::win::OSInfo::GetInstance()->wow64_status() ==
           base::win::OSInfo::WOW64_ENABLED);
 }
 #endif
 
 NaClProcessHost::NaClProcessHost(const std::wstring& url)
     :
 #if defined(OS_WIN)
       process_launched_by_broker_(false),
 #endif
+#if defined(OS_LINUX)

[Mark Seaborn, 2012/04/03 21:57:44]

#elif for neatness.

[halyavin, 2012/04/04 09:23:15]

Done.

+      wait_for_nacl_gdb_(false),
+#endif
       reply_msg_(NULL),
       internal_(new NaClInternal()),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       enable_exception_handling_(false) {
   process_.reset(content::BrowserChildProcessHost::Create(
       content::PROCESS_TYPE_NACL_LOADER, this));
   process_->SetName(WideToUTF16Hack(url));
 
   // We allow untrusted hardware exception handling to be enabled via
   // an env var for consistency with the standalone build of NaCl.
@@ skipping 119 matching lines @@
     internal_->sockets_for_sel_ldr.push_back(pair[1]);
     SetCloseOnExec(pair[0]);
     SetCloseOnExec(pair[1]);
   }
 
   // Launch the process
   if (!LaunchSelLdr()) {
     delete this;
   }
 }
-
+#if defined(OS_WIN)
 scoped_ptr<CommandLine> NaClProcessHost::LaunchWithNaClGdb(
     const FilePath& nacl_gdb,
-    CommandLine* line,
-    const FilePath& manifest_path) {
+    CommandLine* line) {
   CommandLine* cmd_line = new CommandLine(nacl_gdb);
   // We can't use PrependWrapper because our parameters contain spaces.
-  cmd_line->AppendArg("--eval-command");
+  cmd_line->AppendArg("-ex");

[Mark Seaborn, 2012/04/03 21:57:44]

Please stick with --eval-command since it's more obvious what it does [from
previous review].

[halyavin, 2012/04/04 09:23:15]

Done.

   const FilePath::StringType& irt_path =
       NaClBrowser::GetInstance()->GetIrtFilePath().value();
   cmd_line->AppendArgNative(FILE_PATH_LITERAL("nacl-irt ") + irt_path);
+  FilePath manifest_path = GetManifestPath();
   if (!manifest_path.empty()) {
-    cmd_line->AppendArg("--eval-command");
+    cmd_line->AppendArg("-ex");

[Mark Seaborn, 2012/04/03 21:57:44]

ditto

     cmd_line->AppendArgNative(FILE_PATH_LITERAL("nacl-manifest ") +
                               manifest_path.value());
   }
   cmd_line->AppendArg("--args");
   const CommandLine::StringVector& argv = line->argv();
   for (size_t i = 0; i < argv.size(); i++) {
     cmd_line->AppendArgNative(argv[i]);
   }
   return scoped_ptr<CommandLine>(cmd_line);
 }
+#elif defined(OS_LINUX)
+namespace {
+class NaClGdbWatchDelegate : public MessageLoopForIO::Watcher {
+ public:
+  NaClGdbWatchDelegate(int fds_read, int fds_write,
+                       const base::Closure& reply)
+      : fds_read_(fds_read),
+        fds_write_(fds_write),

[Mark Seaborn, 2012/04/03 21:57:44]

Why are you passing fds_write through?  It's not used.  Keeping it open in the
browser means you will never see an EOF on fd_read.

[halyavin, 2012/04/04 09:23:15]

It is used by nacl-gdb. It opens fds_write in our process.

+        reply_(reply) {}
+
+  ~NaClGdbWatchDelegate() {
+    close(fds_read_);

[Mark Seaborn, 2012/04/03 21:57:44]

Please do LOG(ERROR) on error from close(), otherwise mistakes in use of file
descriptors will go unnoticed (such mistakes are potentially as dangerous as
memory unsafety).

[halyavin, 2012/04/04 09:23:15]

Done.

+    close(fds_write_);
+  }
+
+  virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE;
+  virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {}
+
+ private:
+  int fds_read_;

[Mark Seaborn, 2012/04/03 21:57:44]

Should be "fd" not "fds".

[halyavin, 2012/04/04 09:23:15]

Done.

+  int fds_write_;
+  base::Closure reply_;
+};
+
+void NaClGdbWatchDelegate::OnFileCanReadWithoutBlocking(int fd) {
+  char buf;
+  if (read(fds_read_, &buf, 1) != 1)
+    LOG(ERROR) << "Failed to sync with nacl-gdb";

[Mark Seaborn, 2012/04/03 21:57:44]

Suggestion: you might also want to sanity check that you receive the same byte
that you send from gdb (and send a char other than 0).

[halyavin, 2012/04/04 09:23:15]

I added sanity check but I don't think using non-zero char is necessary.

+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, reply_);
+}
+}  // namespace
+
+bool NaClProcessHost::LaunchNaClGdb(base::ProcessId pid) {
+  CommandLine::StringType nacl_gdb =
+      CommandLine::ForCurrentProcess()->GetSwitchValueNative(
+          switches::kNaClGdb);
+  CommandLine::StringVector argv;
+  base::SplitString(nacl_gdb, static_cast<CommandLine::CharType>(' '), &argv);

[Mark Seaborn, 2012/04/03 21:57:44]

Can you put a comment in here to say that we don't support spaces inside
arguments?

[halyavin, 2012/04/04 09:23:15]

Done.

+  CommandLine cmd_line(argv);
+  cmd_line.AppendArg("-ex");

[Mark Seaborn, 2012/04/03 21:57:44]

Please use --eval-command here and below.

+  const FilePath::StringType& irt_path =
+        NaClBrowser::GetInstance()->GetIrtFilePath().value();

[Mark Seaborn, 2012/04/03 21:57:44]

Indent -2

[halyavin, 2012/04/04 09:23:15]

Done.

+  cmd_line.AppendArgNative(FILE_PATH_LITERAL("nacl-irt ") + irt_path);
+  FilePath manifest_path = GetManifestPath();
+  if (!manifest_path.empty()) {
+    cmd_line.AppendArg("-ex");
+    cmd_line.AppendArgNative(FILE_PATH_LITERAL("nacl-manifest ") +
+                             manifest_path.value());
+  }
+  cmd_line.AppendArg("-ex");
+  cmd_line.AppendArg("attach " + base::IntToString(pid));
+  int fds[2];
+  if (pipe(fds) != 0)
+    return false;
+  cmd_line.AppendArg("-ex");
+  cmd_line.AppendArg("dump binary value /proc/" +
+                     base::IntToString(base::GetCurrentProcId()) +
+                     "/fd/" + base::IntToString(fds[1]) + " (char)0");
+  // wait on fds[0]
+  nacl_gdb_watcher_delegate_.reset(
+      new NaClGdbWatchDelegate(
+          fds[0], fds[1],
+          base::Bind(&NaClProcessHost::OnNaClGdbAttached,
+                     weak_factory_.GetWeakPtr())));
+  MessageLoopForIO::current()->WatchFileDescriptor(
+      fds[0],
+      true,
+      MessageLoopForIO::WATCH_READ,
+      &nacl_gdb_watcher_,
+      nacl_gdb_watcher_delegate_.get());
+  return base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);

[Mark Seaborn, 2012/04/03 21:57:44]

This seems to suggest that all FDs, including fds[1], are inherited, which would
be bad.  But process_util.h seems to suggest that only stdin/stdout/stderr are
left open, in which case how does this work?

Anyway, you should use fds_to_remap in the launch options.

[halyavin, 2012/04/04 09:23:15]

nacl-gdb opens file descriptor in our process. I experimented with fds_to_remap
and found that a lot of terminals (gnome-terminal for example) close all file
descriptors before launching external processes. Since launching gdb in terminal
is a requirement and user will be confused why some terminals work and some
doesn't, I decided not to use this approach.

[Mark Seaborn, 2012/04/04 15:46:12]

I missed that.  It's totally non-obvious.  It definitely needs a comment.

It also means that if something goes wrong and gdb dies, the browser will not
receive EOF on the pipe, and the NaCl process's startup will get stuck, and I
think these data structures in the browser will leak.  This needs a comment too.

I'm a bit uneasy about having another process write to the browser's FDs.  If it
goes wrong, it will mess up some random FD in the browser.

Another solution might be to have a shell script that wraps gdb and does this:

#!/bin/bash
notify_fd="$1"
shift
# Can't use "exec" because we have to stay running for /proc/$$ to work
gnome-terminal -x gdb "$@" --eval-command "dump binary value
/proc/$$/fd/$notify_fd (char)0"

This would make me happier because it would keep the workarounds for others'
code out of Chromium's codebase.  Also, you'd no longer need to split the
--nacl-gdb arg on spaces, so this would be more consistent with the Windows code
path.  You're already providing nacl64-gdb; you could provide example wrapper
scripts with it, such as gdb-gnome-terminal.

This might not work because the gnome-terminal command sometimes returns
immediately.

+}
+
+void NaClProcessHost::OnNaClGdbAttached() {
+  wait_for_nacl_gdb_ = false;
+  nacl_gdb_watcher_.StopWatchingFileDescriptor();
+  nacl_gdb_watcher_delegate_.reset();
+  OnProcessLaunched();
+}
+#endif
+
+FilePath NaClProcessHost::GetManifestPath() {
+  GURL manifest_url = GURL(process_->GetData().name);
+  const Extension* extension = extension_info_map_->extensions()
+      .GetExtensionOrAppByURL(ExtensionURLInfo(manifest_url));
+  if (extension != NULL && manifest_url.SchemeIs(chrome::kExtensionScheme)) {
+    std::string path = manifest_url.path();
+    TrimString(path, "/", &path);  // Remove first slash
+    return extension->path().AppendASCII(path);
+  }
+  return FilePath();
+}
 
 bool NaClProcessHost::LaunchSelLdr() {
   std::string channel_id = process_->GetHost()->CreateChannel();
   if (channel_id.empty())
     return false;
 
   CommandLine::StringType nacl_loader_prefix;
 #if defined(OS_POSIX)
   nacl_loader_prefix = CommandLine::ForCurrentProcess()->GetSwitchValueNative(
       switches::kNaClLoaderCmdPrefix);
@@ skipping 38 matching lines @@
   cmd_line->AppendSwitchASCII(switches::kProcessChannelID, channel_id);
   if (logging::DialogsAreSuppressed())
     cmd_line->AppendSwitch(switches::kNoErrorDialogs);
 
   if (!nacl_loader_prefix.empty())
     cmd_line->PrependWrapper(nacl_loader_prefix);
 
   FilePath nacl_gdb = CommandLine::ForCurrentProcess()->GetSwitchValuePath(
       switches::kNaClGdb);
   if (!nacl_gdb.empty()) {
-    GURL manifest_url = GURL(process_->GetData().name);
-    FilePath manifest_path;
-    const Extension* extension = extension_info_map_->extensions().
-        GetExtensionOrAppByURL(ExtensionURLInfo(manifest_url));
-    if (extension != NULL && manifest_url.SchemeIs(chrome::kExtensionScheme)) {
-      std::string path = manifest_url.path();
-      TrimString(path, "/", &path);  // Remove first slash
-      manifest_path = extension->path().AppendASCII(path);
-    }
+#if defined(OS_WIN)
     cmd_line->AppendSwitch(switches::kNoSandbox);
     scoped_ptr<CommandLine> gdb_cmd_line(
-        LaunchWithNaClGdb(nacl_gdb, cmd_line.get(), manifest_path));
+        LaunchWithNaClGdb(nacl_gdb, cmd_line.get()));
     // We can't use process_->Launch() because OnProcessLaunched will be called
     // with process_->GetData().handle filled by handle of gdb process. This
     // handle will be used to duplicate handles for NaCl process and as
     // a result NaCl process will not be able to use them.
     //
     // So we don't fill process_->GetData().handle and wait for
     // OnChannelConnected to get handle of NaCl process from its pid. Then we
     // call OnProcessLaunched.
     return base::LaunchProcess(*gdb_cmd_line, base::LaunchOptions(), NULL);
+#elif defined(OS_LINUX)
+    wait_for_nacl_gdb_ = true;
+#endif
   }
 
   // On Windows we might need to start the broker process to launch a new loader
 #if defined(OS_WIN)
   if (RunningOnWOW64()) {
     return NaClBrokerService::GetInstance()->LaunchLoader(this, channel_id);
   } else {
     process_->Launch(FilePath(), cmd_line.release());
   }
 #elif defined(OS_POSIX)
@@ skipping 94 matching lines @@
                                                 NULL,
                                                 &error_code);
   if (error_code != base::PLATFORM_FILE_OK) {
     LOG(ERROR) << "Failed to open NaCl IRT file \""
                << irt_filepath_.LossyDisplayName()
                << "\": " << error_code;
   }
 }
 
 void NaClProcessHost::OnProcessLaunched() {
+#if defined(OS_LINUX)
+  if (wait_for_nacl_gdb_) {
+    if (LaunchNaClGdb(base::GetProcId(process_->GetData().handle))) {
+      // OnProcessLaunched will be called with wait_for_nacl_gdb_ = false once
+      // debugger is attached to the program.
+      return;
+    }
+    LOG(ERROR) << "Cannot launch debugger";

[Mark Seaborn, 2012/04/03 21:57:44]

"Failed to launch debugger" might be better

[halyavin, 2012/04/04 09:23:15]

Done.

+    // Continue execution without debugger.
+  }
+#endif
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   if (nacl_browser->IrtAvailable()) {
     // The IRT is already open.  Away we go.
     SendStart();
   } else {
     // We're waiting for the IRT to be open.
     if (!nacl_browser->MakeIrtAvailable(
             base::Bind(&NaClProcessHost::IrtReady,
                        weak_factory_.GetWeakPtr())))
@@ skipping 277 matching lines @@
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
     IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
                         OnQueryKnownToValidate)
     IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
                         OnSetKnownToValidate)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }