win: Implement and use ScopedProcessSuspend

win: Implement and use ScopedProcessSuspend

R=mark@chromium.org
BUG=crashpad:1
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/5111a1823f7e464de8a83f5a96eedfb637cf8ade

[Mark Mentovai, 2015-09-04 15:57:51]

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend.cc (right):

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:26:
GetProcAddress(GetModuleHandle(L"ntdll.dll"), "NtSuspendProcess"));
What happens if this process suspends a target process and then this process
crashes so that the destructor never runs? Does the target remain suspended, or
does the system associate a suspension with a caller and implicitly resume it if
the caller dies?

OS X implements the latter behavior.

If Windows implements the former, there’s probably not anything that we can do,
although we should at least understand that it’s a limitation of
NtSuspendProcess, document it as a warning on ScopedSuspendProcess, and keep it
in the backs of our minds. Then, in the future, if we’re ever facing a bug where
we have lots of suspended processes and we don’t know why, we can think of this
day and this call and figure out whether Crashpad itself is crashing.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:28: LOG(ERROR) << "NtSuspendProcess failed";
Will this set GetLastError()? If so, this can be PLOG. Same on line 36.

Also, the string doesn’t need to say “failed”, just the function name is usually
enough (especially with PLOG).

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:30: // suspension, so we verify that all
threads match the desired suspend count.
This comment is a little terse given that the implementation calls an ntdll
function named NtSuspendProcess(). Do you mean that there’s no per-process
suspend count, and that NtSuspendProcess increments each thread’s suspend count?

(If so, for curiosity: does NtSuspendProcess() prevent new threads from being
created while it’s running too?)

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:42: if (te.th32OwnerProcessID ==
process_id) {
if (a) { if (b) { … }} → if (a && b) { … }

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:55: }
else return false? Better yet: if (a) { … } else { return false; } → if (!a) {
return false; } …

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:56: }
Same.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:71: EXPECT_EQ(c, ' ');
For gtest _EQ macros, it’s (expected, actual).

[scottmg, 2015-09-04 22:05:39]

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend.cc (right):

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:26:
GetProcAddress(GetModuleHandle(L"ntdll.dll"), "NtSuspendProcess"));
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> What happens if this process suspends a target process and then this process
> crashes so that the destructor never runs? Does the target remain suspended,
or
> does the system associate a suspension with a caller and implicitly resume it
if
> the caller dies?
> 
> OS X implements the latter behavior.
> 
> If Windows implements the former, there’s probably not anything that we can
do,
> although we should at least understand that it’s a limitation of
> NtSuspendProcess, document it as a warning on ScopedSuspendProcess, and keep
it
> in the backs of our minds. Then, in the future, if we’re ever facing a bug
where
> we have lots of suspended processes and we don’t know why, we can think of
this
> day and this call and figure out whether Crashpad itself is crashing.

It's a not-too-magic wrapper around Suspend/ResumeThread, and doesn't keep track
of where the suspends/resumes came from. So yes, it's not guaranteed to free up
the suspended process.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:28: LOG(ERROR) << "NtSuspendProcess failed";
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> Will this set GetLastError()? If so, this can be PLOG. Same on line 36.
> 
> Also, the string doesn’t need to say “failed”, just the function name is
usually
> enough (especially with PLOG).

No, they're NTSTATUS returns, not GLE. Added to LOG in case of failure.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:30: // suspension, so we verify that all
threads match the desired suspend count.
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> This comment is a little terse given that the implementation calls an ntdll
> function named NtSuspendProcess(). Do you mean that there’s no per-process
> suspend count, and that NtSuspendProcess increments each thread’s suspend
count?

That's right.

> 
> (If so, for curiosity: does NtSuspendProcess() prevent new threads from being
> created while it’s running too?)

Yes, there's a process lock taken in the kernel during suspend, resume, and
create. That's the main reason for NtSuspendProcess over just iterating threads
and calling SuspendThread on them, though it also seems a bit tidier.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:42: if (te.th32OwnerProcessID ==
process_id) {
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> if (a) { if (b) { … }} → if (a && b) { … }

Done.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:55: }
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> else return false? Better yet: if (a) { … } else { return false; } → if (!a) {
> return false; } …

Done.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:56: }
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> Same.

Done.

https://codereview.chromium.org/1303173011/diff/20001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:71: EXPECT_EQ(c, ' ');
On 2015/09/04 15:57:50, Mark Mentovai - August is over wrote:
> For gtest _EQ macros, it’s (expected, actual).

Still, eh? Done.

[Mark Mentovai, 2015-09-05 00:15:44]

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
File util/win/scoped_process_suspend.cc (right):

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:29: LOG(ERROR) << "NtSuspendProcess,
ntstatus=" << status;
I’ve probably said so before, but this would be a good use for NTSTATUS_LOG(),
and a “git grep” shows that maybe we’ve reached the point where it’d be
worthwhile.

See MACH_LOG() (base/mac/mach_logging.h) and OSSTATUS_LOG()
(base/mac/mac_logging.h, but not in mini_chromium).

This suggestion is only food for thought for a follow-up, not for this change at
all.

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:51: if (result >= 0)
result is a DWORD which is an unsigned long, right?

[scottmg, 2015-09-08 16:06:43]

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
File util/win/scoped_process_suspend.cc (right):

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
util/win/scoped_process_suspend.cc:29: LOG(ERROR) << "NtSuspendProcess,
ntstatus=" << status;
On 2015/09/05 00:15:43, Mark Mentovai - August is over wrote:
> I’ve probably said so before, but this would be a good use for NTSTATUS_LOG(),
> and a “git grep” shows that maybe we’ve reached the point where it’d be
> worthwhile.
> 
> See MACH_LOG() (base/mac/mach_logging.h) and OSSTATUS_LOG()
> (base/mac/mac_logging.h, but not in mini_chromium).
> 
> This suggestion is only food for thought for a follow-up, not for this change
at
> all.

Yeah, that sounds like a good idea.

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
File util/win/scoped_process_suspend_test.cc (right):

https://codereview.chromium.org/1303173011/diff/60001/util/win/scoped_process...
util/win/scoped_process_suspend_test.cc:51: if (result >= 0)
On 2015/09/05 00:15:43, Mark Mentovai - August is over wrote:
> result is a DWORD which is an unsigned long, right?

Good catch, thanks. Done.

[Mark Mentovai, 2015-09-08 16:10:40]

LGTM

[scottmg, 2015-09-08 17:09:36]

Committed patchset #5 (id:80001) manually as
5111a1823f7e464de8a83f5a96eedfb637cf8ade (presubmit successful).

[Mark Mentovai, 2015-09-08 20:42:38]

Hmm, one thing…

process_reader_win.cc’s FillThreadContextAndSuspendCount() gets the suspend
count, and with this giant ScopedProcessSuspend around everything, those suspend
counts are now going to show up off-by-one.

We should fix this.

[scottmg, 2015-09-09 00:13:46]

On 2015/09/08 20:42:38, Mark Mentovai - August is over wrote:
> Hmm, one thing…
> 
> process_reader_win.cc’s FillThreadContextAndSuspendCount() gets the suspend
> count, and with this giant ScopedProcessSuspend around everything, those
suspend
> counts are now going to show up off-by-one.
> 
> We should fix this.

Ah, that's true. https://codereview.chromium.org/1326443007/