New issue
Advanced search Search tips

Issue 716206 link

Starred by 2 users
Status: Fixed
Owner:
erikc...@chromium.org
Closed: May 2017
Cc:
etienneb@chromium.org
thakis@chromium.org
mark@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Get rid of base::SharedMemory::ShareToProcessCommon

Project Member Reported by erikc...@chromium.org, Apr 27 2017 
Stages:
1) Make sure that all IPC is explicit about whether the underlying handle belongs to the current process or the destination process
2) Add support for transport of native handles to IPC.
3) Replace all calls to ShareToProcessCommon which duplicate the handle into the destination process with calls that duplicate in the current process, and let IPC take care of transport.

(1) and (2) have been finished since the introduction of attachment brokering [which has since been replaced/subsumed by Mojo]. Now we just need to follow up with some clean up.

Comment 1 by erikc...@chromium.org, Apr 28 2017 


The reason we want to get rid of ShareToProcessCommon is that it allows the caller to copy a handle into the destination process, which:
  1) Requires privileges, which the caller may or may not have.
  2) Makes keeping track of handles very difficult, since the destination process is now responsible for freeing the handle.

Instead, the caller should just let IPC do all the transport of handles.

Comment 2 by erikc...@chromium.org, May 1 2017

Cc: etienneb@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, May 2 2017 

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

commit c87903ecca1345c4363da94e0120adedd83963b0
Author: erikchen <erikchen@chromium.org>
Date: Tue May 02 19:05:01 2017

Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG= 716206 
TBR=brettw@chromium.org

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

[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory.h
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_android.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_helper.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_mac_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_win.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/metrics/field_trial.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/content/browser/renderer_host/clipboard_message_filter_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/extensions/browser/user_script_loader.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/mojo/edk/embedder/platform_shared_buffer.cc
Project Member

Comment 4 by bugdroid1@chromium.org, May 2 2017 

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

commit c87903ecca1345c4363da94e0120adedd83963b0
Author: erikchen <erikchen@chromium.org>
Date: Tue May 02 19:05:01 2017

Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG= 716206 
TBR=brettw@chromium.org

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

[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory.h
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_android.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_helper.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_mac_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/memory/shared_memory_win.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/base/metrics/field_trial.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/content/browser/renderer_host/clipboard_message_filter_unittest.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/extensions/browser/user_script_loader.cc
[modify] https://crrev.com/c87903ecca1345c4363da94e0120adedd83963b0/mojo/edk/embedder/platform_shared_buffer.cc
Project Member

Comment 5 by bugdroid1@chromium.org, May 2 2017 

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

commit 63840887ae819fb18b2103ac939db69e35ffe111
Author: erikchen <erikchen@chromium.org>
Date: Tue May 02 20:52:31 2017

Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG= 716206 
TBR=avi@chromium.org

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

[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory.h
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_handle.h
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_handle_win.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/base/memory/shared_memory_win.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/content/browser/renderer_host/render_message_filter.cc
[modify] https://crrev.com/63840887ae819fb18b2103ac939db69e35ffe111/content/renderer/pepper/ppb_image_data_impl.cc
Project Member

Comment 6 by bugdroid1@chromium.org, May 3 2017 

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

commit 37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd
Author: erikchen <erikchen@chromium.org>
Date: Wed May 03 04:05:02 2017

Remove base::SharedMemory::ShareToProcess.

This CL is a refactor and has no intended behavior change.

The method was initially introduced to assist in IPC, before Chrome's IPC
subsystem was capable of transporting handles across processes. Now that the
IPC subsystem does have the capability, the functionality is no longer needed
in base::SharedMemory.

BUG= 716206 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=brettw@chromium.org

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

[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/discardable_shared_memory.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/discardable_shared_memory_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_handle.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_mac_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_nacl.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_posix.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/memory/shared_memory_win.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/base/metrics/persistent_memory_allocator_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/chrome/renderer/pepper/pepper_shared_memory_message_filter.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/components/display_compositor/host_shared_bitmap_manager_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/components/nacl/browser/nacl_process_host.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/android/synchronous_compositor_host.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/browser_child_process_host_impl.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/renderer_host/media/audio_input_renderer_host.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/renderer_host/media/audio_renderer_host.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/renderer_host/pepper/pepper_gamepad_host.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/renderer_host/pepper/pepper_gamepad_host.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/child/resource_dispatcher_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/common/sandbox_mac_fontloading_unittest.mm
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/renderer/media/audio_message_filter_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/renderer/media/speech_recognition_audio_sink_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/renderer/pepper/pepper_platform_audio_output.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/content/renderer/pepper/pepper_platform_audio_output_dev.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/device/gamepad/gamepad_provider.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/device/gamepad/gamepad_provider.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/device/gamepad/gamepad_provider_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/device/gamepad/gamepad_service.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/device/gamepad/gamepad_service.h
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/extensions/browser/user_script_loader.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/gpu/ipc/service/gpu_channel_test_common.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/media/audio/audio_input_device_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/media/audio/audio_output_device_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/media/gpu/video_decode_accelerator_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/media/gpu/video_encode_accelerator_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/ppapi/proxy/video_decoder_resource_unittest.cc
[modify] https://crrev.com/37e7d8301a078bd1b6b3ef7cffdb6c6ac3277edd/ppapi/proxy/video_encoder_resource_unittest.cc
Project Member

Comment 7 by bugdroid1@chromium.org, May 3 2017 

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

commit 996f592abbeaa1b6a14d0fa4073e709cf57b5207
Author: erikchen <erikchen@chromium.org>
Date: Wed May 03 17:12:30 2017

Get rid of all pid references from base::SharedMemoryHandle.

base::SharedMemory used to support synchronous transport of handles into other
processes using base::SharedMemory::ShareToProcess. This required
base::SharedMemoryHandle to also track whether the OS resource belonged to the
current process. ShareToProcess has been removed, so all the pid tracking is no
longer necessary.

BUG= 716206 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
TBR=wfh@chromium.org

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

[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_handle.h
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_handle_mac.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_handle_win.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_mac.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_mac_unittest.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_unittest.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/memory/shared_memory_win.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/base/metrics/field_trial.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/ipc/ipc_message_utils.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/mojo/edk/embedder/platform_shared_buffer.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/mojo/edk/system/platform_wrapper_unittest.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/mojo/public/cpp/system/platform_handle.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/sandbox/win/src/process_mitigations_win32k_dispatcher.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/sandbox/win/tests/common/controller.cc
[modify] https://crrev.com/996f592abbeaa1b6a14d0fa4073e709cf57b5207/ui/gfx/mojo/buffer_types_struct_traits.cc

Comment 8 by erikc...@chromium.org, May 4 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment