win: Support dumping another process by causing it to crash [crashpad/crashpad : master]

[Sigurdur Asgeirsson (Gerrit)]

Sigurdur Asgeirsson has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 1:

(1 comment)

Sad trombone :(.

BTW: I can't figure how to comment from my Chromium account - it gives me a
weird error.

---
403. That's an error.

si...@chromium.org has already been registered under a different account
accessible by:

si...@google.com

Sign in to another account

That's all we know.

https://chromium-review.googlesource.com/#/c/339263/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

PS1, Line 529: remote_thread
I'm almost 100% sure that this won't do the trick for our purposes.
If "process" is in a deadly embrace on the loader's lock, I'm pretty sure
this thread will just park against the same lock before attempting to
execute the provided "thread proc".
This is because of DLL notifications, which are issued from the
ntdll-underlying thread proc.


--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 1
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>
Gerrit-HasComments: Yes

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 1:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 529: HANDLE remote_thread =

> I'm almost 100% sure that this won't do the trick for our purposes.
> If "pro

Yeah, you're right. :/

I dug into this a bit, and came up with a workaround for the loader lock
being held (using NtCreateThreadEx with the "don't send attach
notifications" flag). I'm not sure if is getting too ugly or not:
https://github.com/sgraham/crash_target_test/blob/master/host.cc#L48.

WDY'allT?

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 1
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>

Gerrit-Reviewer: Scott Graham <sco...@chromium.org>

[Sigurdur Asgeirsson (Gerrit)]

Sigurdur Asgeirsson has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 1:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/1/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 529: HANDLE remote_thread =

> Yeah, you're right. :/

I still think we'll end up needing to do this from out of process, but if
this works, it works.

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 2:

Updated to make it crash a little harder.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 2

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Sigurdur Asgeirsson (Gerrit)]

Sigurdur Asgeirsson has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 2:

(3 comments)

lgtm for what it's worth. Your CL description needs updating?

https://chromium-review.googlesource.com/#/c/339263/2/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

PS2, Line 537: 0x1fffff
These are synonymous with Win32 permission flags. Maybe you can simply ask
for SYNCHRONIZE or some such?


PS2, Line 540: nullptr
maybe worth noting what these things are and how they crash the thread? I'm
guessing you'll end up with an attempted execute at address zero?


https://chromium-review.googlesource.com/#/c/339263/2/handler/win/hanging_program.cc
File handler/win/hanging_program.cc:

PS2, Line 27: const
In the crash refinery we found that this wasn't always effective when the
definition shares a compilation unit with the use - we had to hide the
definition off in another compilation unit :/

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 2
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: Yes

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 3:

(3 comments)

Thanks. Unfortunately, I can't update the CL description due to
https://bugs.chromium.org/p/chromium/issues/detail?id=603207 .

The exception handler now grabs the thread id and exception code and
fabricates the record if that was provided, so this should be good to go
now.

https://chromium-review.googlesource.com/#/c/339263/2/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 537: return false;

> These are synonymous with Win32 permission flags. Maybe you can simply ask

Oops, done.


Line 540: // Cause an exception in the target process by creating a
thread with an entry

> maybe worth noting what these things are and how they crash the thread?
> I'm

That's in the comment block above, but commented the start_routine argument
specifically.


https://chromium-review.googlesource.com/#/c/339263/2/handler/win/hanging_program.cc
File handler/win/hanging_program.cc:

Line 27: }

> In the crash refinery we found that this wasn't always effective when the
> d

Pulled base::debug::Alias() into mini_chromium here
https://chromium-review.googlesource.com/c/339565/ and used that.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 3

[Scott Graham (Gerrit)]

Scott Graham has abandoned this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Abandoned

--
To view, visit https://chromium-review.googlesource.com/339632

To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I217e93e4a0520ea34cf3bfc2b395acf40430dd97
Gerrit-PatchSet: 1

[Scott Graham (Gerrit)]

Scott Graham has abandoned this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Abandoned

--

To view, visit https://chromium-review.googlesource.com/339634

To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: abandon

Gerrit-Change-Id: Id54e5500447affbba3081250c8311ae7c6cb8dc3

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 4:

(12 comments)

This is the most commenty review in the series, but I’m sure that’s
expected.

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client.h
File client/crashpad_client.h:

Line 164: //! referring to the debug break location, an exception
record will be
This implies that DebugBreak[Process]() is used, but that’s no longer the
case.


Line 169: //! \return `true` if the debug break was successfully
triggered.
Again.


https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 485: // Check the main module for a CrashpadInfo structure.
Why’s it got to be in the main module?


Line 488: return false;
So just give up if any of these things don’t work?


Line 514:
You should check that the CrashpadInfo structure is what you think it is
before writing to it. Check the magic number, size, and version.


Line 519: offsetof(CrashpadInfo,
target_thread_id_)),
This and the next WriteProcessMemory() aren’t cross-bitness aware like the
reads above are.


Line 541: // point of 0. Note that we do not use DebugBreakProcess() as
it only works
I would have preferred an entry point that affirmatively crashes to help
this stand out as what it really is in decoded crash reports, but I guess
there are no guarantees about what code might be loaded into a remote
process. A jump to 0 happens often enough in the real world that I’m afraid
of ambiguity.

We could WriteProcessMemory() a crashy instruction sequence. ud2’s my
favorite, but RaiseException() may be better, allowing these crashes to be
identified pretty precisely AND probably eliminate the need to stash
anything in CrashpadInfo. That’d be a little bit of extra work, it requires
having a good place to write it available, might require allocating
something (and maybe the map is full), etc. Basically, no good solutions.
So I guess jump-to-0 is fine, it’s just a different set of compromises.
Have you given any thought to the alternatives?

Using RaiseException() actually sounds particularly interesting. It’d let
us eliminate the need to find any CrashpadInfo (and deal with the “which
module” question) and to write to the target’s memory (and deal with the
“which bitness” question, and other questions I haven’t asked like multiple
DumpAndCrashTargetProcess() racing). It’d allow us to reuse more of the
machinery that we already have, at the cost of some quirky and very
machine-specific code right here, but maybe it’s not that bad given that
it’d save us from some of the other contortions. All of this, of course, is
subject to the “if we can get a place to write our instructions” thing. Or,
if we’re not guaranteed that, to the “is the risk of not having a place to
write our instructions smaller than the risk of the problems with this
other approach” thing.


Line 569: return true;
I kind of want to zero out the fields that you wrote to when things are
done, so that this can all be recycled easily to form the pretty obvious
DumpTargetProcessWithoutCrashing() alternative without leaving that state
polluted so that future crashes look like dumps from the outside. But the
actual crash is totally async with what happens here, so I guess you’d need
to clear it from over in the handler.


https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_info.h
File client/crashpad_info.h:

Line 201: // These fields area written by
CrashpadClient::DumpAndCrashTargetProcess()
area→are


Line 202: // from a client to a target process before causing the target
to crash. If
from→by


Line 203: // target_thread_id is non-zero it indicates that this thread
should be used
“blah that this thread blah blaming this thread,” so that by the time I got
to the end of the sentence, it wasn’t clear what the final “this
thread” actually referred to.


https://chromium-review.googlesource.com/#/c/339263/4/handler/win/crash_report_exception_handler.cc
File handler/win/crash_report_exception_handler.cc:

Line 65: auto main_module = static_cast<const
internal::ModuleSnapshotWin*>(
Here’s this main_module thing again. I don’t get that. The victim might be
a Crashpad client but might have a Chrome-like structure where most of the
app is in a dll. In Chrome’s case on Windows, Crashpad currently happens to
live in the exe, but that’s something that wasn’t always certain when you
were designing it, it might not always be the case for Chrome, and it might
not be the case for other consumers.


--
To view, visit https://chromium-review.googlesource.com/339263

To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 4

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>

Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 541: // point of 0. Note that we do not use DebugBreakProcess() as
it only works
> I would have preferred an entry point that affirmatively crashes to help
> th

Injecting a call to RaiseException() is a good idea. The main drawback
seems to be that we can't nominate the blamed thread as the API currently
allows, as the exception would be raised from the injected thread. At
least, I can't immediately think of a way to do that. (SetThreadContext()
on the target thread? Seems a bit complicated and potentially tricky if the
target thread was suspended.)

Or did you mean stashing the thread id as data near the injected code to
call RaiseException() and then having the handler still fabricate an
exception record? I think that would work. In that case, we might as well
just use ud2 followed by the two DWORDs we need to communicate.

At the moment, the signal for whether to fabricate an exception is whether
the target_thread_id field in the CrashpadInfo is set to non-zero. In this
new approach, we wouldn't have a definitive signal that it was a
fake/target-kill exception, meaning it would have to attempt to fabricate a
record for

More broadly, I do like writing some code into the target to avoid the
CrashpadInfo dependency. It doesn't seem much additional risk/cost as we're
already requiring enough space to create a thread stack (and for thread
creation to succeed).

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 541: // point of 0. Note that we do not use DebugBreakProcess() as
it only works

> Injecting a call to RaiseException() is a good idea. The main drawback
> seem

[3rd paragraph should end "meaning it would have to attempt to fabricate a
record for all invalid opcode, or 'ud2' exceptions." Although now that I
write that we could write

ud2 <some magic signature> <thread_id> <exception_code>

and be relatively confident it was really "sent" from
DumpAndCrashTargetProcess().]

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 541: // point of 0. Note that we do not use DebugBreakProcess() as
it only works

