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.

Similarly, bug 520305 is a long-standing base_unittests bug that only
shows up under a debugger.

This change fixes 520305 and checks for many handle closing failures
always so that subsequent bugs of this type will be detected
immediately.

Most of the CloseHandle calls in base are now checked.

This replaces the uncommitted https://codereview.chromium.org/1343873002/

R=grt@chromium.org,rvargas@chromium.org,dalecurtis@chromium.org
BUG=524267, 520305
Committed: https://crrev.com/9ae49a753d07f5a9266a63a89e7336d774f3fe37
Cr-Commit-Position: refs/heads/master@{#349333}

[brucedawson, 2015-09-16 00:30:52]

Take two at detecting handle errors. Adding checks in base functions actually
detected a bug that the hook *missed*. So that's cool. But, there will be paths
that this will miss. Still, progress.

Adding dalecurtis@ to review the 520305 fix (sync_socket_unittest.cc).

PTAL.

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:89: DCHECK(result);
This would have caught 524267.

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
File base/sync_socket_unittest.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
base/sync_socket_unittest.cc:97: SendReceivePeek(&socket_a, &socket_b);
This fixes 520305. Duplicating handles like that within a process is not valid.
I'm not sure what the point of the duplication was.

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc...
base/sync_socket_win.cc:239: DCHECK(result);
This detected 520305.

[DaleCurtis, 2015-09-16 00:43:19]

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
File base/sync_socket_unittest.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
base/sync_socket_unittest.cc:97: SendReceivePeek(&socket_a, &socket_b);
On 2015/09/16 00:30:52, brucedawson wrote:
> This fixes 520305. Duplicating handles like that within a process is not
valid.
> I'm not sure what the point of the duplication was.

This is just the same as the NormalSendReceivePeek() above then unfortunately --
If we can't fix this, we'll have to just delete this test.

[brucedawson, 2015-09-16 00:52:14]

Got it. The copying of handles requires either DuplicateHandle or cloning a
separate process (which duplicates the handles).

I'll delete the test.

[rvargas (doing something else), 2015-09-16 02:09:12]

https://codereview.chromium.org/1350493002/diff/20001/base/files/file_util_wi...
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/files/file_util_wi...
base/files/file_util_win.cc:238: if (!dir.IsValid())
nit: return dir.IsValid();

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:89: DCHECK(result);
On 2015/09/16 00:30:52, brucedawson wrote:
> This would have caught 524267.

Use CHECK

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc...
base/sync_socket_win.cc:239: DCHECK(result);
On 2015/09/16 00:30:52, brucedawson wrote:
> This detected 520305.

Use CHECK

https://codereview.chromium.org/1350493002/diff/20001/base/win/object_watcher...
File base/win/object_watcher_unittest.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/win/object_watcher...
base/win/object_watcher_unittest.cc:42: HANDLE event = CreateEvent(NULL, TRUE,
FALSE, NULL);
I think it's cleaner to use ScopedHandle for these.

https://codereview.chromium.org/1350493002/diff/20001/base/win/pe_image_test.cc
File base/win/pe_image_test.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/win/pe_image_test....
base/win/pe_image_test.cc:28: CHECK(::CloseHandle(h));
This is never executed, so I would not change this. (and besides, this would add
a dependency on base).

[grt (UTC plus 2), 2015-09-16 15:30:33]

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:162: base::win::ScopedHandle
process(OpenProcess(SYNCHRONIZE,
    Process process(Process::OpenWithAccess(entry->th32ProcessID, SYNCHRONIZE));

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:165: DWORD wait_result =
WaitForSingleObject(process.Get(), remaining_wait);
process.Handle()

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:165: DWORD wait_result =
WaitForSingleObject(process.Get(), remaining_wait);
should this check process.IsValid() before trying to wait?

[brucedawson, 2015-09-16 18:29:00]

Okay, most code review comments addressed. I'm just unsure about:
base/memory/shared_memory_win.cc:89 (rvargas@)
base/process/kill_win.cc:165 (grt@)

https://codereview.chromium.org/1350493002/diff/20001/base/files/file_util_wi...
File base/files/file_util_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/files/file_util_wi...
base/files/file_util_win.cc:238: if (!dir.IsValid())
On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> nit: return dir.IsValid();

D'oh! Of course.

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:89: DCHECK(result);
On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> On 2015/09/16 00:30:52, brucedawson wrote:
> > This would have caught 524267.
> 
> Use CHECK

The reason I used DCHECK was because of the DCHECK above. If handle is NULL (or
INVALID_HANDLE_VALUE) then CloseHandle will fail and I wasn't sure I wanted that
causing customer crashes. Thoughts?

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:162: base::win::ScopedHandle
process(OpenProcess(SYNCHRONIZE,
On 2015/09/16 15:30:33, grt wrote:
>     Process process(Process::OpenWithAccess(entry->th32ProcessID,
SYNCHRONIZE));

Done.

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:165: DWORD wait_result =
WaitForSingleObject(process.Get(), remaining_wait);
On 2015/09/16 15:30:33, grt wrote:
> should this check process.IsValid() before trying to wait?

That seems like a separate change - I'm not sure what it achieves.

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:165: DWORD wait_result =
WaitForSingleObject(process.Get(), remaining_wait);
On 2015/09/16 15:30:33, grt wrote:
> process.Handle()

Done.

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
File base/sync_socket_unittest.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_unitte...
base/sync_socket_unittest.cc:97: SendReceivePeek(&socket_a, &socket_b);
Tests deleted.

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/sync_socket_win.cc...
base/sync_socket_win.cc:239: DCHECK(result);
On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> On 2015/09/16 00:30:52, brucedawson wrote:
> > This detected 520305.
> 
> Use CHECK

Done.

https://codereview.chromium.org/1350493002/diff/20001/base/win/object_watcher...
File base/win/object_watcher_unittest.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/win/object_watcher...
base/win/object_watcher_unittest.cc:42: HANDLE event = CreateEvent(NULL, TRUE,
FALSE, NULL);
On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> I think it's cleaner to use ScopedHandle for these.

Agreed. I can't remember why I didn't.

https://codereview.chromium.org/1350493002/diff/20001/base/win/pe_image_test.cc
File base/win/pe_image_test.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/win/pe_image_test....
base/win/pe_image_test.cc:28: CHECK(::CloseHandle(h));
Ignore this change. I'm reverting it. The CHECK doesn't compile and I don't
think the code is run.

https://codereview.chromium.org/1350493002/diff/20001/base/win/pe_image_test....
base/win/pe_image_test.cc:28: CHECK(::CloseHandle(h));
On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> This is never executed, so I would not change this. (and besides, this would
add
> a dependency on base).

Done.

[rvargas (doing something else), 2015-09-16 18:54:38]

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:89: DCHECK(result);
On 2015/09/16 18:29:00, brucedawson wrote:
> On 2015/09/16 02:09:12, rvargas (slow to respond) wrote:
> > On 2015/09/16 00:30:52, brucedawson wrote:
> > > This would have caught 524267.
> > 
> > Use CHECK
> 
> The reason I used DCHECK was because of the DCHECK above. If handle is NULL
(or
> INVALID_HANDLE_VALUE) then CloseHandle will fail and I wasn't sure I wanted
that
> causing customer crashes. Thoughts?

There's no semantic distinction between DCHECK and CHECK*, as in, if there's a
DCHECK somewhere it should mean that the condition should never happen so it
should be OK to replace it with a CHECK (with the only downside of increasing
the binary size). In other words, line 87 states that it is a bug to reach this
method with a uninitialized handle.

[*] I'm of course ignoring security implications.

[grt (UTC plus 2), 2015-09-16 19:27:02]

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (right):

https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
base/process/kill_win.cc:165: DWORD wait_result =
WaitForSingleObject(process.Get(), remaining_wait);
On 2015/09/16 18:29:00, brucedawson wrote:
> On 2015/09/16 15:30:33, grt wrote:
> > should this check process.IsValid() before trying to wait?
> 
> That seems like a separate change - I'm not sure what it achieves.

I assume Wait will fail with an error if the handle is invalid. It looks like
there's a race here that will lead to the function returning false if a process
exits naturally between getting the pid and opening the handle. I'd argue that
this is wrong: false should mean that the function failed to wait for them all
to exit, not that some exited normally without needing a wait. This could be
something for another CL.

[brucedawson, 2015-09-16 23:06:02]

On 2015/09/16 19:27:02, grt wrote:
> https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.cc
> File base/process/kill_win.cc (right):
> 
>
https://codereview.chromium.org/1350493002/diff/20001/base/process/kill_win.c...
> base/process/kill_win.cc:165: DWORD wait_result =
> WaitForSingleObject(process.Get(), remaining_wait);
> On 2015/09/16 18:29:00, brucedawson wrote:
> > On 2015/09/16 15:30:33, grt wrote:
> > > should this check process.IsValid() before trying to wait?
> > 
> > That seems like a separate change - I'm not sure what it achieves.
> 
> I assume Wait will fail with an error if the handle is invalid. It looks like
> there's a race here that will lead to the function returning false if a
process
> exits naturally between getting the pid and opening the handle. I'd argue that
> this is wrong: false should mean that the function failed to wait for them all
> to exit, not that some exited normally without needing a wait. This could be
> something for another CL.

If it is a race then the proper fix is to hang on to the handle instead of
getting it from the PID. PIDs are reused aggressively so raciness means we could
end up hanging while waiting for an unrelated process to exit. Definitely a
separate CL.

[brucedawson, 2015-09-16 23:10:26]

Okay, DCHECK changed to CHECK. PTAL.

Slow responses today - I'm home sick.

[rvargas (doing something else), 2015-09-16 23:43:47]

Re. kill: To being, that sort of API is destined to fail to say the least. Once
we are deciding to kill (or wait for) processes based on names or PIDs we are in
a bad place... but for some reason there's plenty of code with that pattern :(.

LGTM and thanks for the change.

[DaleCurtis, 2015-09-16 23:56:35]

lgtm

[grt (UTC plus 2), 2015-09-17 00:03:48]

lgtm

[brucedawson, 2015-09-17 01:27:21]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-09-17 01:27:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350493002/60001

[commit-bot: I haz the power, 2015-09-17 02:49:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-17 02:50:23]

Patchset 4 (id:??) landed as
https://crrev.com/9ae49a753d07f5a9266a63a89e7336d774f3fe37
Cr-Commit-Position: refs/heads/master@{#349333}

[brucedawson, 2015-09-18 18:23:42]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1356743003/ by brucedawson@chromium.org.

The reason for reverting is: The SharedMemory checks are causing CHECK crashes
in the printing system. See bug 533310. Reverting to allow time for an
investigation of that underlying problem..

[Lei Zhang, 2015-09-22 08:03:30]

https://codereview.chromium.org/1350493002/diff/60001/base/memory/shared_memo...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/1350493002/diff/60001/base/memory/shared_memo...
base/memory/shared_memory_win.cc:89: CHECK(result);
I know this will probably reland as soon as I fix the printing bug. (hopefully
sometime today) Can you change this to a PCHECK() ?

https://codereview.chromium.org/1350493002/diff/60001/base/sync_socket_win.cc
File base/sync_socket_win.cc (right):

https://codereview.chromium.org/1350493002/diff/60001/base/sync_socket_win.cc...
base/sync_socket_win.cc:239: CHECK(result);
Also PCHECK()

[Lei Zhang, 2015-09-22 08:39:19]

And do we need to handle INVALID_HANDLE_VALUE better in
base/memory/shared_memory_win.cc? The print bug is a case of |mapped_file_|
being set to INVALID_HANDLE_VALUE.

[brucedawson, 2015-09-22 23:06:17]

On 2015/09/22 08:39:19, Lei Zhang wrote:
> And do we need to handle INVALID_HANDLE_VALUE better in
> base/memory/shared_memory_win.cc? The print bug is a case of |mapped_file_|
> being set to INVALID_HANDLE_VALUE.

What counts as handling it better? https://codereview.chromium.org/1351373002/
adds first-class support for INVALID_HANDLE_VALUE as an invalid handle in
SharedMemory. The alternative would be to bar all of the entry points to not let
INVALID_HANDLE_VALUE in. Either method works.

[Lei Zhang, 2015-09-22 23:13:26]

On 2015/09/22 23:06:17, brucedawson wrote:
> On 2015/09/22 08:39:19, Lei Zhang wrote:
> > And do we need to handle INVALID_HANDLE_VALUE better in
> > base/memory/shared_memory_win.cc? The print bug is a case of |mapped_file_|
> > being set to INVALID_HANDLE_VALUE.
> 
> What counts as handling it better? https://codereview.chromium.org/1351373002/
> adds first-class support for INVALID_HANDLE_VALUE as an invalid handle in
> SharedMemory. The alternative would be to bar all of the entry points to not
let
> INVALID_HANDLE_VALUE in. Either method works.

Those are the two options I had in mind too. I have no preferences.

[brucedawson, 2015-09-23 00:20:49]

On 2015/09/22 23:13:26, Lei Zhang wrote:
> On 2015/09/22 23:06:17, brucedawson wrote:
> > On 2015/09/22 08:39:19, Lei Zhang wrote:
> > > And do we need to handle INVALID_HANDLE_VALUE better in
> > > base/memory/shared_memory_win.cc? The print bug is a case of
|mapped_file_|
> > > being set to INVALID_HANDLE_VALUE.
> > 
> > What counts as handling it better?
https://codereview.chromium.org/1351373002/
> > adds first-class support for INVALID_HANDLE_VALUE as an invalid handle in
> > SharedMemory. The alternative would be to bar all of the entry points to not
> let
> > INVALID_HANDLE_VALUE in. Either method works.
> 
> Those are the two options I had in mind too. I have no preferences.

Prohibiting INVALID_HANDLE_VALUE is cleaner. It comes with the risk of causing
canary crashes if it finds another instance of invalid usage of
INVALID_HANDLE_VALUE.

The one exception would be that if we decide to use a ScopedHandle to implement
SharedMemory then that means that it accepts INVALID_HANDLE_VALUE.

rvargas@ - any thoughts?

[rvargas (doing something else), 2015-09-23 00:37:50]

On 2015/09/23 00:20:49, brucedawson wrote:
> On 2015/09/22 23:13:26, Lei Zhang wrote:
> > On 2015/09/22 23:06:17, brucedawson wrote:
> > > On 2015/09/22 08:39:19, Lei Zhang wrote:
> > > > And do we need to handle INVALID_HANDLE_VALUE better in
> > > > base/memory/shared_memory_win.cc? The print bug is a case of
> |mapped_file_|
> > > > being set to INVALID_HANDLE_VALUE.
> > > 
> > > What counts as handling it better?
> https://codereview.chromium.org/1351373002/
> > > adds first-class support for INVALID_HANDLE_VALUE as an invalid handle in
> > > SharedMemory. The alternative would be to bar all of the entry points to
not
> > let
> > > INVALID_HANDLE_VALUE in. Either method works.
> > 
> > Those are the two options I had in mind too. I have no preferences.
> 
> Prohibiting INVALID_HANDLE_VALUE is cleaner. It comes with the risk of causing
> canary crashes if it finds another instance of invalid usage of
> INVALID_HANDLE_VALUE.
> 
> The one exception would be that if we decide to use a ScopedHandle to
implement
> SharedMemory then that means that it accepts INVALID_HANDLE_VALUE.
> 
> rvargas@ - any thoughts?

I vote for both:
- If SharedMemory receives random handles, the value for invalid is null, vs -1.
(as a matter of API, documented)
- SharedMemory should not receive random handles :)
- As an intermediate step, SharedMemory should rely on ScopedHandle (which
basically means that -1 will be accepted again, but that is an implementation
detail, which happens to be consistent with the long term plan of not having raw
handles and instead receiving ScopedHandles). Nobody should be creating
ScopedHandle(INVALID_HANDLE_VALUE).