New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 682794 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
please use my google.com address
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Fix unsafe EDK process launching APIs

Project Member Reported by roc...@chromium.org, Jan 19 2017

Issue description

Right now, informing the EDK of process launch means generating a GUID to identify the potential new process, and eventually notifying the EDK of launch success or failure. The latter event is required to finalize some internal state, and if not done can lead to leaks.

This is generally unsafe because it's easy to get wrong, as evidenced by all the times people have gotten it wrong.

We should replace the dumb child token string with a scoped object and improve the EDK API accordingly.
 

Comment 1 by roc...@chromium.org, Jan 19 2017

API might look something like

// mojo::edk::


class ScopedChildProcessRegistration {
 public:
  ScopedChildProcessRegistration()  // generates a GUID internally
  ~ScopedChildProcessRegistration() // invokes OnLaunchFailed if necessary

  // replaces ChildProcessLaunched()
  void OnLaunched(ProcessHandle);

  // replaces ChildProcessLaunchFailed
  void OnLaunchFailed();

  // Replaces CreateParentMessagePipe
  ScopedMessagePipeHandle CreateParentMessagePipe(const std::string& token);
};

// Must be called before parent pipes can be created
ScopedChildProcessRegistration PrepareToLaunchChildProcess();


CreateChildMessagePipe can continue to work as-is.
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1039b82bdad1ce37bd6716636e918f475ed50d13

commit 1039b82bdad1ce37bd6716636e918f475ed50d13
Author: rockot <rockot@chromium.org>
Date: Thu Feb 09 23:19:41 2017

Mojo EDK: Add safe process connection API

Replaces ad hoc ChildProcessLaunched etc functions and "child tokens"
with a scoping PendingProcessConnection object. This encapsulates the
concept of a process to which we intend to connect (e.g. so we can start
vending message pipe handles for it) but which may not yet be reachable,
and it ensures that any associated resources are reliably cleaned
up in the event that we never actually connect to the process.

This API also begins an intentional departure from the now-inaccurate
naming conventions of "parent" and "child" used at the EDK layer to
refer to some relative process relationships.

BUG= 682794 

Review-Url: https://codereview.chromium.org/2680973006
Cr-Commit-Position: refs/heads/master@{#449457}

[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/chrome/service/service_utility_process_host.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/chrome/service/service_utility_process_host.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/chrome/test/base/mojo_test_connector.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/chrome/utility/importer/firefox_importer_unittest_utils_mac.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/components/arc/arc_session.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/components/nacl/broker/nacl_broker_listener.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/browser_child_process_host_impl.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/child_process_launcher.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/child_process_launcher.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/common/child_process_host_impl.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/common/child_process_host_impl.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/common/service_manager/child_connection.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/common/service_manager/child_connection.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/public/common/BUILD.gn
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/public/common/child_process_host.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/content/renderer/render_thread_impl_browsertest.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/BUILD.gn
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/embedder.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/embedder.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/embedder_unittest.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/named_platform_channel_pair.h
[add] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/pending_process_connection.cc
[add] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/embedder/pending_process_connection.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/mojo/edk/test/multiprocess_test_helper.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/remoting/host/win/unprivileged_process_delegate.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/remoting/host/win/wts_session_process_delegate.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/README.md
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/runner/common/BUILD.gn
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/runner/common/client_util.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/runner/common/client_util.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/runner/host/service_process_launcher.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/runner/host/service_process_launcher.h
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/tests/service_manager/service_manager_unittest.cc
[modify] https://crrev.com/1039b82bdad1ce37bd6716636e918f475ed50d13/services/service_manager/tests/util.cc

Status: Fixed (was: Assigned)

Sign in to add a comment