The code in question won't attempt to close an invalid handle; it will only close a valid handle matching the appropriate type and name pattern. What's blowing up here is the call to QueryObjectTypeInformation(). We should be able to just bracket that and the following while loop in an SEH, then set rc to the exception code. That will cause the error check to increment invalid_count and continue the scanning loop.

I apologize for the ugliness of this code, but there's no API to get an accurate snapshot of the handle table. We tried using NtQuerySystemInformation() with SystemHandleInformation, but it often returns a stale snapshot, and it runs much slower than just manually scanning the handle space.

yes I think wrapping this in SEH seems like a reasonable approach.

Wrapping QueryObjectTypeInformation with SEH is such a good idea that we did it years ago. In fact, QueryObjectTypeInformation is just a wrapper (with SEH) to catch these bad-handle exceptions.

Initially this SEH wasn't working for me because I had windbg configured to stop on first-chance exceptions of this type. After changing that to "ignore" I hit the next problem which is that App Verifier translates the bad-handle exceptions into breakpoints.

So...

Unless there is some way that we can avoid querying these invalid handles (and it sounds like this has been explored sufficiently) then I don't think there is anything to do. We should be getting sufficient coverage of bad-handles from the normal behavior when debugging and from our own checks so this shouldn't be a problem. I'll just document that Leaks and Handles have to be disabled when using App Verifier with Chrome.

I think I'll close this as WontFix.

I have one last thought. If there's an easy way to test for the presence of the app verifier, we could support a command-line flag that potentially just doesn't run the handle closer in that case.

Yes, we can just look for one of the appverifier DLLs to be loaded. That would be handy - reducing the number of steps needed to use App Verifier - as long as it doesn't compromise security. We'll always need to disable the Leaks check.

Well, it weakens security a bit, but it's fine if it's scoped to when the app verifier is running.

Further thought on the mechanics: Maybe add a switch like --app-verifier and when present do a GetModuleHandle() check at startup for the DLLs, to determine if we should disable the handle closer?

TL;DR getting past the sandbox checks reveals another invalid-handle exception. If that is worth fixing then we should proceed along this path and keep our code invalid-handle clean when AppVerifier is loaded. If that other code is fine or unavoidable then we should probably stop going down this path.

Handle race conditions do worry me - they could be causing any number of random "impossible" bugs.



Details:

> it will only close a valid handle matching the appropriate type and name pattern.

Isn't there a TOCTOU risk here? Just wondering.

I think that requiring an --app-verifier switch makes the whole thing too clumsy.



I tried adding a GetModuleHandle check with this diff:

diff --git a/sandbox/win/src/handle_closer_agent.cc b/sandbox/win/src/handle_closer_agent.cc
index ddb6791391b8..5a91155c063a 100644
--- a/sandbox/win/src/handle_closer_agent.cc
+++ b/sandbox/win/src/handle_closer_agent.cc
@@ -134,6 +134,11 @@ bool HandleCloserAgent::CloseHandles() {
   if (!::GetProcessHandleCount(::GetCurrentProcess(), &handle_count))
     return false;

+  // Skip closing these handles when Application Verifier is in use in order to
+  // avoid invalid-handle exceptions.
+  if (GetModuleHandleW(L"vrfcore.dll"))
+    return true;
+
   // Set up buffers for the type info and the name.
   std::vector<BYTE> type_info_buffer(sizeof(OBJECT_TYPE_INFORMATION) +
                                      32 * sizeof(wchar_t));


This gets past the sandbox initialization but after a bit of browsing it failed elsewhere.

(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;
  }

Yeah, that looks like a serious problem, particularly if we're hitting it with app verifier under normal usage. Please file a bug as a blocker against this one and assign to Ken Rockot.

Re #8: I'm not sure what you mean about the possibility of it being an invalid handle. In this case the handle value is valid in owning_process's handle table despite owning_process not being aware yet that it owns the handle. This is a way to close the handle on owning_process's behalf in that case.

> Re #8: I'm not sure what you mean about the possibility of it being an invalid handle.

It's not a possibility, it's a reality. I don't think App Verifier has false positives regarding handle validity. I don't know what the severity is but in general the risk with performing operations on invalid handles is that the handle values may have been reused, which means a risk of performing operations on the wrong object.

I filed  bug 837612 .

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

commit 90b870f65099f0fb504d3fb8f610a54e95edc434
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Tue May 01 17:50:15 2018

Disable closing of handles when AppVerifier is running

The sandbox normally closes open handles when lowering the token,
however this often includes querying object type information from
invalid handles. When the Handles checker is enabled this triggers
exceptions which then make App Verifier harder to use. Since the Handles
checker is quite useful it is worth disabling the closing of handles
when App Verifier is loaded. This is detected by looking for
vrfcore.dll.

Bug:  836315 
Change-Id: I2c794caf5e9b1c719376aabe780c7dcde7be56cd
Reviewed-on: https://chromium-review.googlesource.com/1032783
Reviewed-by: Justin Schuh <jschuh@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555104}
[modify] https://crrev.com/90b870f65099f0fb504d3fb8f610a54e95edc434/sandbox/win/src/handle_closer_agent.cc

Tested with today's 64-bit canary (68.0.3417.0) with Handles checks enabled.