Cross-process handle ownership management is racy on Windows |
||
Issue descriptionTL;DR: When a process sends a handle to the broker, it retains ownership of that handle until ChannelWin::WriteDone is invoked by the I/O subsystem. Meanwhile, the broker process, upon receiving a HANDLE value from another process, immediately calls DuplicateHandle on the HANDLE to rewrite it into its own handle table. DUPLICATE_CLOSE_SOURCE is used to close it in the source process. The call to DuplicateHandle in the broker process can race with the call to ChannelWin::WriteDone in the sending process, resulting in the sending process having one of its handles closed while still tracked by the handle verifier.
,
Nov 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6dc0899060e1b654af0b097b39f6c8c64f960ca0 commit 6dc0899060e1b654af0b097b39f6c8c64f960ca0 Author: Ken Rockot <rockot@google.com> Date: Thu Nov 01 03:32:56 2018 [mojo] Release sent handles earlier on Windows This releases HANDLE ownership *before* sending any handle in a message to a remote process, rather than after. Avoids an extremely subtle race between DuplicateHandle+DUPLICATE_CLOSE_SOURCE in the receiving process racing with ScopedHandle release in the sending process. The trade-off is the introduction of potential leaks which turn out to not really matter in any practical scenarios. Bug: 900655 Change-Id: Ieec82bb062bc66cdb3c732c920a220ea2aa2f8d0 Reviewed-on: https://chromium-review.googlesource.com/c/1310505 Commit-Queue: Ken Rockot <rockot@google.com> Reviewed-by: Joe Mason <joenotcharles@chromium.org> Cr-Commit-Position: refs/heads/master@{#604510} [modify] https://crrev.com/6dc0899060e1b654af0b097b39f6c8c64f960ca0/mojo/core/channel.cc [modify] https://crrev.com/6dc0899060e1b654af0b097b39f6c8c64f960ca0/mojo/core/channel_win.cc
,
Nov 1
Should be fixed! Let's continue any further discussion on this bug if we need to re-open. Thanks for your diligence investigating this.
,
Nov 1
When I apply this patch to the internal version of the chrome cleanup tool, I'm getting a failure in MojoSandboxHooksTest.SpawnSandboxTarget. Still trying to diagnose it. Since the open-source version of that test is working (in chrome/chrome_cleaner/ipc/mojo_sandbox_hooks_unittest.cc) there must be some divergence.
,
Nov 1
Nevermind. Missed the change to channel.cc when cherry-picking somehow.
,
Nov 1
Ah, cool. I was kind of wondering about that. The change to channel.cc was added after the initial upload.
,
Nov 5
|
||
►
Sign in to add a comment |
||
Comment 1 by rockot@google.com
, Oct 31