Replace handles that the handle closer closes with dummy Events.

This is because on Windows, we close some handles opened by Windows DLLs to prevent them from leaking into sandboxed processes, and this can cause INVALID_HANDLE_EXCEPTION to be raised if "bad handle detection" gflag is enabled, as it currently is by default on Windows 10.

Only replace File and Event handles at the moment, as File handles are the ones causing the issues on Windows 10.

BUG=452613
TEST=sbox_integration_tests
TEST=Run Chrome on Windows 10 with crash reporting enabled, and check there are no crashes listed in chrome://crashes

Review URL: https://codereview.chromium.org/919893002

Cr-Commit-Position: refs/heads/master@{#317016}

sandbox/win/src/handle_closer_agent.cc

@@ -41,6 +41,50 @@ bool HandleCloserAgent::NeedsHandlesClosed() {
   return g_handles_to_close != NULL;
 }
 
+HandleCloserAgent::HandleCloserAgent()
+    : dummy_handle_(::CreateEvent(NULL, FALSE, FALSE, NULL)) {
+}
+
+// Attempts to stuff |closed_handle| with a duplicated handle for a dummy Event
+// with no access. This should allow the handle to be closed, to avoid
+// generating EXCEPTION_INVALID_HANDLE on shutdown, but nothing else. For now
+// the only supported |type| is Event or File.
+bool HandleCloserAgent::AttemptToStuffHandleSlot(HANDLE closed_handle,
+                                                 const base::string16& type) {
+  // Only attempt to stuff Files and Events at the moment.
+  if (type != L"Event" && type != L"File") {
+    return true;
+  }
+
+  if (!dummy_handle_.IsValid())
+    return false;
+
+  // This should never happen, as g_dummy is created before closing to_stuff.
+  DCHECK(dummy_handle_.Get() != closed_handle);
+
+  std::vector<HANDLE> to_close;
+  HANDLE dup_dummy = NULL;
+  size_t count = 16;
+
+  do {
+    if (!::DuplicateHandle(::GetCurrentProcess(), dummy_handle_.Get(),
+                           ::GetCurrentProcess(), &dup_dummy, 0, FALSE, 0))
+      break;
+    if (dup_dummy != closed_handle)
+      to_close.push_back(dup_dummy);
+  } while (count-- &&
+           reinterpret_cast<uintptr_t>(dup_dummy) <
+               reinterpret_cast<uintptr_t>(closed_handle));
+
+  for (auto h : to_close)
+    ::CloseHandle(h);
+
+  // Useful to know when we're not able to stuff handles.
+  DCHECK(dup_dummy == closed_handle);
+
+  return dup_dummy == closed_handle;
+}
+
 // Reads g_handles_to_close and creates the lookup map.
 void HandleCloserAgent::InitializeHandlesToClose() {
   CHECK(g_handles_to_close != NULL);
@@ -136,6 +180,8 @@ bool HandleCloserAgent::CloseHandles() {
         return false;
       if (!::CloseHandle(handle))
         return false;
+      // Attempt to stuff this handle with a new dummy Event.
+      AttemptToStuffHandleSlot(handle, result->first);
     }
   }
 

sandbox/win/src/handle_closer_agent.h

@@ -7,6 +7,7 @@
 
 #include "base/basictypes.h"
 #include "base/strings/string16.h"
+#include "base/win/scoped_handle.h"
 #include "sandbox/win/src/handle_closer.h"
 #include "sandbox/win/src/sandbox_types.h"
 
@@ -15,7 +16,7 @@ namespace sandbox {
 // Target process code to close the handle list copied over from the broker.
 class HandleCloserAgent {
  public:
-  HandleCloserAgent() {}
+  HandleCloserAgent();
 
   // Reads the serialized list from the broker and creates the lookup map.
   void InitializeHandlesToClose();
@@ -23,11 +24,16 @@ class HandleCloserAgent {
   // Closes any handles matching those in the lookup map.
   bool CloseHandles();
 
-  // True if we have handles waiting to be closed
+  // True if we have handles waiting to be closed.
   static bool NeedsHandlesClosed();
 
  private:
+  // Attempt to stuff a closed handle with a dummy Event.
+  bool AttemptToStuffHandleSlot(HANDLE closed_handle,
+                                const base::string16& type);
+
   HandleMap handles_to_close_;
+  base::win::ScopedHandle dummy_handle_;
 
   DISALLOW_COPY_AND_ASSIGN(HandleCloserAgent);
 };

sandbox/win/src/handle_closer_test.cc

@@ -5,6 +5,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/win/scoped_handle.h"
 #include "sandbox/win/src/handle_closer_agent.h"
+#include "sandbox/win/src/nt_internals.h"
 #include "sandbox/win/src/sandbox.h"
 #include "sandbox/win/src/sandbox_factory.h"
 #include "sandbox/win/src/target_services.h"
@@ -42,6 +43,26 @@ HANDLE GetMarkerFile(const wchar_t *extension) {
       NULL, OPEN_ALWAYS, FILE_FLAG_DELETE_ON_CLOSE, NULL);
 }
 
+// Returns type infomation for an NT object. This routine is expected to be
+// called for invalid handles so it catches STATUS_INVALID_HANDLE exceptions
+// that can be generated when handle tracing is enabled.
+NTSTATUS QueryObjectTypeInformation(HANDLE handle, void* buffer, ULONG* size) {
+  static NtQueryObject QueryObject = NULL;
+  if (!QueryObject)
+    ResolveNTFunctionPtr("NtQueryObject", &QueryObject);
+
+  NTSTATUS status = STATUS_UNSUCCESSFUL;
+  __try {
+    status = QueryObject(handle, ObjectTypeInformation, buffer, *size, size);
+  }
+  __except(GetExceptionCode() == STATUS_INVALID_HANDLE
+               ? EXCEPTION_EXECUTE_HANDLER
+               : EXCEPTION_CONTINUE_SEARCH) {
+    status = STATUS_INVALID_HANDLE;
+  }
+  return status;
+}
+
 // Used by the thread pool tests.
 HANDLE finish_event;
 const int kWaitCount = 20;
@@ -66,7 +87,7 @@ SBOX_TESTS_COMMAND int CheckForFileHandles(int argc, wchar_t **argv) {
       // Create a unique marker file that is open while the test is running.
       // The handles leak, but it will be closed by the test or on exit.
       for (int i = 0; i < arraysize(kFileExtensions); ++i)
-        EXPECT_NE(GetMarkerFile(kFileExtensions[i]), INVALID_HANDLE_VALUE);
+        CHECK_NE(GetMarkerFile(kFileExtensions[i]), INVALID_HANDLE_VALUE);
       return SBOX_TEST_SUCCEEDED;
 
     case AFTER_REVERT: {
@@ -104,6 +125,70 @@ SBOX_TESTS_COMMAND int CheckForFileHandles(int argc, wchar_t **argv) {
   return SBOX_TEST_SUCCEEDED;
 }
 
+// Checks that supplied handle is an Event and it's not waitable.
+// Format: CheckForEventHandles
+SBOX_TESTS_COMMAND int CheckForEventHandles(int argc, wchar_t** argv) {
+  static int state = BEFORE_INIT;
+  static std::vector<HANDLE> to_check;
+
+  switch (state++) {
+    case BEFORE_INIT:
+      // Create a unique marker file that is open while the test is running.
+      for (int i = 0; i < arraysize(kFileExtensions); ++i) {
+        HANDLE handle = GetMarkerFile(kFileExtensions[i]);
+        CHECK_NE(handle, INVALID_HANDLE_VALUE);
+        to_check.push_back(handle);
+      }
+      return SBOX_TEST_SUCCEEDED;
+
+    case AFTER_REVERT:
+      for (auto handle : to_check) {
+        // Set up buffers for the type info and the name.
+        std::vector<BYTE> type_info_buffer(sizeof(OBJECT_TYPE_INFORMATION) +
+                                           32 * sizeof(wchar_t));
+        OBJECT_TYPE_INFORMATION* type_info =
+            reinterpret_cast<OBJECT_TYPE_INFORMATION*>(&(type_info_buffer[0]));
+        NTSTATUS rc;
+
+        // Get the type name, reusing the buffer.
+        ULONG size = static_cast<ULONG>(type_info_buffer.size());
+        rc = QueryObjectTypeInformation(handle, type_info, &size);
+        while (rc == STATUS_INFO_LENGTH_MISMATCH ||
+               rc == STATUS_BUFFER_OVERFLOW) {
+          type_info_buffer.resize(size + sizeof(wchar_t));
+          type_info = reinterpret_cast<OBJECT_TYPE_INFORMATION*>(
+              &(type_info_buffer[0]));
+          rc = QueryObjectTypeInformation(handle, type_info, &size);
+          // Leave padding for the nul terminator.
+          if (NT_SUCCESS(rc) && size == type_info_buffer.size())
+            rc = STATUS_INFO_LENGTH_MISMATCH;
+        }
+
+        CHECK(NT_SUCCESS(rc));
+        CHECK(type_info->Name.Buffer);
+
+        type_info->Name.Buffer[type_info->Name.Length / sizeof(wchar_t)] =
+            L'\0';
+
+        // Should be an Event now.
+        CHECK_EQ(wcslen(type_info->Name.Buffer), 5U);
+        CHECK_EQ(wcscmp(L"Event", type_info->Name.Buffer), 0);
+
+        // Should not be able to wait.
+        CHECK_EQ(WaitForSingleObject(handle, INFINITE), WAIT_FAILED);
+
+        // Should be able to close.
+        CHECK_EQ(TRUE, CloseHandle(handle));
+      }
+      return SBOX_TEST_SUCCEEDED;
+
+    default:  // Do nothing.
+      break;
+  }
+
+  return SBOX_TEST_SUCCEEDED;
+}
+
 TEST(HandleCloserTest, CheckForMarkerFiles) {
   TestRunner runner;
   runner.SetTimeout(2000);
@@ -145,6 +230,24 @@ TEST(HandleCloserTest, CloseMarkerFiles) {
     "Failed: " << command;
 }
 
+TEST(HandleCloserTest, CheckStuffedHandle) {
+  TestRunner runner;
+  runner.SetTimeout(2000);
+  runner.SetTestState(EVERY_STATE);
+  sandbox::TargetPolicy* policy = runner.GetPolicy();
+
+  for (int i = 0; i < arraysize(kFileExtensions); ++i) {
+    base::string16 handle_name;
+    base::win::ScopedHandle marker(GetMarkerFile(kFileExtensions[i]));
+    CHECK(marker.IsValid());
+    CHECK(sandbox::GetHandleName(marker.Get(), &handle_name));
+    CHECK_EQ(policy->AddKernelObjectToClose(L"File", handle_name.c_str()),
+             SBOX_ALL_OK);
+  }
+
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(L"CheckForEventHandles"));
+}
+
 void WINAPI ThreadPoolTask(void* event, BOOLEAN timeout) {
   static volatile LONG waiters_remaining = kWaitCount;
   CHECK(!timeout);