base::GetProcId should CHECK if passed an invalid handle |
||||||||
Issue descriptionAs part of crrev.com/c/1145767, which was part of getting Chrome to run cleanly with App Verifier enabled, this implementation of GetProcId was written: ProcessId GetProcId(ProcessHandle process) { if (process == base::kNullProcessHandle) return 0; // This returns 0 if we have insufficient rights to query the process handle. // Invalid handles or non-process handles will cause a hard failure. ProcessId result = GetProcessId(process); CHECK(result != 0 || GetLastError() != ERROR_INVALID_HANDLE) << "process handle = " << process; return result; } Previously GetProcId would silently ignore all failures but this change makes it treat invalid handles as a fatal error. Unfortunately this has revealed several bugs - race conditions of various sorts. The individual fixes should be landed separately from adding the checks above in order to avoid having to revert everything. ⛆ |
|
|
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02f07de804a9c247c5b6405f0d7dde88da6bee3c commit 02f07de804a9c247c5b6405f0d7dde88da6bee3c Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Jul 31 17:28:26 2018 Reland "Fix invalid handle errors found with App Verifier" This is a reland of c31de3260f48a6c197d698f8f2e295cd9f04d49b. The CHECK that was triggering (crbug.com/868341) has been removed but the change is otherwise the same. The CHECK will be readded once all of the bugs that it reveals are fixed (crbug.com/869154) Original change's description: > Fix invalid handle errors found with App Verifier > > ChildProcessData was holding a raw ProcessHandle which meant that there > was no guarantee that the handle would be valid when it was used. This > change switches to a ScopedHandle on Windows, with the associated > duplication of the handle, so that it is guaranteed to be valid. This > fixes a frequently encountered use of an invalid handle that was > detected by Application Verifier. This change also adds a CHECK to > base::GetProcId to check for use of invalid handles even when not using > Application Verifier. > > ChildProcessData::handle is now a private member so all references to > it were updated to use accessors. > > When ChildProcessData::SetHandle is called the handle is duplicated. > When a ChildProcessData object needs to be copied an explicit > Duplicate function is called. > > An IsHandleValid helper function was added. > > An invalid test was removed because it was triggering the CHECK in > base::GetProcId. > > Bug: 417532,821453, 865805 > Change-Id: I7e38140335c0140536919341f011f144f150c88f > Reviewed-on: https://chromium-review.googlesource.com/1145767 > Commit-Queue: Bruce Dawson <brucedawson@chromium.org> > Reviewed-by: Jochen Eisinger <jochen@chromium.org> > Cr-Commit-Position: refs/heads/master@{#578419} Bug: 417532, 821453, 865805 , 868341, 869154 Change-Id: I022982f447cce4649618f4413e960f5639ad750a Reviewed-on: https://chromium-review.googlesource.com/1155498 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#579454} [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/android/compositor/compositor_view.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/android/feedback/process_id_feedback_source.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chrome_content_browser_client_browsertest_chromeos.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chrome_plugin_browsertest.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/chromeos/app_mode/app_session.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/extensions/api/processes/processes_api.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/image_decoder_browsertest.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/memory_details.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/performance_monitor/performance_monitor.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/resource_coordinator/browser_child_process_watcher.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_provider.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_provider.h [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/child_process_task_unittest.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/task_manager/providers/render_process_host_task_provider.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/ui/hung_plugin_tab_helper.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/chrome/browser/ui/webui/memory_internals_ui.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/browser_watcher/exit_code_watcher_win_unittest.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/crash/content/browser/child_exit_observer_android.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/heap_profiling/client_connection_manager.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/nacl/browser/nacl_broker_host_win.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/components/nacl/browser/nacl_process_host.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/browser_child_process_host_impl.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/histogram_controller.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/ppapi_plugin_process_host.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/renderer_host/media/audio_service_listener.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/renderer_host/media/audio_service_listener_unittest.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/browser/service_manager/service_manager_context.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/BUILD.gn [add] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/child_process_data.cc [modify] https://crrev.com/02f07de804a9c247c5b6405f0d7dde88da6bee3c/content/public/browser/child_process_data.h
,
Sep 24
CCing two interested parties. Work needed for this bug is: Step 1: Move ScopedProcessHandle to base Step 2: Use it as demonstrated in https://chromium-review.googlesource.com/c/chromium/src/+/1153577 (currently violates checkdeps rules due to ScopedProcessHandle not being in base) Step 3: Fix the blocked-on bugs that are still open. Step 4: Use code inspection to find other unwrapped process handles or places where #ifdefs can be avoided by using ScopedProcessHandle (i.e.; uses of base::ScopedHandle for processes in portable code) Step 5: Fix whatever other bugs pop up (by doing step N locally and testing) ... Step N: Add the CHECK back to GetProcId - see the opening comment in this bug
,
Sep 27
,
Jan 10
Reassigning. Details of the plan are in comment#3.
,
Jan 10
See also bug 412104 (stop passing SharedMemoryHandle as raw handles) which should probably be investigated and fixed as part of this, along with bug 822984 (DCHECK(result) in ProcessMetrics::ProcessMetrics). Both are marked as blockers for this bug. |
|||||
►
Sign in to add a comment |
||||||||
Comment 1 by brucedaw...@chromium.org
, Jul 30Status: Assigned (was: Untriaged)