Mojo has a race condition that causes it to duplicate an invalid handle from an invalid process |
||||||
Issue descriptionWhen testing Chrome with App Verifier (requires M68 or later) with modifications to tell HandleCloserAgent::CloseHandles() not to close handles when dropping the token I frequently see invalid handle exceptions in mojo. To reproduce this it may be necessary to apply the patch mentioned in this comment: https://bugs.chromium.org/p/chromium/issues/detail?id=836315#c8 Then run 64-bit chrome with App Verifier enabled and with leaks and locks checks disabled, but with handles checks enabled. After moving between sites a few times I eventually hit this error: (2d3c.3da0): Invalid handle - code c0000008 (first chance) First chance exceptions are reported before any exception handling. This exception may be expected and handled. ntdll!KiRaiseUserExceptionDispatcher+0x3a: 00007ffc`6d6c917a 8b8424c0000000 mov eax,dword ptr [rsp+0C0h] ss:000000bb`e4dff1d0=c0000008 *** WARNING: Unable to verify checksum for c:\src\chromium\src\out\release_64_component\mojo_edk.dll *** WARNING: Unable to verify checksum for c:\src\chromium\src\out\release_64_component\base.dll *** WARNING: Unable to verify checksum for c:\src\chromium\src\out\release_64_component\content.dll 0:009> kL # Child-SP RetAddr Call Site 00 000000bb`e4dff110 00007ffc`5b021952 ntdll!KiRaiseUserExceptionDispatcher+0x3a 01 000000bb`e4dff1e0 00007ffc`6a818efb vfbasics!AVrfpNtDuplicateObject+0x72 02 000000bb`e4dff240 00007ffc`35fa4513 KERNELBASE!DuplicateHandle+0x4b 03 000000bb`e4dff290 00007ffc`35fa5c95 mojo_edk!mojo::edk::PlatformHandle::CloseIfNecessary+0x43 04 000000bb`e4dff2e0 00007ffc`35fa876d mojo_edk!std::vector<mojo::edk::ScopedPlatformHandle,std::allocator<mojo::edk::ScopedPlatformHandle> >::_Tidy+0x27 05 000000bb`e4dff320 00007ffc`35fa8a23 mojo_edk!std::unique_ptr<mojo::edk::Channel::Message,std::default_delete<mojo::edk::Channel::Message> >::~unique_ptr+0x15 06 000000bb`e4dff350 00007ffc`35fa7e97 mojo_edk!base::circular_deque<std::unique_ptr<mojo::edk::Channel::Message,std::default_delete<mojo::edk::Channel::Message> > >::DestructRange+0x2f 07 000000bb`e4dff3a0 00007ffc`35fa84bb mojo_edk!mojo::edk::`anonymous namespace'::ChannelWin::~ChannelWin+0x51 08 000000bb`e4dff3e0 00007ffc`2a23221d mojo_edk!base::internal::BindState<void (mojo::edk::(anonymous namespace)::ChannelWin::*)(),scoped_refptr<mojo::edk::(anonymous namespace)::ChannelWin> >::Destroy+0x3b 09 000000bb`e4dff420 00007ffc`2a268dcc base!base::debug::TaskAnnotator::RunTask+0x13d 0a 000000bb`e4dff540 00007ffc`2a2694e8 base!base::MessageLoop::RunTask+0x23c 0b 000000bb`e4dff6a0 00007ffc`2a26b5ea base!base::MessageLoop::DoWork+0x198 0c 000000bb`e4dff890 00007ffc`2a26a7c8 base!base::MessagePumpForIO::DoRunLoop+0x14a 0d 000000bb`e4dff930 00007ffc`2a2a4da1 base!base::MessagePumpWin::Run+0x68 0e 000000bb`e4dff990 00007ffc`0cf15f79 base!base::RunLoop::Run+0x31 0f 000000bb`e4dff9c0 00007ffc`2a2e925a content!content::BrowserProcessSubThread::IOThreadRun+0x25 10 000000bb`e4dffa00 00007ffc`2a2e60a4 base!base::Thread::ThreadMain+0x18a 11 000000bb`e4dffa90 00007ffc`5b01ed34 base!base::`anonymous namespace'::ThreadFunc+0xf4 12 000000bb`e4dffb10 00007ffc`6c2a2784 vfbasics!AVrfpStandardThreadFunction+0x44 13 000000bb`e4dffb50 00007ffc`6d690d51 KERNEL32!BaseThreadInitThunk+0x14 14 000000bb`e4dffb80 00000000`00000000 ntdll!RtlUserThreadStart+0x21 The code in question is here - notice that it acknowledges the possibility of it being an invalid handle. This feels dodgy - shouldn't we be duping the handle while we know it is guaranteed to be valid? if (owning_process != base::GetCurrentProcessHandle()) { // This handle may have been duplicated to a new target process but not yet // sent there. In this case CloseHandle should NOT be called. From MSDN // documentation for DuplicateHandle[1]: // // Normally the target process closes a duplicated handle when that // process is finished using the handle. To close a duplicated handle // from the source process, call DuplicateHandle with the following // parameters: // // * Set hSourceProcessHandle to the target process from the // call that created the handle. // * Set hSourceHandle to the duplicated handle to close. // * Set lpTargetHandle to NULL. // * Set dwOptions to DUPLICATE_CLOSE_SOURCE. // // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251 // // NOTE: It's possible for this operation to fail if the owning process // was terminated or is in the process of being terminated. Either way, // there is nothing we can reasonably do about failure, so we ignore it. DuplicateHandle(owning_process, handle, NULL, &handle, 0, FALSE, DUPLICATE_CLOSE_SOURCE); return; } It looks like both handles (owning_process and handle) are invalid. Debugging it a bit more I verified that both handles are invalid: 0:013> dt this Local var @ rsi Type mojo::edk::PlatformHandle* +0x000 handle : 0x00000000`0000167c Void +0x008 owning_process : 0x00000000`00001308 Void +0x010 needs_connection : 0 0:013> !handle 0x167c Could not duplicate handle 167c, error 6 0:013> !handle 0x1308 Could not duplicate handle 1308, error 6 0:013> !handle 0x1fac Handle 1fac Type Process I ran !handle on a randomly chosen good handle just to demonstrate the expected results. I had to modify the code slightly to preserve this->handle in the call since otherwise it gets zeroed before the exception is thrown. This doesn't change the behavior, just the debuggability. This feels like a race condition. When the handles are invalid the race condition is probably benign, but occasionally handle reuse may mean that the wrong handles get duplicated, maybe? I'm having trouble reasoning about the exact possibilities but if it leads to the wrong handle being closed it would be very bad. I'm not clear on the actual risks at the moment, and this is not a new issue, so setting to low priority for now.
,
Apr 27 2018
I'm not sure where the inherent raciness is coming from - usually it is possible to avoid the raciness entirely by making sure that everyone who needs ownership of the handle has their own copy so that it can't be pulled out from underneath them, similar to shared_ptr.
I'm not sure what mojo is actually doing with the handles so I can't suggest changes so instead I've got this pseudo-code which represents racy code I found at a previous job:
HANDLE hThread;
void myFunction() {
CloseHandle(
}
void ThreadyStuff() {
hThread = CreateThread(myFunction);
WaitForSingleObject(hThread);
}
Because myFunction closes the handle there is a race condition on the WaitForSingleObject(hThread) in the main thread. It's best avoided by having ThreadyStuff close the handle when it's finished instead of myFunction.
In the code above ThreadyStuff is performing operations on a handle it doesn't own, and the solution is to get it to own the handle. Can we apply the same thing to mojo?
Is there actually a risk of handle leaks? Handle leaks are certainly bad (I've ranted about them a lot lately, process handle leaks are particularly bad) but if the handles are no longer valid then that suggests that they are being cleaned up by the OS.
Apologies if I'm making dumb suggestions - I don't understand the data flow of the code well enough to say anything more concrete.
,
Apr 27 2018
Mojo isn't doing anything too crazy. It duplicates handles in a message (with DUPLICATE_CLOSE_SOURCE) to the destination process shortly before writing the message to a named pipe. If that message fails to be written to the pipe while the destination process is still alive, we are left owning handle values that actually belong to the destination process. We will imminently destroy the undelivered message object, and in this case it is correct to ensure that we close the duplicated handles on behalf of the destination process who knows nothing about them. Hence the special DuplicateHandle call. The raciness bug here is that said destination process may die some time between the write failure and the message destruction. In *practice* outside of testing environments it is likely true that a failure to send an IPC message will result in the imminent death of one or the other process involved, so leaks may not be a serious issue. I would however like to consider how we can address this kind of behavior without relying on such assumptions.
,
Apr 27 2018
Actually I'm not sure there is any reasonable alternative. There is effectively no foolproof way we can guarantee that a process is informed about handles which have been duplicated to it, as any available communication medium may fail. The failed WriteFile attempt in this case is about as best-effort as we can get. Given the inherent raciness of the sender attempting to close the handles on the would-be receiver's behalf , I think we must tolerate "leaks" under the assumption that owning process will die very shortly thereafter and the handles will therefore be closed by the OS.
,
Apr 27 2018
I agree. If I'm understanding correctly then the handles are "owned" by the target process from the moment that they are created, from the perspective of the OS. That means that any operations we perform with them (other than copying them around as a bag-of-bits) are racy. I assume that the child process is useless to us if the sending of the handles' message fails? If so then I think that the best we can do as far as avoiding leaks is to make sure that the child process is killed. If this reliably happens already then we don't have a leak. If there are some edge cases then TerminateProcess may be the correct solution.
,
Apr 27 2018
All sounds correct to me. I'm going to remove this call to DuplicateHandle and we'll see what happens. In production code we do always terminate child processes when their connection is lost. I may expect to see leaks appear in some mojo multiprocess tests, but we'll see what happens.
,
Apr 28 2018
It looks like some mojo tests are failing with the attempted CL, but I couldn't see why. As predicted? One more question: The handles are owned by the child process and will be closed by the child process. With that in mind it is not clear to me that it is even *possible* for us to close the handles. Having "given them away" we would have to take them back from the child process, somehow, in order to close them. Is that actually possible? I have no familiarity with these aspects of Win32.
,
Apr 30 2018
First, for your question, yes the handles are owned by the child process, but the child process is not aware of that fact unless it receives a message conveying as much. Mojo is trying to support that case without letting the handle leak. It is indeed possible for a process to close a handle it doesn't own, and this is documented in the comment being deleted by this CL. DuplicateHandle serves exactly that purpose when called this way (see also the MSDN link in the comment). Thinking about this some more with a fresh Monday brain though, I think this code is correct after all. What makes it racy is that the process handle in |owning_process| may not be valid (or rather, may refer to some other, newer random object) by the time the PlatformHandle is closed. The solution to that problem is to ensure that the PlatformHandle actually *owns* a handle to the target process rather than simply holding a copy of a handle someone else owns. That should alleviate the raciness. Even if the target process is died by this point, an open handle to it will still be "valid" in the sense that it will refer to the dead process object rather than potentially some other random object in the system. As for the test failing, it's essentially because we're no longer closing a handle when we should be, and the test will hang until that handle is closed.
,
May 1 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/27c9967014302ecc77c888a8628773f53a830e5c commit 27c9967014302ecc77c888a8628773f53a830e5c Author: Ken Rockot <rockot@chromium.org> Date: Tue May 01 18:28:13 2018 Mojo EDK: Improve internal process handle ownership Mojo passes around base::ProcessHandle values for various reasons. On most systems this is fine, but at least on Windows, a ProcessHandle refers to an owned reference to a system process object, and if not careful it's possible for a base::ProcessHandle value to inadvertently change meaning over time. This CL introduces the concept of a move-only ScopedProcessHandle within Mojo, which on most platforms is just a base::PlatformHandle. On Windows, this represents an owned base::ProcessHandle which closes on destruction and clones correctly using DuplicateHandle rather than merely copying the raw handle value. ScopedProcessHandle is used in a few places where process handle ownership semantics were previously weaker than necessary, or were correct but implemented ad hoc. This also updates ScopedPlatformHandle (and supporting code like Channel::RewriteHandles) such that the |owning_process| field (if not the current process) is always an owned process handle. This ensures that when such handles are closed in unsent messages, they can be safely closed in the target process (from within the source process) without any risk of raciness against target process termination. Bug: 837612 Change-Id: I943bb5f70ede56351d52b2ecea7d76fcfdee46ce Reviewed-on: https://chromium-review.googlesource.com/1036459 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#555117} [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/BUILD.gn [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/embedder/platform_handle.cc [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/embedder/platform_handle.h [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/broker_host.cc [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/broker_host.h [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/channel.cc [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/node_channel.cc [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/node_channel.h [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/node_controller.cc [modify] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/node_controller.h [add] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/scoped_process_handle.cc [add] https://crrev.com/27c9967014302ecc77c888a8628773f53a830e5c/mojo/edk/system/scoped_process_handle.h
,
May 1 2018
,
May 2 2018
Tested with today's 64-bit canary (68.0.3417.0) with Handles checks enabled.
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859 commit 2f8cd83e52bbab72c8c1ff79308fd7f6b9647859 Author: Ken Rockot <rockot@chromium.org> Date: Mon May 14 18:46:22 2018 Revert "Mojo EDK: Improve internal process handle ownership" This reverts commit 27c9967014302ecc77c888a8628773f53a830e5c. Reason for revert: https://crbug.com/841565 Original change's description: > Mojo EDK: Improve internal process handle ownership > > Mojo passes around base::ProcessHandle values for various reasons. On > most systems this is fine, but at least on Windows, a ProcessHandle > refers to an owned reference to a system process object, and if not > careful it's possible for a base::ProcessHandle value to inadvertently > change meaning over time. > > This CL introduces the concept of a move-only ScopedProcessHandle > within Mojo, which on most platforms is just a base::PlatformHandle. > On Windows, this represents an owned base::ProcessHandle which closes > on destruction and clones correctly using DuplicateHandle rather than > merely copying the raw handle value. > > ScopedProcessHandle is used in a few places where process handle > ownership semantics were previously weaker than necessary, or were > correct but implemented ad hoc. > > This also updates ScopedPlatformHandle (and supporting code like > Channel::RewriteHandles) such that the |owning_process| field (if not > the current process) is always an owned process handle. This ensures > that when such handles are closed in unsent messages, they can be > safely closed in the target process (from within the source process) > without any risk of raciness against target process termination. > > Bug: 837612 > Change-Id: I943bb5f70ede56351d52b2ecea7d76fcfdee46ce > Reviewed-on: https://chromium-review.googlesource.com/1036459 > Reviewed-by: Jay Civelli <jcivelli@chromium.org> > Commit-Queue: Ken Rockot <rockot@chromium.org> > Cr-Commit-Position: refs/heads/master@{#555117} TBR=jcivelli@chromium.org,rockot@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 837612 Change-Id: Ief6e1d6d6f2f96dc7420e06d8438cc06cbf17490 Reviewed-on: https://chromium-review.googlesource.com/1057699 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#558388} [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/BUILD.gn [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/embedder/platform_handle.cc [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/broker_host.cc [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/broker_host.h [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/channel.cc [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/node_channel.cc [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/node_channel.h [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/node_controller.cc [modify] https://crrev.com/2f8cd83e52bbab72c8c1ff79308fd7f6b9647859/mojo/edk/system/node_controller.h [delete] https://crrev.com/7ca50812c30a33e4961968018170650a6f05c484/mojo/edk/system/scoped_process_handle.cc [delete] https://crrev.com/7ca50812c30a33e4961968018170650a6f05c484/mojo/edk/system/scoped_process_handle.h
,
May 14 2018
Re-opening until I can land a fix that doesn't leak.
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac commit a9ba958ea40196b4de231fc1dffd8c6bacdb83ac Author: Ken Rockot <rockot@chromium.org> Date: Fri May 18 02:16:29 2018 Reland: Mojo EDK: Improve internal process handle ownership Reland of r555117 with process handle leak fixed. Original commit message follows. Mojo passes around base::ProcessHandle values for various reasons. On most systems this is fine, but at least on Windows, a ProcessHandle refers to an owned reference to a system process object, and if not careful it's possible for a base::ProcessHandle value to inadvertently change meaning over time. This CL introduces the concept of a move-only ScopedProcessHandle within Mojo, which on most platforms is just a base::PlatformHandle. On Windows, this represents an owned base::ProcessHandle which closes on destruction and clones correctly using DuplicateHandle rather than merely copying the raw handle value. ScopedProcessHandle is used in a few places where process handle ownership semantics were previously weaker than necessary, or were correct but implemented ad hoc. This also updates ScopedPlatformHandle (and supporting code like Channel::RewriteHandles) such that the |owning_process| field (if not the current process) is always an owned process handle. This ensures that when such handles are closed in unsent messages, they can be safely closed in the target process (from within the source process) without any risk of raciness against target process termination. TBR=jcivelli@chromium.org Bug: 837612 Change-Id: I28aaa04ca09f483e7e6f073db2edb762893a8b17 Reviewed-on: https://chromium-review.googlesource.com/1065129 Commit-Queue: Ken Rockot <rockot@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#559777} [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/BUILD.gn [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/embedder/platform_handle.cc [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/broker_host.cc [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/broker_host.h [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/channel.cc [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/node_channel.cc [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/node_channel.h [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/node_controller.cc [modify] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/node_controller.h [add] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/scoped_process_handle.cc [add] https://crrev.com/a9ba958ea40196b4de231fc1dffd8c6bacdb83ac/mojo/edk/system/scoped_process_handle.h
,
May 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ab73dd4f03b62cbe61e83482356506e02cd1bf4 commit 6ab73dd4f03b62cbe61e83482356506e02cd1bf4 Author: Ken Rockot <rockot@chromium.org> Date: Sat May 19 20:52:58 2018 Revert "Reland: Mojo EDK: Improve internal process handle ownership" This reverts commit a9ba958ea40196b4de231fc1dffd8c6bacdb83ac. Reason for revert: Seems there's still a leak here somewhere. Original change's description: > Reland: Mojo EDK: Improve internal process handle ownership > > Reland of r555117 with process handle leak fixed. Original commit > message follows. > > Mojo passes around base::ProcessHandle values for various reasons. On > most systems this is fine, but at least on Windows, a ProcessHandle > refers to an owned reference to a system process object, and if not > careful it's possible for a base::ProcessHandle value to inadvertently > change meaning over time. > > This CL introduces the concept of a move-only ScopedProcessHandle > within Mojo, which on most platforms is just a base::PlatformHandle. > On Windows, this represents an owned base::ProcessHandle which closes > on destruction and clones correctly using DuplicateHandle rather than > merely copying the raw handle value. > > ScopedProcessHandle is used in a few places where process handle > ownership semantics were previously weaker than necessary, or were > correct but implemented ad hoc. > > This also updates ScopedPlatformHandle (and supporting code like > Channel::RewriteHandles) such that the |owning_process| field (if not > the current process) is always an owned process handle. This ensures > that when such handles are closed in unsent messages, they can be > safely closed in the target process (from within the source process) > without any risk of raciness against target process termination. > > TBR=jcivelli@chromium.org > > Bug: 837612 > Change-Id: I28aaa04ca09f483e7e6f073db2edb762893a8b17 > Reviewed-on: https://chromium-review.googlesource.com/1065129 > Commit-Queue: Ken Rockot <rockot@chromium.org> > Reviewed-by: Ken Rockot <rockot@chromium.org> > Cr-Commit-Position: refs/heads/master@{#559777} TBR=jcivelli@chromium.org,rockot@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 837612 Change-Id: Id0e326686cecd305f315b4a67bbda5e9f7c52b34 Reviewed-on: https://chromium-review.googlesource.com/1067051 Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#560157} [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/BUILD.gn [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/embedder/platform_handle.cc [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/broker_host.cc [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/broker_host.h [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/channel.cc [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/node_channel.cc [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/node_channel.h [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/node_controller.cc [modify] https://crrev.com/6ab73dd4f03b62cbe61e83482356506e02cd1bf4/mojo/edk/system/node_controller.h [delete] https://crrev.com/e3cc1035c519882634e4736506c8b179ccda8393/mojo/edk/system/scoped_process_handle.cc [delete] https://crrev.com/e3cc1035c519882634e4736506c8b179ccda8393/mojo/edk/system/scoped_process_handle.h
,
May 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/21070080327d78d72e881a2f1c7d030ccae8b256 commit 21070080327d78d72e881a2f1c7d030ccae8b256 Author: Ken Rockot <rockot@chromium.org> Date: Sun May 20 23:00:44 2018 Reland: Mojo EDK: Improve internal process handle ownership Re-reland of r555117 with process handle leak actually fixed. Totes for reals. Original commit message follows. Mojo passes around base::ProcessHandle values for various reasons. On most systems this is fine, but at least on Windows, a ProcessHandle refers to an owned reference to a system process object, and if not careful it's possible for a base::ProcessHandle value to inadvertently change meaning over time. This CL introduces the concept of a move-only ScopedProcessHandle within Mojo, which on most platforms is just a base::PlatformHandle. On Windows, this represents an owned base::ProcessHandle which closes on destruction and clones correctly using DuplicateHandle rather than merely copying the raw handle value. ScopedProcessHandle is used in a few places where process handle ownership semantics were previously weaker than necessary, or were correct but implemented ad hoc. This also updates ScopedPlatformHandle (and supporting code like Channel::RewriteHandles) such that the |owning_process| field (if not the current process) is always an owned process handle. This ensures that when such handles are closed in unsent messages, they can be safely closed in the target process (from within the source process) without any risk of raciness against target process termination. TBR=jcivelli@chromium.org Bug: 837612 Change-Id: I182e6849fd87fb44e22f6ed21457b17422956613 Reviewed-on: https://chromium-review.googlesource.com/1066999 Commit-Queue: Ken Rockot <rockot@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Cr-Commit-Position: refs/heads/master@{#560206} [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/BUILD.gn [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/embedder/platform_handle.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/broker_host.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/broker_host.h [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/channel.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/channel_win.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/node_channel.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/node_channel.h [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/node_controller.cc [modify] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/node_controller.h [add] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/scoped_process_handle.cc [add] https://crrev.com/21070080327d78d72e881a2f1c7d030ccae8b256/mojo/edk/system/scoped_process_handle.h
,
May 20 2018
Landed and no handle leaks this time. Woot |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by roc...@chromium.org
, Apr 27 2018