[dart:io,fuchsia] Pass Isolate namespace to child process. Fix double free.

Before this change, the child process inherits the namespace of the
process. After this change the child process inherits the namespace of
the calling isolate.

Related: DX-710

Change-Id: Idbc72e30f5796f8034cedae776d4572ad0b0360f
Reviewed-on: https://dart-review.googlesource.com/c/89460
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Zach Anderson <zra@google.com>

runtime/bin/io_impl_sources.gni

@@ -37,6 +37,7 @@ io_impl_sources = [
   "namespace.h",
   "namespace_android.cc",
   "namespace_fuchsia.cc",
+  "namespace_fuchsia.h",
   "namespace_linux.cc",
   "namespace_macos.cc",
   "namespace_win.cc",

runtime/bin/namespace_fuchsia.cc

@@ -6,6 +6,7 @@
 #if defined(HOST_OS_FUCHSIA)
 
 #include "bin/namespace.h"
+#include "bin/namespace_fuchsia.h"
 
 #include <errno.h>
 #include <fcntl.h>
@@ -19,82 +20,67 @@
 namespace dart {
 namespace bin {
 
-class NamespaceImpl {
- public:
-  explicit NamespaceImpl(fdio_ns_t* fdio_ns)
+NamespaceImpl::NamespaceImpl(fdio_ns_t* fdio_ns)
       : fdio_ns_(fdio_ns),
         rootfd_(fdio_ns_opendir(fdio_ns)),
         cwd_(strdup("/")) {
-    ASSERT(rootfd_ > 0);
-    cwdfd_ = dup(rootfd_);
-    ASSERT(cwdfd_ > 0);
-  }
+  ASSERT(rootfd_ > 0);
+  cwdfd_ = dup(rootfd_);
+  ASSERT(cwdfd_ > 0);
+}
 
-  explicit NamespaceImpl(const char* path)
+NamespaceImpl::NamespaceImpl(const char* path)
       : fdio_ns_(NULL),
         rootfd_(TEMP_FAILURE_RETRY(open(path, O_DIRECTORY))),
         cwd_(strdup("/")) {
-    ASSERT(rootfd_ > 0);
-    cwdfd_ = dup(rootfd_);
-    ASSERT(cwdfd_ > 0);
-  }
+  ASSERT(rootfd_ > 0);
+  cwdfd_ = dup(rootfd_);
+  ASSERT(cwdfd_ > 0);
+}
 
-  ~NamespaceImpl() {
-    NO_RETRY_EXPECTED(close(rootfd_));
-    free(cwd_);
-    NO_RETRY_EXPECTED(close(cwdfd_));
-    if (fdio_ns_ != NULL) {
-      zx_status_t status = fdio_ns_destroy(fdio_ns_);
-      if (status != ZX_OK) {
-        Log::PrintErr("fdio_ns_destroy: %s\n", zx_status_get_string(status));
-      }
+NamespaceImpl::~NamespaceImpl() {
+  NO_RETRY_EXPECTED(close(rootfd_));
+  free(cwd_);
+  NO_RETRY_EXPECTED(close(cwdfd_));
+  if (fdio_ns_ != NULL) {
+    zx_status_t status = fdio_ns_destroy(fdio_ns_);
+    if (status != ZX_OK) {
+      Log::PrintErr("fdio_ns_destroy: %s\n", zx_status_get_string(status));
     }
   }
+}
 
-  intptr_t rootfd() const { return rootfd_; }
-  char* cwd() const { return cwd_; }
-  intptr_t cwdfd() const { return cwdfd_; }
-
-  bool SetCwd(Namespace* namespc, const char* new_path) {
-    NamespaceScope ns(namespc, new_path);
-    const intptr_t new_cwdfd =
-        TEMP_FAILURE_RETRY(openat(ns.fd(), ns.path(), O_DIRECTORY));
-    if (new_cwdfd != 0) {
-      return false;
-    }
-
-    // Build the new cwd.
-    TextBuffer tbuf(PATH_MAX);
-    if (!File::IsAbsolutePath(new_path)) {
-      tbuf.AddString(cwd_);
-    }
-    tbuf.AddString(File::PathSeparator());
-    tbuf.AddString(ns.path());
-
-    // Normalize it.
-    char result[PATH_MAX];
-    const intptr_t result_len =
-        File::CleanUnixPath(tbuf.buf(), result, PATH_MAX);
-    if (result_len < 0) {
-      errno = ENAMETOOLONG;
-      return false;
-    }
-
-    free(cwd_);
-    cwd_ = strdup(result);
-    close(cwdfd_);
-    cwdfd_ = new_cwdfd;
-    return true;
+bool NamespaceImpl::SetCwd(Namespace* namespc, const char* new_path) {
+  NamespaceScope ns(namespc, new_path);
+  const intptr_t new_cwdfd =
+      TEMP_FAILURE_RETRY(openat(ns.fd(), ns.path(), O_DIRECTORY));
+  if (new_cwdfd != 0) {
+    return false;
   }
 
- private:
-  fdio_ns_t* fdio_ns_;  // native namespace object, if any.
-  intptr_t rootfd_;     // dirfd for the namespace root.
-  char* cwd_;           // cwd relative to the namespace.
-  intptr_t cwdfd_;      // dirfd for the cwd.
+  // Build the new cwd.
+  TextBuffer tbuf(PATH_MAX);
+  if (!File::IsAbsolutePath(new_path)) {
+    tbuf.AddString(cwd_);
+  }
+  tbuf.AddString(File::PathSeparator());
+  tbuf.AddString(ns.path());
 
-  DISALLOW_COPY_AND_ASSIGN(NamespaceImpl);
-};
+  // Normalize it.
+  char result[PATH_MAX];
+  const intptr_t result_len =
+      File::CleanUnixPath(tbuf.buf(), result, PATH_MAX);
+  if (result_len < 0) {
+    errno = ENAMETOOLONG;
+    return false;
+  }
+
+  free(cwd_);
+  cwd_ = strdup(result);
+  close(cwdfd_);
+  cwdfd_ = new_cwdfd;
+  return true;
+}
 
 Namespace* Namespace::Create(intptr_t namespc) {
   NamespaceImpl* namespc_impl = NULL;

runtime/bin/namespace_fuchsia.h

@@ -0,0 +1,43 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#ifndef RUNTIME_BIN_NAMESPACE_FUCHSIA_H_
+#define RUNTIME_BIN_NAMESPACE_FUCHSIA_H_
+
+#include "platform/globals.h"
+#if !defined(HOST_OS_FUCHSIA)
+#error "This header file should only be included when targeting Fuchsia."
+#endif
+
+#include <lib/fdio/namespace.h>
+
+namespace dart {
+namespace bin {
+
+class NamespaceImpl {
+ public:
+  explicit NamespaceImpl(fdio_ns_t* fdio_ns);
+  explicit NamespaceImpl(const char* path);
+  ~NamespaceImpl();
+
+  intptr_t rootfd() const { return rootfd_; }
+  char* cwd() const { return cwd_; }
+  intptr_t cwdfd() const { return cwdfd_; }
+  fdio_ns_t* fdio_ns() const { return fdio_ns_; }
+
+  bool SetCwd(Namespace* namespc, const char* new_path);
+
+ private:
+  fdio_ns_t* fdio_ns_;  // native namespace object, if any.
+  intptr_t rootfd_;     // dirfd for the namespace root.
+  char* cwd_;           // cwd relative to the namespace.
+  intptr_t cwdfd_;      // dirfd for the cwd.
+
+  DISALLOW_COPY_AND_ASSIGN(NamespaceImpl);
+};
+
+}  // namespace bin
+}  // namespace dart
+
+#endif  // RUNTIME_BIN_NAMESPACE_FUCHSIA_H_

runtime/bin/process_fuchsia.cc

@@ -34,6 +34,7 @@
 #include "bin/lockers.h"
 #include "bin/log.h"
 #include "bin/namespace.h"
+#include "bin/namespace_fuchsia.h"
 #include "platform/signal_blocker.h"
 #include "platform/utils.h"
 
@@ -346,18 +347,9 @@ bool Process::Wait(intptr_t pid,
                    intptr_t err,
                    intptr_t exit_event,
                    ProcessResult* result) {
-  // input not needed.
-  IOHandle* in_iohandle = reinterpret_cast<IOHandle*>(in);
-  in_iohandle->Close();
-  in_iohandle->Release();
-  in_iohandle = NULL;
-
   IOHandle* out_iohandle = reinterpret_cast<IOHandle*>(out);
   IOHandle* err_iohandle = reinterpret_cast<IOHandle*>(err);
   IOHandle* exit_iohandle = reinterpret_cast<IOHandle*>(exit_event);
-  IOHandleScope out_ioscope(out_iohandle);
-  IOHandleScope err_ioscope(err_iohandle);
-  IOHandleScope exit_ioscope(exit_iohandle);
 
   // There is no return from this function using Dart_PropagateError
   // as memory used by the buffer lists is freed through their
@@ -597,15 +589,14 @@ class ProcessStarter {
       return status;
     }
 
-    fdio_spawn_action_t actions[4];
-    memset(actions, 0, sizeof(actions));
-    AddPipe(0, &write_out_, &actions[0]);
-    AddPipe(1, &read_in_, &actions[1]);
-    AddPipe(2, &read_err_, &actions[2]);
-    actions[3] = {
-      .action = FDIO_SPAWN_ACTION_SET_NAME,
-      .name.data = program_arguments_[0],
-    };
+    fdio_spawn_action_t* actions;
+    const intptr_t actions_count = BuildSpawnActions(
+        namespc_->namespc()->fdio_ns(), &actions);
+    if (actions_count < 0) {
+      *os_error_message_ = DartUtils::ScopedCopyCString(
+          "Failed to build spawn actions array.");
+      return ZX_ERR_IO;
+    }
 
     // TODO(zra): Use the supplied working directory when fdio_spawn_vmo adds an
     // API to set it.
@@ -613,12 +604,13 @@ class ProcessStarter {
     LOG_INFO("ProcessStarter: Start() Calling fdio_spawn_vmo\n");
     zx_handle_t process = ZX_HANDLE_INVALID;
     char err_msg[FDIO_SPAWN_ERR_MSG_MAX_LENGTH];
-    uint32_t flags = FDIO_SPAWN_CLONE_JOB | FDIO_SPAWN_CLONE_LDSVC |
-        FDIO_SPAWN_CLONE_NAMESPACE;
+    uint32_t flags = FDIO_SPAWN_CLONE_JOB | FDIO_SPAWN_CLONE_LDSVC;
     status =
         fdio_spawn_vmo(ZX_HANDLE_INVALID, flags, vmo, program_arguments_,
-                       program_environment_, 4, actions, &process, err_msg);
-
+                       program_environment_, actions_count, actions, &process,
+                       err_msg);
+    // Handles are consumed by fdio_spawn_vmo even if it fails.
+    delete[] actions;
     if (status != ZX_OK) {
       LOG_ERR("ProcessStarter: Start() fdio_spawn_vmo failed\n");
       close(exit_pipe_fds[0]);
@@ -681,6 +673,54 @@ class ProcessStarter {
     return ZX_OK;
   }
 
+  // Fills in 'actions_out' and returns action count.
+  intptr_t BuildSpawnActions(fdio_ns_t* ns, fdio_spawn_action_t** actions_out) {
+    const intptr_t fixed_actions_cnt = 4;
+    intptr_t ns_cnt = 0;
+
+    // First, figure out how many namespace actions are needed.
+    fdio_flat_namespace_t* flat_ns = nullptr;
+    if (ns != nullptr) {
+      zx_status_t status = fdio_ns_export(ns, &flat_ns);
+      if (status != ZX_OK) {
+        LOG_ERR("ProcessStarter: BuildSpawnActions: fdio_ns_export: %s\n",
+                zx_status_get_string(status));
+        return -1;
+      }
+      ns_cnt = flat_ns->count;
+    }
+
+    // Allocate the actions array.
+    const intptr_t actions_cnt = ns_cnt + fixed_actions_cnt;
+    fdio_spawn_action_t* actions = new fdio_spawn_action_t[actions_cnt];
+
+    // Fill in the entries for passing stdin/out/err handles, and the program
+    // name.
+    AddPipe(0, &write_out_, &actions[0]);
+    AddPipe(1, &read_in_, &actions[1]);
+    AddPipe(2, &read_err_, &actions[2]);
+    actions[3] = {
+      .action = FDIO_SPAWN_ACTION_SET_NAME,
+      .name.data = program_arguments_[0],
+    };
+
+    // Then fill in the namespace actions.
+    if (ns != nullptr) {
+      for (size_t i = 0; i < flat_ns->count; i++) {
+        actions[fixed_actions_cnt + i] = {
+          .action = FDIO_SPAWN_ACTION_ADD_NS_ENTRY,
+          .ns.prefix = flat_ns->path[i],
+          .ns.handle = flat_ns->handle[i],
+        };
+      }
+      free(flat_ns);
+      flat_ns = nullptr;
+    }
+
+    *actions_out = actions;
+    return actions_cnt;
+  }
+
   int read_in_;    // Pipe for stdout to child process.
   int read_err_;   // Pipe for stderr to child process.
   int write_out_;  // Pipe for stdin to child process.

runtime/vm/thread_interrupter_fuchsia.cc

@@ -159,7 +159,7 @@ class ThreadInterrupterFuchsia : public AllStatic {
     // Currently we sample only threads that are associated
     // with an isolate. It is safe to call 'os_thread->thread()'
     // here as the thread which is being queried is suspended.
-    Thread* thread = os_thread->thread();
+    Thread* thread = static_cast<Thread*>(os_thread->thread());
     if (thread != NULL) {
       Profiler::SampleThread(thread, its);
     }