> [3rd paragraph should end "meaning it would have to attempt to fabricate a

I thought we might be able to stash the extra bits in the exception itself,
by using nNumberOfArguments and lpArguments when calling RaiseException().
On the other side, in the handler, we could detect our custom
dwExceptionCode and then process those arguments. Our custom exception code
becomes the clear signal that this was a remote dump-and-crash request, and
then we could decide how to interpret the extra arguments.

I guess that this is the same as ud2 blah, especially if we can cook up a
signature that's extremely unlikely to follow ud2. It requires reading the
victim's memory after every illegal-opcode exception to make the call,
whereas the RaiseException() thing only requires us to do the extra
processing when we see our magic exception code, and we'd get the rest of
data in the EXCEPTION_RECORD without having to read memory. (Hopefully
that's all properly plumbed through to the handler already.)

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 541: // point of 0. Note that we do not use DebugBreakProcess() as
it only works

> I thought we might be able to stash the extra bits in the exception
> itself,

Gotcha. It would be excellent if there was no extra code required at all in
the handler, but it seems we can't avoid that.

My only other reservation for RaiseException() is the address lookup. It's
easy to get for a process that's the same bitness (because kernel32 is in
the same location for all processes) but if an x64 client requests a dump
on an x86 target then the injected code would have to do a GetProcAddress()
(I think?), and that requires a string argument which makes the generated
code start to get fairly convoluted.

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

I see where you're heading. Another concern: would GetProcAddress() take
the loader lock, or would that just be LoadLibrary()/GetModuleHandle()?

Now that we're talking about this, does NtCreateThreadEx() create the
remote thread in the native bitness of the caller or does it set up a
32-bit environment in a WoW64 process? I assume that the unhandled
exception filter runs on the created thread, a WoW64 process would have a
32-bit UEF, and I hope that this all works out cleanly.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 4
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

(That second paragraph is a way of saying "we can't just ignore this if we
use ud2, the bitness question is relevant to a ud2 implementation too.")

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 4:

> Patch Set 4:

> I see where you're heading. Another concern: would GetProcAddress() take
> the loader lock, or would that just be LoadLibrary()/GetModuleHandle()?

Thinking about it some more, we should be able to resolve this without
running code to do so in the target. We can get the base address of
kernel32 in the other bitness by reading the \KnownDlls32\kernel32.dll
section (or probably other ways) and then we could walk through the export
table to get the function address. In fact it looks like one "MrNukealizer"
has already done so
http://www.codeproject.com/Tips/139349/Getting-the-address-of-a-function-in-a-
DLL-loaded . i.e. just reimplementing GetProcAddress() but we could be
confident it wasn't taking the loader lock in-process, anyway.

But having said that, I'll go with just implementing the same-bitness
version since that should be relatively straightforward, and since Chrome
will be the only immediate consumer, and it doesn't require cross-bitness
target dumping at the moment.

> Now that we're talking about this, does NtCreateThreadEx() create the
> remote thread in the native bitness of the caller or does it set up a
> 32-bit environment in a WoW64 process? I assume that the unhandled
> exception filter runs on the created thread, a WoW64 process would have a
> 32-bit UEF, and I hope that this all works out cleanly.

Not sure, I'd go with hoping it all just works out, but I would have to
test that for cross-bitness.


So, in summary, I think I will switch to an injected RaiseException() call,
and a hopefully a bit less code in the handler's InitializeException() path
that's special to this case.

I'm not sure if we should still move the pe_image_reader, etc. from
snapshot to util. We won't need it here now for reading CrashpadInfo. But
the code that's in snapshot/api feels like it's in the wrong place and
belongs in client, but can't be there now because of using those classes to
read the annotations.

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

If it's not going to work cross-bitted for now because you're just using
the known address of RaiseException() from the requesting process, then we
should make the client call fail early when it knows that it's in an
impossible cross-bitted situation. IsWow64Process(), I guess. Make sure to
document the limitation, or leave behind a TODO pointing to Mr. Nukealizer.

It's basically a PE reader. Hmm, I wonder where we could get our hands on
one of those. :)

> So, in summary, I think I will switch to an injected RaiseException()
> call, and a hopefully a bit less code in the handler's
> InitializeException() path that's special to this case.

I think that this sounds good.

At least until some unexpected thing crops up that sends us back to the
drawing board.

> I'm not sure if we should still move the pe_image_reader, etc. from
> snapshot to util. We won't need it here now for reading CrashpadInfo. But
> the code that's in snapshot/api feels like it's in the wrong place and
> belongs in client, but can't be there now because of using those classes
> to read the annotations.

Let's leave it alone for the time being, since there's no sense in exposing
it more widely if it's not actually needed more widely.

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 8:

(2 comments)

Implemented the x86 injection only so far, but I think this is much
improved (in that keeps things localized more nicely). Maybe you want to
take a look now to see if this is what you were expecting.

https://chromium-review.googlesource.com/#/c/339263/4/client/crashpad_client.h
File client/crashpad_client.h:

Line 164: //! \param[in] process A `HANDLE` identifying the process to be
dumped.

> This implies that DebugBreak[Process]() is used, but that's no longer the
> c

Done


Line 169: //! \param[in] exception_code If \a thread_id is non-zero, this
will be used
> Again.
Done

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 8

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: Yes

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 9:

Added x64 code to call to RaiseException() in the latest patch set.
Unfortunately seems to skip SetUnhandledExceptionFilter() and go straight
to the "program has crashed" system dialog. Argh!

Going to do a bit more testing to make sure it's not because the injection
code is somehow messing up the exception handler setup, but I have the
feeling that might be just how RaiseException() works on x64.

So might need to rework yet-again using ud2.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 9

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 10:

> Patch Set 9:

> Added x64 code to call to RaiseException() in the latest patch set.
> Unfortunately seems to skip SetUnhandledExceptionFilter() and go straight
> to the "program has crashed" system dialog. Argh!

> Going to do a bit more testing to make sure it's not because the
> injection code is somehow messing up the exception handler setup, but I
> have the feeling that might be just how RaiseException() works on x64.

> So might need to rework yet-again using ud2.

A simple test program demonstrates the above. However, adding a vectored
exception handler
https://msdn.microsoft.com/en-us/library/windows/desktop/ms679274(v=vs.85).aspx
makes things work as expected. I guess we probably should have been doing
that all along. (It's also another one to disable post-installation in
https://bugs.chromium.org/p/crashpad/issues/detail?id=106.)

So this is ready for a look now.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 10

[Sigurður Ásgeirsson]

A VEH gets first chance exceptions. Most of those are not crashes, but end up being handled by all and sundry - RPC machinery etc. 

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 11:

(Another minor update, now the hanging test program blocks with the loader
lock taken as were concerned with that case, and I realized I wasn't
setting the thread id properly in the fake record, fixed that + test.)

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 11

[Scott Graham]

Sigh, this didn't show up on https://chromium-review.googlesource.com/c/339263/. I guess Gerrit doesn't do email the other way. :(

Thanks for the correction in any case. So I guess I'll rework to use ud2 instead. Shucks.

[Sigurður Ásgeirsson]

What's UD2?

[Scott Graham]

http://www.tptp.cc/mirrors/siyobik.info/instruction/UD2.html

Instead of calling RaiseException(), writing an invalid instruction followed by a magic signature of some sort (so that we can believe it's "our" ud2), followed by the data we are passing via RaiseException() at the moment.

Can't believe how many times I've re-written this! Hopefully getting closer.

[Mark Mentovai]

It's guaranteed to raise #UD, the undefined instruction trap. It's the "officially undefined" instruction.

[Mark Mentovai]

Final suggestion: call the UEF directly, if the pointer to it is really available. Just for ease of disambiguation and getting the data to the right place easily.

I haven't checked whether the pointer is readily available. I'd believe both "yes" and "no".

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 12:

Sorry, I'm going around in circles here still.

I realized that my test program was calling

SetUnhandledExceptionFilter(&UnhandledExceptionFilter);

thus causing a stack overflow exception (it should have been
&UnhandledExceptionHandler), oops!

So in a test program (even when it does the same injection via
NtCreateThreadEx()) I'm now unable to get it to miss the the UEF, whether
in x64 or x86.

In looking at what cl.exe generated for x64, I noticed it cleverly just
does effectively:
mov r9, xyz
mov r8, xyz
mov rdx, xyz
mov rcx, xyz
jmp RaiseException

that is, it avoids setting up a stack frame or using call. When I change
the x64 injection to do that, everything works as expected.

So I think my first try at injection code was probably broken in some way I
didn't realize, likely incorrectly setting up the stack.

So I think we're good as-is.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 12

[Mark Mentovai]

Cool, Scott. So this is ready for re-review now?

[Scott Graham]

Yes please. I promise I won't touch it any more, unless you see a problem. :)

[Mark Mentovai]

You can keep messing with it if you need, I just wasn’t sure if all of those uploads were for play or for real. I’ll have a look either way.

--
You received this message because you are subscribed to the Google Groups "Crashpad-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to crashpad-dev...@chromium.org.
To post to this group, send email to crashp...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/crashpad-dev/CANHK6RaWJXw%3DTmfOG2mAxW_5QiA0gGp%2BXDV6MxXRuzUzCE_0Cw%40mail.gmail.com.

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 13:

(10 comments)

https://chromium-review.googlesource.com/#/c/339263/13//COMMIT_MSG
Commit Message:

Line 7: win: Support dumping another process by causing it to crash
The commit message looks good!


https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client.h
File client/crashpad_client.h:

Line 169: //! \param[in] exception_code If \a thread_id is non-zero, this
will be used

Should we leave exception_code in the client's control, or just send our
magic one via this interface?


Line 181: //! process. Note that we avoid setting the top nibble,
as most real
The "note" sentence isn't really "brief." You can use \note for this.


https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 508: 4 << 10,
I match "* 1024" better than "<< 10".

I trust that our tests will catch any case in which 4kB isn't enough. I
don't anticipate it being too little, we just need enough to raise the
exception and run the unhandled exception filter, which we've intentionally
kept very simple.


Line 510:
PAGE_EXECUTE_READWRITE));
Beneficial to make this read-write while we're splatting stuff into it and
then read-execute when we're ready to inject the thread?


Line 519: WinVMAddress raise_exception_address =
reinterpret_cast<WinVMAddress>(
I doubt you need to do this. Can't you just say &RaiseException?


Line 589: // push <data_array_address>
Where's the stack for this new thread? The same question applies for
x86_64. Even though the code you wrote there doesn't use the stack,
RaiseException() might, and the unhandled exception filter definitely will.


Line 594: //
Can you stick some poison here like hlt in case of an unexpected return
from RaiseException()?


Line 639: if (!FlushInstructionCache(
Surprised to see this, since it's not required on x86 and that's all that
we support.


Line 662: 0,
These StackSize/MaximumStackSize arguments down here were going to answer
my question about the stack from above, but they're 0. Does that mean
"default?"

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 13

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: Yes

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 13:

(9 comments)

Thanks!

https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client.h
File client/crashpad_client.h:

Line 169: //! \param[in] exception_code If \a thread_id is non-zero, this
will be used
> Should we leave exception_code in the client's control, or just send our
> ma

I think if the client is controlling the thread id, it seems reasonable
that they might want to set a particular exception code that makes sense to
them. But it probably doesn't matter too much. I can remove it if you think
it's more clear.

Line 181: //! process. Note that we avoid setting the top nibble,
as most real
> The "note" sentence isn't really "brief." You can use \note for this.

Done

https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 508: 4 << 10,
> I match "* 1024" better than "<< 10".

We need < 100 bytes at the moment, but it's going to be a full page in any
case.

I guess we could get unlucky after a complicated refactor and end up
allocating this right next to other memory that was writable, and then
overflow into it. Added a DCHECK that we don't exceed the limit before the
WriteProcessMemory().

Line 510:
PAGE_EXECUTE_READWRITE));
> Beneficial to make this read-write while we're splatting stuff into it and

I don't see any upside to doing that. The fewer calls we make here, the
better, I think.

Line 519: WinVMAddress raise_exception_address =
reinterpret_cast<WinVMAddress>(
> I doubt you need to do this. Can't you just say &RaiseException?

Hah, GetProcAddress() on the brain. Done.

Line 589: // push <data_array_address>
> Where's the stack for this new thread? The same question applies for
> x86_64

Yes, the injected stack will get the default stack size from the target's
PE header:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686774(v=vs.85).aspx .
Are you thinking maybe we want to try to squish it smaller?

Line 594: //
> Can you stick some poison here like hlt in case of an unexpected return
> fro

How about ud2, since hlt is ring 0? Done. (Not in the x64 case, since
there's no way we can return there.)

Line 639: if (!FlushInstructionCache(
> Surprised to see this, since it's not required on x86 and that's all that
> w

Hmm, true. It is documented as being required per Remarks in
https://msdn.microsoft.com/en-us/library/windows/desktop/ms679350(v=vs.85).aspx
and Raymond even says it might be
https://blogs.msdn.microsoft.com/oldnewthing/20031208-00/?p=41583 (though I
think he may be mistaken). But yeah, it doesn't actually do anything. I can
remove it if you prefer.

Line 662: 0,
> These StackSize/MaximumStackSize arguments down here were going to answer
> m

Right, defaults.

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 14:

(9 comments)

https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client.h
File client/crashpad_client.h:

Line 169: //! \param[in] exception_code If \a thread_id is non-zero, this
will be used

> I think if the client is controlling the thread id, it seems reasonable
> tha

You can leave it if you want, it was just something that struck me as maybe
more control than the client needs.


https://chromium-review.googlesource.com/#/c/339263/14/client/crashpad_client.h
File client/crashpad_client.h:

Line 165: //! \param[in] thread_id If non-zero, instead of the exception
record
In light of the stuff that you and Siggi were discussing about IDs not
being unique, do we need to use (thread_id, start_time) to accurately
identify it?


https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 510: MEM_RESERVE |
MEM_COMMIT,

> I don't see any upside to doing that. The fewer calls we make here, the
> bet

The security guys always seem to want W xor X, but maybe not on Windows? I
thought that it was because having a page that was both at the same time
made it easier to abuse, but if it's xor, your attack's got to have spot-on
timing or else it'll just crash.


Line 589: //

> Yes, the injected stack will get the default stack size from the target's
> P

I just wasn't clear on whether NtCreateThreadEx() used the default or if
that was a higher-level CreateThread()/CreateRemoteThread() thing.

If there's no chance of running anyone else's code on our new thread's
stack, I'd say we should make it smaller to reduce the risk of running out
of address space. But if there's an avenue to run something else (via VEH?)
then we should probably leave it at the default.


Line 594: // call <address_of_RaiseException>

> How about ud2, since hlt is ring 0? Done. (Not in the x64 case, since
> there

Oh, and in the x86_64 case, RaiseException can still try to return, but
it'll jump to (a garbage address? the thing that our entry point would have
returned to?) I don't know if NtCreateThreadEx() sets things up so that the
entry point can return or whether it cares about the return value.


Line 594: // call <address_of_RaiseException>

> How about ud2, since hlt is ring 0? Done. (Not in the x64 case, since
> there

Right, hlt raises #gp. #gp vs. #ud, doesn't make much of a difference to
me. ud2's good, I just didn't want an errant return (from a debugger?) to
try executing garbage and make things worse.


Line 639: reinterpret_cast<void*>(inject_memory),

> Hmm, true. It is documented as being required per Remarks in
> https://msdn.m

We can leave it as you have it to humor Raymond, but he's way off base.


https://chromium-review.googlesource.com/#/c/339263/14/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 540: // jmp <address_of_RaiseException>
Take care with %rsp alignment on x86_64. (But if we found the stack
properly-aligned for function entry and we did a jmp instead of a call,
we're probably fine.)


Line 644: PLOG(ERROR) << "WriteProcessMemory";
The P in PLOG is wrong if you failed because bytes_written wasn't the
expected value.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 14

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 14:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/14/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 663: NTSTATUS status = NtCreateThreadEx(&thread_handle,
This isn't available pre-Vista, right?

That's fine for Chrome, but we were just talking to someone that still
wanted XP support.

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 16:

(8 comments)

Thanks!

https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client.h
File client/crashpad_client.h:

Line 169: //! instead of the exception referring to the location
where the exception

> You can leave it if you want, it was just something that struck me as
> maybe

OK, I think I'll keep it. It's not adding a lot of complexity on our side.

https://chromium-review.googlesource.com/#/c/339263/14/client/crashpad_client.h
File client/crashpad_client.h:

Line 165: //!

> In light of the stuff that you and Siggi were discussing about IDs not
> bein

Yes, there's the possibility of a race here.

I thought about it, but didn't thing the failure case was worth worrying
about though (no thread being blamed, or the very unlikely case of the
wrong thread being blamed).

If we add the time, then we just get no thread being blamed (always) at the
cost of a kind of weird API. Hmm.

OK, how about this? I switched the API to take a HANDLE. In the body of the
injection function, we suspend the process, and then get the thread_id to
pass on as before, but only if the thread hasn't already terminated. To
avoid a race between our crash thread crashing (which starts immediately)
and us letting the target go again and having the candidate thread exiting,
we wait on the injected thread termination. This also is a nicer API from
the client side I think because it means the dump is "done" at the point
the function returns.

The only failure case I can think of then is an adversary process that's
resuming threads underneath our nose, but at that point we're basically in
a huge mess so very-unlikely-wrong-thread-blame seems like the least of our
problems.

https://chromium-review.googlesource.com/#/c/339263/13/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 510: }

> The security guys always seem to want W xor X, but maybe not on Windows? I

OK. It seemed like the window was already quite small because we're going
to crash, suspend, and terminate the process in short order here anyway.

But I guess it's a but smaller that way.


Line 589:

> I just wasn't clear on whether NtCreateThreadEx() used the default or if
> th

AFAIK, all ways of creating a thread will get the default size. It's
undocumented of course. I just poked around to see what
DebugBreakProcess()'s call to NtCreateThreadEx() eventually does and it
passes.... 0x4000 (and 0 for zero_bits and maximum_stack_size). So I'll go
with that, somewhat arbitrarily.


Line 594:

> Oh, and in the x86_64 case, RaiseException can still try to return, but
> it'

Yes, it'll return to the kernel32 code that calls the user entry point
(BaseThreadInitSomethingOrOther) and the thread would exit.


https://chromium-review.googlesource.com/#/c/339263/14/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 540:
PAGE_READWRITE));

> Take care with %rsp alignment on x86_64. (But if we found the stack
> properl

Right, I think we're OK in the x64 case because we can assume we were
called correctly by the thread entry point, and we're not modifying the
stack here.


Line 644: // Push 1 for dwExceptionCode.

> The P in PLOG is wrong if you failed because bytes_written wasn't the
> expec

Done


Line 663:

> This isn't available pre-Vista, right?

Oh, good catch. I'm not going to write an alternate injection mechanism
now, but just added a LOG+fail for pre-Vista.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 16

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 16:

(9 comments)

This is pretty snazzy now.

https://chromium-review.googlesource.com/#/c/339263/16/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 527: if (thread_exit_code == STILL_ACTIVE) {
Ha, ambiguous!

Does |WaitForSingleObject(thread, 0) == WAIT_TIMEOUT| work? If it does,
it's more what we mean to do here, even if it means that the code looks
less like what we mean.


Line 530: thread_id = GetThreadId(thread);
thread_id = get_thread_id(thread);

otherwise, why did you just use GET_FUNCTION_REQUIRED?


Line 706: const size_t kStackSize = 0x4000;
(in-line OK) // This is what DebugBreakProcess() uses.


Line 707: NTSTATUS status = NtCreateThreadEx(&thread_handle,
Does the fact that this call is here prevent this code from even loading
pre-Vista? I thought that's why we used GET_FUNCTION(), and why you used
GET_FUNCTION_REQUIRED() for GetThreadId().


Line 728: if (!NT_SUCCESS(NtClose(thread_handle))) {
The NtClose() should happen even if WaitForSingleObject() failed, so either
use a scoper for the NtClose() or if you're sick of that, have a bool to
track the return value and get rid of the return above.


https://chromium-review.googlesource.com/#/c/339263/16/handler/win/loader_lock_dll.cc
File handler/win/loader_lock_dll.cc:

Line 17: // This program intentionally blocks in DllMain which is executed
with the
Good test.


Line 29:
Lose the blank line here.


https://chromium-review.googlesource.com/#/c/339263/16/snapshot/win/exception_snapshot_win.cc
File snapshot/win/exception_snapshot_win.cc:

Line 197: LOG(WARNING) << "thread not found";
"thread " << thread_id << " not found"

as a nice little extra tidbit for when we get crashpad_handler logs carried
in minidumps.


https://chromium-review.googlesource.com/#/c/339263/16/snapshot/win/exception_snapshot_win.h
File snapshot/win/exception_snapshot_win.h:

Line 100: void NativeContextToCPUContext64(const CONTEXT& context_record);
It looks like these could be static, and static + private = a good
candidate to move to the .cc's unnamed namespace. I see that you've got the
pointer-to-member thing going on up above, which must be why this is here.
But maybe that can just be a simple function pointer?

If you move this to the .cc, Context32 should be able to go with it.

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 17:

(8 comments)

Thanks

https://chromium-review.googlesource.com/#/c/339263/16/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 527:

> Ha, ambiguous!
Done


Line 530: reinterpret_cast<WinVMAddress>(VirtualAllocEx(process,
> thread_id = get_thread_id(thread);
Oh shoot, of course. Done.


Line 706: nullptr,

> (in-line OK) // This is what DebugBreakProcess() uses.

Done


Line 707:
THREAD_CREATE_FLAGS_SKIP_THREAD_ATTACH,

> Does the fact that this call is here prevent this code from even loading
> pr

This is calling crashpad::NtCreateThreadEx() in nt_internals.h. There's no
prototype or import library that contains ntdll!NtCreateThreadEx().


Line 728: return result;

> The NtClose() should happen even if WaitForSingleObject() failed, so
> either

Done


https://chromium-review.googlesource.com/#/c/339263/16/handler/win/loader_lock_dll.cc
File handler/win/loader_lock_dll.cc:

Line 29
> Lose the blank line here.

Done

https://chromium-review.googlesource.com/#/c/339263/16/snapshot/win/exception_snapshot_win.cc
File snapshot/win/exception_snapshot_win.cc:

Line 197:

> "thread " << thread_id << " not found"
Done

https://chromium-review.googlesource.com/#/c/339263/16/snapshot/win/exception_snapshot_win.h
File snapshot/win/exception_snapshot_win.h:

Line 100:

> It looks like these could be static, and static + private = a good
> candidat

Done. (I moved it out here because the union didn't have a name previously
and it seemed a bit of a weird prototype having to pass both the context
and the union... but anyway, you're right, better to keep it out of the
header.)

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 17

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 17: Code-Review+2

(4 comments)

https://chromium-review.googlesource.com/#/c/339263/17/client/crashpad_client.h
File client/crashpad_client.h:

Line 162: //! mechanism used.
Say something quickly about how this blocks.


Line 164: //! This function is unavailable when running on Windows XP.
and will return `false`.


https://chromium-review.googlesource.com/#/c/339263/16/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 707:
THREAD_CREATE_FLAGS_SKIP_THREAD_ATTACH,

> This is calling crashpad::NtCreateThreadEx() in nt_internals.h. There's no

Oh, right!


https://chromium-review.googlesource.com/#/c/339263/17/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 699: HANDLE thread_handle;
There are two thread handles in this function now. Less confusing would be
to give them descriptive names like blame_thread and injected_thread.

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 17:

Cool stuff. LGTM.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 17
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 18:

(3 comments)

Thanks for all the help with this one.

https://chromium-review.googlesource.com/#/c/339263/17/client/crashpad_client.h
File client/crashpad_client.h:

Line 162: //! mechanism used. This function will block until the
exception in the target

> Say something quickly about how this blocks.

Done


Line 164: //!

> and will return `false`.

Done


https://chromium-review.googlesource.com/#/c/339263/17/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 699: HANDLE injected_thread;

> There are two thread handles in this function now. Less confusing would be

Done

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 18

Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: Yes

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 18: Code-Review+2

(2 comments)

LGTM!

https://chromium-review.googlesource.com/#/c/339263/18/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 713: LOG(ERROR) << "NtCreateThreadEx";
NTSTATUS_LOG here and on line 724.


Line 719: LOG(ERROR) << "WaitForSingleObject";
PLOG.

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 19:

(2 comments)

https://chromium-review.googlesource.com/#/c/339263/18/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 713: if (!NT_SUCCESS(status)) {

> NTSTATUS_LOG here and on line 724.

Done


Line 719: if (WaitForSingleObject(injected_thread, 60 * 1000) !=
WAIT_OBJECT_0) {
> PLOG.
Done

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106

Gerrit-PatchSet: 19

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 19: Code-Review+2

We need to find out some "sticky LGTM" configuration setting.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 19
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Scott Graham (Gerrit)]

Scott Graham has submitted this change and it was merged.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


win: Support dumping another process by causing it to crash

Adds a new client API which allows causing an exception in another
process. This is accomplished by injecting a thread that calls
RaiseException(). A special exception code is used that indicates to the
handler that the exception arguments contain a thread id and exception
code, which are in turn used to fabricate an exception record. This is
so that the API can allow the client to "blame" a particular thread in
the target process.

The target process must also be a registered Crashpad client, as the
normal exception mechanism is used to handle the exception.

The injection of a thread is used instead of DebugBreakProcess() which
does not cause the UnhandledExceptionFilter() to be executed.
NtCreateThreadEx() is used in lieu of CreateRemoteThread() as it allows
passing of a flag which avoids calling DllMain()s. This is necessary to
allow thread creation to succeed even when the target process is
deadlocked on the loader lock.

BUG=crashpad:103

Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Reviewed-on: https://chromium-review.googlesource.com/339263
Reviewed-by: Mark Mentovai <ma...@chromium.org>
---
M client/crashpad_client.h
M client/crashpad_client_win.cc
M handler/handler.gyp
A handler/win/crash_other_program.cc
A handler/win/hanging_program.cc
A handler/win/loader_lock_dll.cc
M snapshot/win/end_to_end_test.py
M snapshot/win/exception_snapshot_win.cc
M snapshot/win/exception_snapshot_win.h
M snapshot/win/process_snapshot_win.cc
M util/win/nt_internals.cc
M util/win/nt_internals.h
12 files changed, 782 insertions(+), 44 deletions(-)

Approvals:
Mark Mentovai: Looks good to me, approved



diff --git a/client/crashpad_client.h b/client/crashpad_client.h
index 28a87a3..61338a3 100644
--- a/client/crashpad_client.h
+++ b/client/crashpad_client.h
@@ -19,6 +19,8 @@
#include <string>
#include <vector>

+#include <stdint.h>
+
#include "base/files/file_path.h"
#include "base/macros.h"
#include "build/build_config.h"
@@ -152,6 +154,41 @@
//! \param[in] exception_pointers An `EXCEPTION_POINTERS`, as would
generally
//! passed to an unhandled exception filter.
static void DumpAndCrash(EXCEPTION_POINTERS* exception_pointers);
+
+ //! \brief Requests that the handler capture a dump of a different
process.
+ //!
+ //! The target process must be an already-registered Crashpad client. An
+ //! exception will be triggered in the target process, and the regular
dump
+ //! mechanism used. This function will block until the exception in the
target
+ //! process has been handled by the Crashpad handler.
+ //!
+ //! This function is unavailable when running on Windows XP and will
return
+ //! `false`.
+ //!
+ //! \param[in] process A `HANDLE` identifying the process to be dumped.
+ //! \param[in] blame_thread If non-null, a `HANDLE` valid in the caller's
+ //! process, referring to a thread in the target process. If this is
+ //! supplied, instead of the exception referring to the location
where the
+ //! exception was injected, an exception record will be fabricated
that
+ //! refers to the current location of the given thread.
+ //! \param[in] exception_code If \a blame_thread is non-null, this will
be
+ //! used as the exception code in the exception record.
+ //!
+ //! \return `true` if the exception was triggered successfully.
+ bool DumpAndCrashTargetProcess(HANDLE process,
+ HANDLE blame_thread,
+ DWORD exception_code) const;
+
+ enum : uint32_t {
+ //! \brief The exception code (roughly "Client called") used when
+ //! DumpAndCrashTargetProcess() triggers an exception in a target
+ //! process.
+ //!
+ //! \note This value does not have any bits of the top nibble set, to
avoid
+ //! confusion with real exception codes which tend to have those
bits
+ //! set.
+ kTriggeredExceptionCode = 0xcca11ed,
+ };
#endif

//! \brief Configures the process to direct its crashes to a Crashpad
handler.
diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc
index 1f10686..b0fca53 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -27,12 +27,17 @@
#include "base/strings/utf_string_conversions.h"
#include "base/synchronization/lock.h"
#include "util/file/file_io.h"
+#include "util/win/address_types.h"
#include "util/win/command_line.h"
#include "util/win/critical_section_with_debug_info.h"
#include "util/win/get_function.h"
#include "util/win/handle.h"
+#include "util/win/nt_internals.h"
+#include "util/win/ntstatus_logging.h"
+#include "util/win/process_info.h"
#include "util/win/registration_protocol_win.h"
#include "util/win/scoped_handle.h"
+#include "util/win/scoped_process_suspend.h"

namespace {

@@ -165,6 +170,19 @@
handle_list->end()) {
handle_list->push_back(handle);
}
+}
+
+void AddUint32(std::vector<unsigned char>* data_vector, uint32_t data) {
+ data_vector->push_back(static_cast<unsigned char>(data & 0xff));
+ data_vector->push_back(static_cast<unsigned char>((data & 0xff00) >> 8));
+ data_vector->push_back(static_cast<unsigned char>((data & 0xff0000) >>
16));
+ data_vector->push_back(static_cast<unsigned char>((data & 0xff000000) >>
24));
+}
+
+void AddUint64(std::vector<unsigned char>* data_vector, uint64_t data) {
+ AddUint32(data_vector, static_cast<uint32_t>(data & 0xffffffffULL));
+ AddUint32(data_vector,
+ static_cast<uint32_t>((data & 0xffffffff00000000ULL) >> 32));
}

} // namespace
@@ -469,4 +487,247 @@
UnhandledExceptionHandler(exception_pointers);
}

+bool CrashpadClient::DumpAndCrashTargetProcess(HANDLE process,
+ HANDLE blame_thread,
+ DWORD exception_code) const
{
+ // Confirm we're on Vista or later.
+ const DWORD version = GetVersion();
+ const DWORD major_version = LOBYTE(LOWORD(version));
+ if (major_version < 6) {
+ LOG(ERROR) << "unavailable before Vista";
+ return false;
+ }
+
+ // Confirm that our bitness is the same as the process we're crashing.
+ ProcessInfo process_info;
+ if (!process_info.Initialize(process)) {
+ LOG(ERROR) << "ProcessInfo::Initialize";
+ return false;
+ }
+#if defined(ARCH_CPU_64_BITS)
+ if (!process_info.Is64Bit()) {
+ LOG(ERROR) << "DumpAndCrashTargetProcess currently not supported
x64->x86";
+ return false;
+ }
+#endif // ARCH_CPU_64_BITS
+
+ ScopedProcessSuspend suspend(process);
+
+ // If no thread handle was provided, or the thread has already exited,
we pass
+ // 0 to the handler, which indicates no fake exception record to be
created.
+ DWORD thread_id = 0;
+ if (blame_thread) {
+ // Now that we've suspended the process, if our thread hasn't exited,
we
+ // know we're relatively safe to pass the thread id through.
+ if (WaitForSingleObject(blame_thread, 0) == WAIT_TIMEOUT) {
+ static const auto get_thread_id =
+ GET_FUNCTION_REQUIRED(L"kernel32.dll", ::GetThreadId);
+ thread_id = get_thread_id(blame_thread);
+ }
+ }
+
+ const size_t kInjectBufferSize = 4 * 1024;
+ WinVMAddress inject_memory =
+ reinterpret_cast<WinVMAddress>(VirtualAllocEx(process,
+ nullptr,
+ kInjectBufferSize,
+ MEM_RESERVE |
MEM_COMMIT,
+ PAGE_READWRITE));
+ if (!inject_memory) {
+ PLOG(ERROR) << "VirtualAllocEx";
+ return false;
+ }
+
+ // Because we're the same bitness as our target, we can rely kernel32
being
+ // loaded at the same address in our process as the target, and just
look up
+ // its address here.
+ WinVMAddress raise_exception_address =
+ reinterpret_cast<WinVMAddress>(&RaiseException);
+
+ WinVMAddress code_entry_point = 0;
+ std::vector<unsigned char> data_to_write;
+ if (process_info.Is64Bit()) {
+ // Data written is first, the data for the 4th argument (lpArguments)
to
+ // RaiseException(). A two element array:
+ //
+ // DWORD64: thread_id
+ // DWORD64: exception_code
+ //
+ // Following that, code which sets the arguments to RaiseException()
and
+ // then calls it:
+ //
+ // mov r9, <data_array_address>
+ // mov r8d, 2 ; nNumberOfArguments
+ // mov edx, 1 ; dwExceptionFlags = EXCEPTION_NONCONTINUABLE
+ // mov ecx, 0xcca11ed ; dwExceptionCode, interpreted specially by the
+ // ; handler.
+ // jmp <address_of_RaiseException>
+ //
+ // Note that the first three arguments to RaiseException() are DWORDs
even
+ // on x64, so only the 4th argument (a pointer) is a full-width
register.
+ //
+ // We also don't need to set up a stack or use call, since the only
+ // registers modified are volatile ones, and we can just jmp straight
to
+ // RaiseException().
+
+ // The data array.
+ AddUint64(&data_to_write, thread_id);
+ AddUint64(&data_to_write, exception_code);
+
+ // The thread entry point.
+ code_entry_point = inject_memory + data_to_write.size();
+
+ // r9 = pointer to data.
+ data_to_write.push_back(0x49);
+ data_to_write.push_back(0xb9);
+ AddUint64(&data_to_write, inject_memory);
+
+ // r8d = 2 for nNumberOfArguments.
+ data_to_write.push_back(0x41);
+ data_to_write.push_back(0xb8);
+ AddUint32(&data_to_write, 2);
+
+ // edx = 1 for dwExceptionFlags.
+ data_to_write.push_back(0xba);
+ AddUint32(&data_to_write, 1);
+
+ // ecx = kTriggeredExceptionCode for dwExceptionCode.
+ data_to_write.push_back(0xb9);
+ AddUint32(&data_to_write, kTriggeredExceptionCode);
+
+ // jmp to RaiseException() via rax.
+ data_to_write.push_back(0x48); // mov rax, imm.
+ data_to_write.push_back(0xb8);
+ AddUint64(&data_to_write, raise_exception_address);
+ data_to_write.push_back(0xff); // jmp rax.
+ data_to_write.push_back(0xe0);
+ } else {
+ // Data written is first, the data for the 4th argument (lpArguments)
to
+ // RaiseException(). A two element array:
+ //
+ // DWORD: thread_id
+ // DWORD: exception_code
+ //
+ // Following that, code which pushes our arguments to RaiseException()
and
+ // then calls it:
+ //
+ // push <data_array_address>
+ // push 2 ; nNumberOfArguments
+ // push 1 ; dwExceptionFlags = EXCEPTION_NONCONTINUABLE
+ // push 0xcca11ed ; dwExceptionCode, interpreted specially by the
handler.
+ // call <address_of_RaiseException>
+ // ud2 ; Generate invalid opcode to make sure we still crash if we
return
+ // ; for some reason.
+ //
+ // No need to clean up the stack, as RaiseException() is __stdcall.
+
+ // The data array.
+ AddUint32(&data_to_write, thread_id);
+ AddUint32(&data_to_write, exception_code);
+
+ // The thread entry point.
+ code_entry_point = inject_memory + data_to_write.size();
+
+ // Push data address.
+ data_to_write.push_back(0x68);
+ AddUint32(&data_to_write, static_cast<uint32_t>(inject_memory));
+
+ // Push 2 for nNumberOfArguments.
+ data_to_write.push_back(0x6a);
+ data_to_write.push_back(2);
+
+ // Push 1 for dwExceptionCode.
+ data_to_write.push_back(0x6a);
+ data_to_write.push_back(1);
+
+ // Push dwExceptionFlags.
+ data_to_write.push_back(0x68);
+ AddUint32(&data_to_write, kTriggeredExceptionCode);
+
+ // Relative call to RaiseException().
+ int64_t relative_address_to_raise_exception =
+ raise_exception_address - (inject_memory + data_to_write.size() +
5);
+ data_to_write.push_back(0xe8);
+ AddUint32(&data_to_write,
+ static_cast<uint32_t>(relative_address_to_raise_exception));
+
+ // ud2.
+ data_to_write.push_back(0x0f);
+ data_to_write.push_back(0x0b);
+ }
+
+ DCHECK_LT(data_to_write.size(), kInjectBufferSize);
+
+ SIZE_T bytes_written;
+ if (!WriteProcessMemory(process,
+ reinterpret_cast<void*>(inject_memory),
+ data_to_write.data(),
+ data_to_write.size(),
+ &bytes_written)) {
+ PLOG(ERROR) << "WriteProcessMemory";
+ return false;
+ }
+
+ if (bytes_written != data_to_write.size()) {
+ LOG(ERROR) << "WriteProcessMemory unexpected number of bytes";
+ return false;
+ }
+
+ if (!FlushInstructionCache(
+ process, reinterpret_cast<void*>(inject_memory), bytes_written))
{
+ PLOG(ERROR) << "FlushInstructionCache";
+ return false;
+ }
+
+ DWORD old_protect;
+ if (!VirtualProtectEx(process,
+ reinterpret_cast<void*>(inject_memory),
+ kInjectBufferSize,
+ PAGE_EXECUTE_READ,
+ &old_protect)) {
+ PLOG(ERROR) << "VirtualProtectEx";
+ return false;
+ }
+
+ // Cause an exception in the target process by creating a thread which
calls
+ // RaiseException with our arguments above. Note that we cannot get away
with
+ // using DebugBreakProcess() (nothing happens unless a debugger is
attached)
+ // and we cannot get away with CreateRemoteThread() because it doesn't
work if
+ // the target is hung waiting for the loader lock. We use
NtCreateThreadEx()
+ // with the SKIP_THREAD_ATTACH flag, which skips various notifications,
+ // letting this cause an exception, even when the target is stuck in the
+ // loader lock.
+ HANDLE injected_thread;
+ const size_t kStackSize = 0x4000; // This is what DebugBreakProcess()
uses.
+ NTSTATUS status = NtCreateThreadEx(&injected_thread,
+ STANDARD_RIGHTS_ALL |
SPECIFIC_RIGHTS_ALL,
+ nullptr,
+ process,
+
reinterpret_cast<void*>(code_entry_point),
+ nullptr,
+
THREAD_CREATE_FLAGS_SKIP_THREAD_ATTACH,
+ 0,
+ kStackSize,
+ 0,
+ nullptr);
+ if (!NT_SUCCESS(status)) {
+ NTSTATUS_LOG(ERROR, status) << "NtCreateThreadEx";
+ return false;
+ }
+
+ bool result = true;
+ if (WaitForSingleObject(injected_thread, 60 * 1000) != WAIT_OBJECT_0) {
+ PLOG(ERROR) << "WaitForSingleObject";
+ result = false;
+ }
+
+ status = NtClose(injected_thread);
+ if (!NT_SUCCESS(status)) {
+ NTSTATUS_LOG(ERROR, status) << "NtClose";
+ result = false;
+ }
+
+ return result;
+}
+
} // namespace crashpad
diff --git a/handler/handler.gyp b/handler/handler.gyp
index 40d5977..bbb2ce4 100644
--- a/handler/handler.gyp
+++ b/handler/handler.gyp
@@ -111,6 +111,40 @@
],
},
{
+ 'target_name': 'crash_other_program',
+ 'type': 'executable',
+ 'dependencies': [
+ '../client/client.gyp:crashpad_client',
+ '../test/test.gyp:crashpad_test',
+ '../third_party/mini_chromium/mini_chromium.gyp:base',
+ '../util/util.gyp:crashpad_util',
+ ],
+ 'sources': [
+ 'win/crash_other_program.cc',
+ ],
+ },
+ {
+ 'target_name': 'hanging_program',
+ 'type': 'executable',
+ 'dependencies': [
+ '../client/client.gyp:crashpad_client',
+ '../third_party/mini_chromium/mini_chromium.gyp:base',
+ ],
+ 'sources': [
+ 'win/hanging_program.cc',
+ ],
+ },
+ {
+ 'target_name': 'loader_lock_dll',
+ 'type': 'loadable_module',
+ 'sources': [
+ 'win/loader_lock_dll.cc',
+ ],
+ 'msvs_settings': {
+ 'NoImportLibrary': 'true',
+ },
+ },
+ {
'target_name': 'self_destroying_program',
'type': 'executable',
'dependencies': [
diff --git a/handler/win/crash_other_program.cc
b/handler/win/crash_other_program.cc
new file mode 100644
index 0000000..db532bb
--- /dev/null
+++ b/handler/win/crash_other_program.cc
@@ -0,0 +1,120 @@
+// Copyright 2016 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <stdlib.h>
+#include <windows.h>
+#include <tlhelp32.h>
+
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "client/crashpad_client.h"
+#include "test/paths.h"
+#include "test/win/child_launcher.h"
+#include "util/file/file_io.h"
+#include "util/win/scoped_handle.h"
+#include "util/win/xp_compat.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+bool CrashAndDumpTarget(const CrashpadClient& client, HANDLE process) {
+ DWORD target_pid = GetProcessId(process);
+
+ HANDLE thread_snap_raw = CreateToolhelp32Snapshot(TH32CS_SNAPTHREAD, 0);
+ if (thread_snap_raw == INVALID_HANDLE_VALUE) {
+ LOG(ERROR) << "CreateToolhelp32Snapshot";
+ return false;
+ }
+ ScopedFileHANDLE thread_snap(thread_snap_raw);
+
+ THREADENTRY32 te32;
+ te32.dwSize = sizeof(THREADENTRY32);
+ if (!Thread32First(thread_snap.get(), &te32)) {
+ LOG(ERROR) << "Thread32First";
+ return false;
+ }
+
+ int thread_count = 0;
+ do {
+ if (te32.th32OwnerProcessID == target_pid) {
+ thread_count++;
+ if (thread_count == 2) {
+ // Nominate this lucky thread as our blamee, and dump it. This
will be
+ // "Thread1" in the child.
+ ScopedKernelHANDLE thread(
+ OpenThread(kXPThreadAllAccess, false, te32.th32ThreadID));
+ if (!client.DumpAndCrashTargetProcess(
+ process, thread.get(), 0xdeadbea7)) {
+ LOG(ERROR) << "DumpAndCrashTargetProcess failed";
+ return false;
+ }
+ return true;
+ }
+ }
+ } while (Thread32Next(thread_snap.get(), &te32));
+
+ return false;
+}
+
+int CrashOtherProgram(int argc, wchar_t* argv[]) {
+ CrashpadClient client;
+
+ if (argc == 2 || argc == 3) {
+ if (!client.SetHandlerIPCPipe(argv[1])) {
+ LOG(ERROR) << "SetHandler";
+ return EXIT_FAILURE;
+ }
+ } else {
+ fprintf(stderr, "Usage: %ls <server_pipe_name> [noexception]\n",
argv[0]);
+ return EXIT_FAILURE;
+ }
+
+ if (!client.UseHandler()) {
+ LOG(ERROR) << "UseHandler";
+ return EXIT_FAILURE;
+ }
+
+ // Launch another process that hangs.
+ base::FilePath test_executable = Paths::Executable();
+ std::wstring child_test_executable =
+ test_executable.DirName().Append(L"hanging_program.exe").value();
+ ChildLauncher child(child_test_executable, argv[1]);
+ child.Start();
+
+ // Wait until it's ready.
+ char c;
+ if (!LoggingReadFile(child.stdout_read_handle(), &c, sizeof(c)) ||
c != ' ') {
+ LOG(ERROR) << "failed child communication";
+ return EXIT_FAILURE;
+ }
+
+ if (argc == 3 && wcscmp(argv[2], L"noexception") == 0) {
+ client.DumpAndCrashTargetProcess(child.process_handle(), 0, 0);
+ return EXIT_SUCCESS;
+ } else {
+ if (CrashAndDumpTarget(client, child.process_handle()))
+ return EXIT_SUCCESS;
+ }
+
+ return EXIT_FAILURE;
+}
+
+} // namespace
+} // namespace test
+} // namespace crashpad
+
+int wmain(int argc, wchar_t* argv[]) {
+ return crashpad::test::CrashOtherProgram(argc, argv);
+}
diff --git a/handler/win/hanging_program.cc b/handler/win/hanging_program.cc
new file mode 100644
index 0000000..877578e
--- /dev/null
+++ b/handler/win/hanging_program.cc
@@ -0,0 +1,86 @@
+// Copyright 2016 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <stdio.h>
+#include <windows.h>
+
+#include "base/debug/alias.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "client/crashpad_client.h"
+#include "client/crashpad_info.h"
+
+DWORD WINAPI Thread1(LPVOID dummy) {
+ Sleep(INFINITE);
+ return 0;
+}
+
+DWORD WINAPI Thread2(LPVOID dummy) {
+ Sleep(INFINITE);
+ return 0;
+}
+
+DWORD WINAPI Thread3(LPVOID dummy) {
+ HMODULE dll = LoadLibrary(L"loader_lock_dll.dll");
+ if (!dll)
+ PLOG(ERROR) << "LoadLibrary";
+
+ // This call is not expected to return.
+ if (!FreeLibrary(dll))
+ PLOG(ERROR) << "FreeLibrary";
+
+ return 0;
+}
+
+int wmain(int argc, wchar_t* argv[]) {
+ crashpad::CrashpadClient client;
+
+ if (argc == 2) {
+ if (!client.SetHandlerIPCPipe(argv[1])) {
+ LOG(ERROR) << "SetHandler";
+ return EXIT_FAILURE;
+ }
+ } else {
+ fprintf(stderr, "Usage: %ls <server_pipe_name>\n", argv[0]);
+ return EXIT_FAILURE;
+ }
+
+ if (!client.UseHandler()) {
+ LOG(ERROR) << "UseHandler";
+ return EXIT_FAILURE;
+ }
+
+ // Make sure this module has a CrashpadInfo structure.
+ crashpad::CrashpadInfo* crashpad_info =
+ crashpad::CrashpadInfo::GetCrashpadInfo();
+ base::debug::Alias(crashpad_info);
+
+ HANDLE threads[3];
+ threads[0] = CreateThread(nullptr, 0, Thread1, nullptr, 0, nullptr);
+ threads[1] = CreateThread(nullptr, 0, Thread2, nullptr, 0, nullptr);
+ threads[2] = CreateThread(nullptr, 0, Thread3, nullptr, 0, nullptr);
+
+ // Our whole process is going to hang when the loaded DLL hangs in its
+ // DllMain(), so we can't signal to our parent that we're "ready". So,
use a
+ // hokey delay of 1s after we spawn the threads, and hope that we make
it to
+ // the FreeLibrary call by then.
+ Sleep(1000);
+
+ fprintf(stdout, " ");
+ fflush(stdout);
+
+ WaitForMultipleObjects(ARRAYSIZE(threads), threads, true, INFINITE);
+
+ return 0;
+}
diff --git a/handler/win/loader_lock_dll.cc b/handler/win/loader_lock_dll.cc
new file mode 100644
index 0000000..0a05af3
--- /dev/null
+++ b/handler/win/loader_lock_dll.cc
@@ -0,0 +1,28 @@
+// Copyright 2016 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <windows.h>
+
+// This program intentionally blocks in DllMain which is executed with the
+// loader lock locked. This allows us to test that
+// CrashpadClient::DumpAndCrashTargetProcess() can still dump the target
in this
+// case.
+BOOL WINAPI DllMain(HINSTANCE, DWORD reason, LPVOID) {
+ switch (reason) {
+ case DLL_PROCESS_DETACH:
+ case DLL_THREAD_DETACH:
+ Sleep(INFINITE);
+ }
+ return TRUE;
+}
diff --git a/snapshot/win/end_to_end_test.py
b/snapshot/win/end_to_end_test.py
index 53d1b07..a5540e5 100755
--- a/snapshot/win/end_to_end_test.py
+++ b/snapshot/win/end_to_end_test.py
@@ -79,11 +79,12 @@
return None


-def GetDumpFromProgram(out_dir, pipe_name, executable_name):
+def GetDumpFromProgram(out_dir, pipe_name, executable_name, *args):
"""Initialize a crash database, and run |executable_name| connecting to a
crash handler. If pipe_name is set, crashpad_handler will be started
first. If
pipe_name is empty, the executable is responsible for starting
- crashpad_handler. Returns the minidump generated by crashpad_handler for
+ crashpad_handler. *args will be passed after other arguments to
+ executable_name. Returns the minidump generated by crashpad_handler for
further testing.
"""
test_database = MakeTempDir()
@@ -111,11 +112,12 @@
printed = True
time.sleep(0.1)

- subprocess.call([os.path.join(out_dir, executable_name), pipe_name])
+ subprocess.call([os.path.join(out_dir, executable_name), pipe_name] +
+ list(args))
else:
subprocess.call([os.path.join(out_dir, executable_name),
os.path.join(out_dir, 'crashpad_handler.exe'),
- test_database])
+ test_database] + list(args))

out = subprocess.check_output([
os.path.join(out_dir, 'crashpad_database_util.exe'),
@@ -133,6 +135,11 @@

def GetDumpFromCrashyProgram(out_dir, pipe_name):
return GetDumpFromProgram(out_dir, pipe_name, 'crashy_program.exe')
+
+
+def GetDumpFromOtherProgram(out_dir, pipe_name, *args):
+ return GetDumpFromProgram(out_dir, pipe_name, 'crash_other_program.exe',
+ *args)


def GetDumpFromSelfDestroyingProgram(out_dir, pipe_name):
@@ -182,6 +189,8 @@
start_handler_dump_path,
destroyed_dump_path,
z7_dump_path,
+ other_program_path,
+ other_program_no_exception_path,
pipe_name):
"""Runs various tests in sequence. Runs a new cdb instance on the dump
for
each block of tests to reduce the chances that output from one command is
@@ -311,6 +320,18 @@
out.Check(r'z7_test C \(codeview symbols\) z7_test.dll',
'expected non-pdb symbol format')

+ out = CdbRun(cdb_path, other_program_path, '.ecxr;k;~')
+ out.Check('Unknown exception - code deadbea7',
+ 'other program dump exception code')
+ out.Check('!Sleep', 'other program reasonable location')
+ out.Check('hanging_program!Thread1', 'other program dump right thread')
+ out.Check('\. 1 Id', 'other program exception on correct thread')
+
+ out = CdbRun(cdb_path, other_program_no_exception_path, '.ecxr;k')
+ out.Check('Unknown exception - code 0cca11ed',
+ 'other program with no exception given')
+ out.Check('!RaiseException', 'other program in RaiseException()')
+

def main(args):
try:
@@ -352,11 +373,22 @@
if not z7_dump_path:
return 1

+ other_program_path = GetDumpFromOtherProgram(args[0], pipe_name)
+ if not other_program_path:
+ return 1
+
+ other_program_no_exception_path = GetDumpFromOtherProgram(
+ args[0], pipe_name, 'noexception')
+ if not other_program_no_exception_path:
+ return 1
+
RunTests(cdb_path,
crashy_dump_path,
start_handler_dump_path,
destroyed_dump_path,
z7_dump_path,
+ other_program_path,
+ other_program_no_exception_path,
pipe_name)

return 0
diff --git a/snapshot/win/exception_snapshot_win.cc
b/snapshot/win/exception_snapshot_win.cc
index 2abe6d0..ca29e1f 100644
--- a/snapshot/win/exception_snapshot_win.cc
+++ b/snapshot/win/exception_snapshot_win.cc
@@ -14,6 +14,7 @@

#include "snapshot/win/exception_snapshot_win.h"

+#include "client/crashpad_client.h"
#include "snapshot/capture_memory.h"
#include "snapshot/memory_snapshot.h"
#include "snapshot/win/cpu_context_win.h"
@@ -24,6 +25,34 @@

namespace crashpad {
namespace internal {
+
+namespace {
+
+#if defined(ARCH_CPU_32_BITS)
+using Context32 = CONTEXT;
+#elif defined(ARCH_CPU_64_BITS)
+using Context32 = WOW64_CONTEXT;
+#endif
+
+#if defined(ARCH_CPU_64_BITS)
+void NativeContextToCPUContext64(const CONTEXT& context_record,
+ CPUContext* context,
+ CPUContextUnion* context_union) {
+ context->architecture = kCPUArchitectureX86_64;
+ context->x86_64 = &context_union->x86_64;
+ InitializeX64Context(context_record, context->x86_64);
+}
+#endif
+
+void NativeContextToCPUContext32(const Context32& context_record,
+ CPUContext* context,
+ CPUContextUnion* context_union) {
+ context->architecture = kCPUArchitectureX86;
+ context->x86 = &context_union->x86;
+ InitializeX86Context(context_record, context->x86);
+}
+
+} // namespace

ExceptionSnapshotWin::ExceptionSnapshotWin()
: ExceptionSnapshot(),
@@ -40,9 +69,11 @@
ExceptionSnapshotWin::~ExceptionSnapshotWin() {
}

-bool ExceptionSnapshotWin::Initialize(ProcessReaderWin* process_reader,
- DWORD thread_id,
- WinVMAddress
exception_pointers_address) {
+bool ExceptionSnapshotWin::Initialize(
+ ProcessReaderWin* process_reader,
+ DWORD thread_id,
+ WinVMAddress exception_pointers_address,
+ const PointerVector<internal::ThreadSnapshotWin>& threads) {
INITIALIZATION_STATE_SET_INITIALIZING(initialized_);

const ProcessReaderWin::Thread* thread = nullptr;
@@ -62,32 +93,28 @@

#if defined(ARCH_CPU_32_BITS)
const bool is_64_bit = false;
- using Context32 = CONTEXT;
#elif defined(ARCH_CPU_64_BITS)
const bool is_64_bit = process_reader->Is64Bit();
- using Context32 = WOW64_CONTEXT;
if (is_64_bit) {
- CONTEXT context_record;
if (!InitializeFromExceptionPointers<EXCEPTION_RECORD64,

process_types::EXCEPTION_POINTERS64>(
- *process_reader, exception_pointers_address, &context_record))
{
+ *process_reader,
+ exception_pointers_address,
+ threads,
+ &NativeContextToCPUContext64)) {
return false;
}
- context_.architecture = kCPUArchitectureX86_64;
- context_.x86_64 = &context_union_.x86_64;
- InitializeX64Context(context_record, context_.x86_64);
}
#endif
if (!is_64_bit) {
- Context32 context_record;
if (!InitializeFromExceptionPointers<EXCEPTION_RECORD32,

process_types::EXCEPTION_POINTERS32>(
- *process_reader, exception_pointers_address, &context_record))
{
+ *process_reader,
+ exception_pointers_address,
+ threads,
+ &NativeContextToCPUContext32)) {
return false;
}
- context_.architecture = kCPUArchitectureX86;
- context_.x86 = &context_union_.x86;
- InitializeX86Context(context_record, context_.x86);
}

CaptureMemoryDelegateWin capture_memory_delegate(
@@ -143,7 +170,10 @@
bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
const ProcessReaderWin& process_reader,
WinVMAddress exception_pointers_address,
- ContextType* context_record) {
+ const PointerVector<internal::ThreadSnapshotWin>& threads,
+ void (*native_to_cpu_context)(const ContextType& context_record,
+ CPUContext* context,
+ CPUContextUnion* context_union)) {
ExceptionPointersType exception_pointers;
if (!process_reader.ReadMemory(exception_pointers_address,
sizeof(exception_pointers),
@@ -164,22 +194,60 @@
LOG(ERROR) << "ExceptionRecord";
return false;
}
- exception_code_ = first_record.ExceptionCode;
- exception_flags_ = first_record.ExceptionFlags;
- exception_address_ = first_record.ExceptionAddress;
- for (DWORD i = 0; i < first_record.NumberParameters; ++i)
- codes_.push_back(first_record.ExceptionInformation[i]);
- if (first_record.ExceptionRecord) {
- // https://crashpad.chromium.org/bug/43
- LOG(WARNING) << "dropping chained ExceptionRecord";
- }

- if (!process_reader.ReadMemory(
- static_cast<WinVMAddress>(exception_pointers.ContextRecord),
- sizeof(*context_record),
- context_record)) {
- LOG(ERROR) << "ContextRecord";
- return false;
+ if (first_record.ExceptionCode ==
CrashpadClient::kTriggeredExceptionCode &&
+ first_record.NumberParameters == 2 &&
+ first_record.ExceptionInformation[0] != 0) {
+ // This special exception code indicates that the target was crashed by
+ // another client calling CrashpadClient::DumpAndCrashTargetProcess().
In
+ // this case the parameters are a thread id and an exception code
which we
+ // use to fabricate a new exception record.
+ using ArgumentType = decltype(first_record.ExceptionInformation[0]);
+ const ArgumentType thread_id = first_record.ExceptionInformation[0];
+ exception_code_ =
static_cast<DWORD>(first_record.ExceptionInformation[1]);
+ exception_flags_ = EXCEPTION_NONCONTINUABLE;
+ for (const auto* thread : threads) {
+ if (thread->ThreadID() == thread_id) {
+ thread_id_ = thread_id;
+ exception_address_ = thread->Context()->InstructionPointer();
+ context_.architecture = thread->Context()->architecture;
+ if (context_.architecture == kCPUArchitectureX86_64) {
+ context_union_.x86_64 = *thread->Context()->x86_64;
+ context_.x86_64 = &context_union_.x86_64;
+ } else {
+ context_union_.x86 = *thread->Context()->x86;
+ context_.x86 = &context_union_.x86;
+ }
+ break;
+ }
+ }
+
+ if (exception_address_ == 0) {
+ LOG(WARNING) << "thread " << thread_id << " not found";
+ return false;
+ }
+ } else {
+ // Normal case.
+ exception_code_ = first_record.ExceptionCode;
+ exception_flags_ = first_record.ExceptionFlags;
+ exception_address_ = first_record.ExceptionAddress;
+ for (DWORD i = 0; i < first_record.NumberParameters; ++i)
+ codes_.push_back(first_record.ExceptionInformation[i]);
+ if (first_record.ExceptionRecord) {
+ // https://crashpad.chromium.org/bug/43
+ LOG(WARNING) << "dropping chained ExceptionRecord";
+ }
+
+ ContextType context_record;
+ if (!process_reader.ReadMemory(
+ static_cast<WinVMAddress>(exception_pointers.ContextRecord),
+ sizeof(context_record),
+ &context_record)) {
+ LOG(ERROR) << "ContextRecord";
+ return false;
+ }
+
+ native_to_cpu_context(context_record, &context_, &context_union_);
}

return true;
diff --git a/snapshot/win/exception_snapshot_win.h
b/snapshot/win/exception_snapshot_win.h
index 3e66dd2..8ff7d38 100644
--- a/snapshot/win/exception_snapshot_win.h
+++ b/snapshot/win/exception_snapshot_win.h
@@ -22,6 +22,7 @@
#include "build/build_config.h"
#include "snapshot/cpu_context.h"
#include "snapshot/exception_snapshot.h"
+#include "snapshot/win/thread_snapshot_win.h"
#include "util/misc/initialization_state_dcheck.h"
#include "util/stdlib/pointer_container.h"
#include "util/win/address_types.h"
@@ -34,6 +35,13 @@
namespace internal {

class MemorySnapshotWin;
+
+#if defined(ARCH_CPU_X86_FAMILY)
+union CPUContextUnion {
+ CPUContextX86 x86;
+ CPUContextX86_64 x86_64;
+};
+#endif

class ExceptionSnapshotWin final : public ExceptionSnapshot {
public:
@@ -53,7 +61,8 @@
//! an appropriate message logged.
bool Initialize(ProcessReaderWin* process_reader,
DWORD thread_id,
- WinVMAddress exception_pointers);
+ WinVMAddress exception_pointers,
+ const PointerVector<internal::ThreadSnapshotWin>&
threads);

// ExceptionSnapshot:

@@ -69,15 +78,16 @@
template <class ExceptionRecordType,
class ExceptionPointersType,
class ContextType>
- bool InitializeFromExceptionPointers(const ProcessReaderWin&
process_reader,
- WinVMAddress
exception_pointers_address,
- ContextType* context_record);
+ bool InitializeFromExceptionPointers(
+ const ProcessReaderWin& process_reader,
+ WinVMAddress exception_pointers_address,
+ const PointerVector<internal::ThreadSnapshotWin>& threads,
+ void (*native_to_cpu_context)(const ContextType& context_record,
+ CPUContext* context,
+ CPUContextUnion* context_union));

#if defined(ARCH_CPU_X86_FAMILY)
- union {
- CPUContextX86 x86;
- CPUContextX86_64 x86_64;
- } context_union_;
+ CPUContextUnion context_union_;
#endif
CPUContext context_;
std::vector<uint64_t> codes_;
diff --git a/snapshot/win/process_snapshot_win.cc
b/snapshot/win/process_snapshot_win.cc
index 6af6f20..888a3e0 100644
--- a/snapshot/win/process_snapshot_win.cc
+++ b/snapshot/win/process_snapshot_win.cc
@@ -107,7 +107,8 @@
exception_.reset(new internal::ExceptionSnapshotWin());
if (!exception_->Initialize(&process_reader_,
exception_information.thread_id,
- exception_information.exception_pointers)) {
+ exception_information.exception_pointers,
+ threads_)) {
exception_.reset();
return false;
}
diff --git a/util/win/nt_internals.cc b/util/win/nt_internals.cc
index 6fc0999..8cf96a4 100644
--- a/util/win/nt_internals.cc
+++ b/util/win/nt_internals.cc
@@ -21,6 +21,18 @@

struct CLIENT_ID;

+NTSTATUS NTAPI NtCreateThreadEx(PHANDLE ThreadHandle,
+ ACCESS_MASK DesiredAccess,
+ POBJECT_ATTRIBUTES ObjectAttributes,
+ HANDLE ProcessHandle,
+ PVOID StartRoutine,
+ PVOID Argument,
+ ULONG CreateFlags,
+ SIZE_T ZeroBits,
+ SIZE_T StackSize,
+ SIZE_T MaximumStackSize,
+ PVOID /*PPS_ATTRIBUTE_LIST*/
AttributeList);
+
NTSTATUS NTAPI NtOpenThread(HANDLE* ThreadHandle,
ACCESS_MASK DesiredAccess,
OBJECT_ATTRIBUTES* ObjectAttributes,
@@ -30,6 +42,38 @@

namespace crashpad {

+NTSTATUS NtClose(HANDLE handle) {
+ static const auto nt_close =
GET_FUNCTION_REQUIRED(L"ntdll.dll", ::NtClose);
+ return nt_close(handle);
+}
+
+NTSTATUS
+NtCreateThreadEx(PHANDLE thread_handle,
+ ACCESS_MASK desired_access,
+ POBJECT_ATTRIBUTES object_attributes,
+ HANDLE process_handle,
+ PVOID start_routine,
+ PVOID argument,
+ ULONG create_flags,
+ SIZE_T zero_bits,
+ SIZE_T stack_size,
+ SIZE_T maximum_stack_size,
+ PVOID attribute_list) {
+ static const auto nt_create_thread_ex =
+ GET_FUNCTION_REQUIRED(L"ntdll.dll", ::NtCreateThreadEx);
+ return nt_create_thread_ex(thread_handle,
+ desired_access,
+ object_attributes,
+ process_handle,
+ start_routine,
+ argument,
+ create_flags,
+ zero_bits,
+ stack_size,
+ maximum_stack_size,
+ attribute_list);
+}
+
NTSTATUS NtQuerySystemInformation(
SYSTEM_INFORMATION_CLASS system_information_class,
PVOID system_information,
diff --git a/util/win/nt_internals.h b/util/win/nt_internals.h
index b622101..3ced5cc 100644
--- a/util/win/nt_internals.h
+++ b/util/win/nt_internals.h
@@ -19,6 +19,23 @@

namespace crashpad {

+NTSTATUS NtClose(HANDLE handle);
+
+// http://processhacker.sourceforge.net/doc/ntpsapi_8h_source.html
+#define THREAD_CREATE_FLAGS_SKIP_THREAD_ATTACH 0x00000002
+NTSTATUS
+NtCreateThreadEx(PHANDLE thread_handle,
+ ACCESS_MASK desired_access,
+ POBJECT_ATTRIBUTES object_attributes,
+ HANDLE process_handle,
+ PVOID start_routine,
+ PVOID argument,
+ ULONG create_flags,
+ SIZE_T zero_bits,
+ SIZE_T stack_size,
+ SIZE_T maximum_stack_size,
+ PVOID /*PPS_ATTRIBUTE_LIST*/ attribute_list);
+
// Copied from ntstatus.h because um/winnt.h conflicts with general
inclusion of
// ntstatus.h.
#define STATUS_BUFFER_TOO_SMALL ((NTSTATUS)0xC0000023L)

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 21

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................

Patch Set 21:

> Patch Set 19: Code-Review+2

> We need to find out some "sticky LGTM" configuration setting.

Agreed, filed https://bugs.chromium.org/p/chromium/issues/detail?id=605974.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 21
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: No

[Mark Mentovai (Gerrit)]

Mark Mentovai has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 21:

(1 comment)

https://chromium-review.googlesource.com/#/c/339263/21/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 514: ScopedProcessSuspend suspend(process);
I just realized that we might want to account for this with another -1 in
the suspension counts that we report for all except the blame thread.

--
To view, visit https://chromium-review.googlesource.com/339263
To unsubscribe, visit https://chromium-review.googlesource.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I797007bd2b1e3416afe3f37a6566c0cdb259b106
Gerrit-PatchSet: 21
Gerrit-Project: crashpad/crashpad
Gerrit-Branch: master
Gerrit-Owner: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Mark Mentovai <ma...@chromium.org>
Gerrit-Reviewer: Scott Graham <sco...@chromium.org>
Gerrit-Reviewer: Sigurdur Asgeirsson <si...@chromium.org>

Gerrit-HasComments: Yes

[Scott Graham (Gerrit)]

Scott Graham has posted comments on this change.

Change subject: win: Support dumping another process by causing it to crash
......................................................................


Patch Set 21:

(1 comment)

(Can you stamp https://chromium-review.googlesource.com/c/340361/ if you
have a sec, too?)

https://chromium-review.googlesource.com/#/c/339263/21/client/crashpad_client_win.cc
File client/crashpad_client_win.cc:

Line 514: ScopedProcessSuspend suspend(process);
> I just realized that we might want to account for this with another -1 in
> t

Oh, good point. I'll see how tricky that is now.