Check for CloseHandle failures even when not debugging

Check for CloseHandle failures even when not debugging

Bug 524267 was a handle closing failure that only showed up when running
renderer processes under a debugger at startup, so it was not discovered
for a while. This change checks for handle closing failures always so
that subsequent bugs of this type will be detected immediately.

A DCHECK is used rather than a CHECK because this checks all in-process
calls to CloseHandle and will therefore trigger on any buggy third-party
DLL injections and we don't want that happening on customer machines.

Checking at individual call sites would necessarily give only partial
coverage because there are hundreds of CloseHandle call sites.

R=grt@chromium.org
BUG=524267

[brucedawson, 2015-09-14 21:45:36]

Whadya think? I think that with this change we can mark the bug as fixed.

[brucedawson, 2015-09-15 00:17:27]

I noticed a Chrome shutdown crash captured by WER in
urlmon!CRegZone::CRegZoneCache::~CRegZoneCache where it calls CloseHandle.
Unfortunately the call stack is damaged, probably because executing DCHECK
during shutdown is perilous.

Perhaps the DCHECK should be replaced with a debug-only __debuggerbreak()?

[brucedawson, 2015-09-15 00:23:04]

Update: That urlmon shutdown crash was from before I added the NULL check to the
DCHECK, so it should not happen now.

It may still be that a __debuggerbreak() call is safer than a DCHECK for a hook.
Maybe check with siggi@?

[grt (UTC plus 2), 2015-09-15 14:08:07]

grt@chromium.org changed reviewers:
+ rvargas@chromium.org

[grt (UTC plus 2), 2015-09-15 14:08:07]

This LGTM since it's a DCHECK, but I'd like rvargas to have a chance to comment.

https://codereview.chromium.org/1343873002/diff/1/chrome/app/close_handle_hoo...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/1343873002/diff/1/chrome/app/close_handle_hoo...
chrome/app/close_handle_hook_win.cc:45: DCHECK(result || handle == NULL ||
handle == INVALID_HANDLE_VALUE);
nit: DPCHECK so that the last error code is nicely formatted

[brucedawson, 2015-09-15 16:49:27]

https://codereview.chromium.org/1343873002/diff/1/chrome/app/close_handle_hoo...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/1343873002/diff/1/chrome/app/close_handle_hoo...
chrome/app/close_handle_hook_win.cc:45: DCHECK(result || handle == NULL ||
handle == INVALID_HANDLE_VALUE);
On 2015/09/15 14:08:07, grt wrote:
> nit: DPCHECK so that the last error code is nicely formatted

Done.

[rvargas (doing something else), 2015-09-15 18:08:04]

https://codereview.chromium.org/1343873002/diff/20001/chrome/app/close_handle...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/1343873002/diff/20001/chrome/app/close_handle...
chrome/app/close_handle_hook_win.cc:45: DPCHECK(result || handle == NULL ||
handle == INVALID_HANDLE_VALUE);
This seems like a weird place to looks for close failures.

On one hand, _our_ code should be CHECKing on close failures (yes, all
processes, all build types). That's what base/ generally does and that's what
other parts of the code that call CloseHandle directly should do.

On the other hand, this is an OS hook. While it is intended to help track handle
consistency, it covers all modules loaded on the process so we cannot generally
enforce our policies on them.

If urlmon is closing NULL (or INVALID_HANDLE_VALUE) handles we should fix that.

[brucedawson, 2015-09-15 18:33:27]

urlmon.dll is Microsoft's code so we can't fix it.

There is also room for disagreement about whether CloseHandle(NULL) is 'wrong'.
CloseHandle(NULL) returns FALSE, meaning failure, but it doesn't thrown an
exception when being debugged. So, supported (like delete nullptr), or not?
Unclear.

For the best place to check handle closing it is a coverage decision.
SharedMemory::CloseHandle doesn't check and that is what let bug 524267 sneak
through, and unfortunately there are a lot of other places that don't check. I
don't know if any of those other sites are likely to conceal bugs. Checking
elsewhere gives us incomplete coverage, checking in the hook gives us excessive
coverage (and no coverage when hooks are not applied).

I could instead or as-well add checks to some of the call sites in base. This
would let us enforce a policy of not closing NULL or INVALID_HANDLE_VALUE if we
want to.

[rvargas (doing something else), 2015-09-15 19:21:20]

On 2015/09/15 18:33:27, brucedawson wrote:
> urlmon.dll is Microsoft's code so we can't fix it.

doh!. I read urlmon and though about url/

> 
> There is also room for disagreement about whether CloseHandle(NULL) is
'wrong'.
> CloseHandle(NULL) returns FALSE, meaning failure, but it doesn't thrown an
> exception when being debugged. So, supported (like delete nullptr), or not?
> Unclear.

agreed. I would have thrown the towel too if I had to deal with throwing from
the debugger in that case. In fact, I did exactly that at some point with
ScopedHandle (but my squared mind still tells me it's an error to call
CloseHandle when Create failed, for example; that's just not the contract).

> 
> For the best place to check handle closing it is a coverage decision.
> SharedMemory::CloseHandle doesn't check and that is what let bug 524267 sneak
> through, and unfortunately there are a lot of other places that don't check. I
> don't know if any of those other sites are likely to conceal bugs. Checking
> elsewhere gives us incomplete coverage, checking in the hook gives us
excessive
> coverage (and no coverage when hooks are not applied).
> 
> I could instead or as-well add checks to some of the call sites in base. This
> would let us enforce a policy of not closing NULL or INVALID_HANDLE_VALUE if
we
> want to.

yes, I would prefer to grep/fix our calls to check after CloseHandle. Seems
simple enough instead of having to rely on hooks and deal with third party code.

Also, the ideal solution IMO is to change callers of CloseHandle to use
ScopedHandle instead. In fact, I think I filed a bug for doing that for
SharedMemory many moons ago.

[brucedawson, 2015-09-15 20:56:48]

Okay, sounds reasonable. I'll put together a CL to check some of our
explicit CloseHandle calls, preferably by using ScopedHandle. I won't
be attempting to capture all of them however. But running under the
debugger will still find those.

I'll probably ignore the ones where the alloc/use/free are sufficiently
close together to make errors unlikely, and focus on ones like
SharedMemory::CloseHandle where the usage is more spread out.

[brucedawson, 2015-09-17 04:33:46]

This was replaced by https://codereview.chromium.org/1350493002.
I'm closing this CL now.