This supports the “double handler” or “double handler with low
probability” models from https://crashpad.chromium.org/bug/143.
For crashpad_handler to be become its own client, it needs access to its
own executable path to pass to CrashpadClient::StartHandler(). This was
formerly available in the test-only test::Paths::Executable(). Bring
that function’s implementation to the non-test Paths::Executable() in
util/misc, and rename test::Paths to test::TestPaths to avoid future
confusion.
test::TestPaths must still be used to access TestDataRoot(), which does
not make any sense to non-test code.
test::TestPaths::Executable() is retained for use by tests, which most
likely prefer the fatal semantics of that function. Paths::Executable()
is not fatal because for the purposes of implementing the double
handler, a failure to locate the executable path (which may happen on
some systems in deeply-nested directory hierarchies) shouldn’t cause the
initial crashpad_handler to abort, even if it does prevent a second
crashpad_handler from being started.
Bug: crashpad:143
Test: crashpad_util_test Paths.*, crashpad_test_test TestPaths.*
Change-Id: I9f75bf61839ce51e33c9f7c0d7031cebead6a156
Reviewed-on: https://chromium-review.googlesource.com/466346
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Checking child process’ exit codes would have helped catch bug
crashpad:160 sooner. Instead, we had a flaky hang that was difficult to
reproduce locally.
Bug: crashpad:160
Test: crashpad_snapshot_test ExceptionSnapshotWinTest.ChildCrash*:ProcessSnapshotTest.CrashpadInfoChild*:SimulateCrash.ChildDumpWithoutCrashing*, crashpad_util_test ProcessInfo.OtherProcess
Change-Id: I73bd2be1437d05f0501a146dcb9efbe3b8e0f8b7
Reviewed-on: https://chromium-review.googlesource.com/459039
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
ReadFile() attempted to continue reading after a short read. In most
cases, this is fine. However, ReadFile() would keep trying to fill a
partially-filled buffer until experiencing a 0-length read(), signaling
end-of-file. For certain weird file descriptors like terminal input, EOF
is an ephemeral condition, and attempting to read beyond EOF doesn’t
actually return 0 (EOF) provided that they remain open, it will block
waiting for more input. Consequently, ReadFile() and anything based on
ReadFile() had an undocumented and quirky interface, which was that any
short read that it returned (not an underlying short read) actually
indicated EOF.
This facet of ReadFile() was unexpected, so it’s being removed. The new
behavior is that ReadFile() will return an underlying short read. The
behavior of FileReaderInterface::Read() is updated in accordance with
this change.
Upon experiencing a short read, the caller can determine the best
action. Most callers were already prepared for this behavior. Outside of
util/file, only crashpad_database_util properly implemented EOF
detection according to previous semantics, and adapting it to new
semantics is trivial.
Callers who require an exact-length read can use the new
ReadFileExactly(), or the newly renamed LoggingReadFileExactly() or
CheckedReadFileExactly(). These functions will retry following a short
read. The renamed functions were previously called LoggingReadFile() and
CheckedReadFile(), but those names implied that they were simply
wrapping ReadFile(), which is not the case. They wrapped ReadFile() and
further, insisted on a full read. Since ReadFile()’s semantics are now
changing but these functions’ are not, they’re now even more distinct
from ReadFile(), and must be renamed to avoid confusion.
Test: *
Change-Id: I06b77e0d6ad8719bd2eb67dab93a8740542dd908
Reviewed-on: https://chromium-review.googlesource.com/456676
Reviewed-by: Robert Sesek <rsesek@chromium.org>
It could be useful to put our existing Crashpad.HandlerCrashed metrics
into context by getting a sense of handler starts, clean exits, and
other types of exits.
BUG=crashpad:100
Change-Id: I8982075158ea6d210eb2ddad678302e339a42192
Reviewed-on: https://chromium-review.googlesource.com/444124
Reviewed-by: Scott Graham <scottmg@chromium.org>
BUG=crashpad:158
Change-Id: If8666140a7fc5315eeb791d0998226de89a22cc3
Reviewed-on: https://chromium-review.googlesource.com/438791
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
I haven't been able to reproduce this locally, but we see errors in
crash dumps where the unloaded module list consists of a number of
modules with invalid names and implausible addresses. My assumption is
that RTL_UNLOAD_EVENT_TRACE isn't correct for some OS levels. Instead of
trying to finesse and test that, use RtlGetUnloadEventTraceEx() instead
of RtlGetUnloadEventTrace(), which returns an element size. (This
function is Vista+ which is why it wasn't used the first time around.)
R=mark@chromium.org
BUG=chromium:620175
Change-Id: I4d7080a03623276f9c1c038d6e7329af70e4a64c
Reviewed-on: https://chromium-review.googlesource.com/421564
Reviewed-by: Mark Mentovai <mark@chromium.org>
ConvertStringSecurityDescriptorToSecurityDescriptor() is used when
creating the initial connection pipe. Because this is done from inside
DllMain(), we cannot use advapi32 (where this function is). Instead,
save the binary representation of the self-relative SECURITY_DESCRIPTOR.
It is conceivable that this could change, but unlikely as this is the
same blob that would be stored on a file in NTFS.
Another potential approach would be to not make the pipe available to
all integrity levels here, and instead modify the Chromium sandbox code
to allow a specific pipe name prefix that would have to correspond with
the pipe name that Crashpad creates.
Similarly, UuidCreate() (used when initializing the database) is in a
DLL that can't be loaded early, so use the Linux/Android implementation
on Windows too.
R=mark@chromium.org
BUG=chromium:655788,chromium:656800
Change-Id: I434f8e96fc275fc30d0a31208b025bfc08595ff9
Reviewed-on: https://chromium-review.googlesource.com/417223
Reviewed-by: Mark Mentovai <mark@chromium.org>
This eliminates all constructors, but nearly all points of use were
using the default constructor to initialize a UUID member variable as in
uuid_(). This syntax will still produce a zeroed-out UUID.
While compiling, for example, minidump_rva_list_writer.cc:
In file included from ../../minidump/minidump_rva_list_writer.h:25:0,
from ../../minidump/minidump_rva_list_writer.cc:15:
../../minidump/minidump_extensions.h:412:8: error: ignoring packed attribute because of unpacked non-POD field ‘crashpad::UUID crashpad::MinidumpCrashpadInfo::report_id’ [-Werror]
UUID report_id;
^~~~~~~~~
../../minidump/minidump_extensions.h:424:8: error: ignoring packed attribute because of unpacked non-POD field ‘crashpad::UUID crashpad::MinidumpCrashpadInfo::client_id’ [-Werror]
UUID client_id;
^~~~~~~~~
Tested with:
- GCC 4.9 from NDK r13 targeting arm with SDK 16
- GCC 4.9 from NDK r13 targeting arm64 with SDK 21
- GCC 6.2 targeting x86_64
BUG=crashpad:30
Change-Id: Iec6b1557441b69d75246f2f75c59c4158fb7ca29
Reviewed-on: https://chromium-review.googlesource.com/409641
Reviewed-by: Scott Graham <scottmg@chromium.org>
This makes Doxygen’s output more actionable by setting QUIET = YES to
suppress verbose progress spew, and WARN_IF_UNDOCUMENTED = NO to prevent
warnings for undocumented classes and members from being generated. The
latter is too noisy, producing 721 warnings in the current codebase.
The remaining warnings produced by Doxygen were useful and actionable.
They fell into two categories: abuses of Doxygen’s markup syntax, and
missing (or misspelled) parameter documentation. In a small number of
cases, pass-through parameters had intentionally been left undocumented.
In these cases, they are now given blank \param descriptions. This is
not optimal, but there doesn’t appear to be any other way to tell
Doxygen to allow a single parameter to be undocumented.
Some tricky Doxygen errors were resolved by asking it to not enter
directiores that we do not provide documentation in (such as the
“on-platform” compat directories, compat/mac and compat/win, as well as
compat/non_cxx11_lib) while allowing it to enter the
“off-platform” directories that we do document (compat/non_mac and
compat/non_win).
A Doxygen run (doc/support/generate_doxygen.sh) now produces no output
at all. It would produce warnings if any were triggered.
Not directly related, but still relevant to documentation,
doc/support/generate.sh is updated to remove temporary removals of
now-extinct files and directories. doc/appengine/README is updated so
that a consistent path to “goapp” is used throughout the file.
Change-Id: I300730c04de4d3340551ea3086ca70cc5ff862d1
Reviewed-on: https://chromium-review.googlesource.com/408812
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Second follow up to https://chromium-review.googlesource.com/c/400015/
The ideal would be that if we fail to start the handler, then we don't
end up passing through our unhandled exception filter at all.
In the case of the non-initial client (i.e. renderers) we can do this by
not setting our UnhandledExceptionFilter until after we know we've
connected successfully (because those connections are synchronous from
its point of view). We also change WaitForNamedPipe in the connection
message to block forever, so as long as the precreated pipe exists,
they'll wait to connect. After the initial client has passed the server
side of that pipe to the handler, the handler has the only handle to it.
So, if the handler has disappeared for whatever reason, pipe-connecting
clients will fail with FILE_NOT_FOUND, and will not stick around in the
connection loop. This means non-initial clients do not need additional
logic to avoid getting stuck in our UnhandledExceptionFilter.
For the initial client, it would be ideal to avoid passing through our
UEF too, but none of the 3 options are great:
1. Block until we find out if we started, and then install the filter.
We don't want to do that, because we don't want to wait.
2. Restore the old filter if it turns out we failed to start. We can't
do that because Chrome disables ::SetUnhandledExceptionFilter()
immediately after StartHandler/SetHandlerIPCPipe returns.
3. Don't install our filter until we've successfully started. We don't
want to do that because we'd miss early crashes, negating the benefit
of deferred startup.
So, we do need to pass through our UnhandledExceptionFilter. I don't
want more Win32 API calls during the vulnerable filter function. So, at
any point during async startup where there's a failure, set a global
atomic that allows the filter function to abort without trying to signal
a handler that's known to not exist.
One further improvement we might want to look at is unexpected
termination of the handler (as opposed to a failure to start) which
would still result in a useless Sleep(60s). This isn't new behaviour,
but now we have a clear thing to do if we detect the handler is gone.
(Also a missing DWORD/size_t cast for the _x64 bots.)
R=mark@chromium.org
BUG=chromium:567850,chromium:656800
Change-Id: I5be831ca39bd8b2e5c962b9647c8bd469e2be878
Reviewed-on: https://chromium-review.googlesource.com/400985
Reviewed-by: Mark Mentovai <mark@chromium.org>
Previously, StartHandler() launched the handler process, then connected
over a pipe to register for crash handling. Instead, the initial client
can create and inherit handles to the handler and pass those handle
values and other data (addresses, etc.) on the command line.
This should improve startup time as there's no need to synchronize with
the process at startup, and allows avoiding a call to CreateProcess()
directly in StartHandler(), which is important for registration for
crash reporting from DllMain().
Incidentally adds new utility functions for string/number conversion and
string splitting.
Note: API change; UseHandler() is removed for all platforms.
BUG=chromium:567850,chromium:656800
Change-Id: I1602724183cb107f805f109674c53e95841b24fd
Reviewed-on: https://chromium-review.googlesource.com/400015
Reviewed-by: Mark Mentovai <mark@chromium.org>
d:\src\crashpad\crashpad>git checkout origin/master
Note: checking out 'origin/master'.
...
HEAD is now at f497e54... win: Fix indirectly gathered memory cap
[f497e54...]d:\src\crashpad\crashpad>ninja -C out\Debug
ninja: Entering directory `out\Debug'
[0->23/23 ~0] STAMP obj\All.actions_depends.stamp
[f497e54...]d:\src\crashpad\crashpad>tim out\Debug\crashpad_snapshot_test --gtest_filter=ProcessSnapshotTest.CrashpadInfoChild
Running main() from gtest_main.cc
Note: Google Test filter = ProcessSnapshotTest.CrashpadInfoChild
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ProcessSnapshotTest
[ RUN ] ProcessSnapshotTest.CrashpadInfoChild
[ OK ] ProcessSnapshotTest.CrashpadInfoChild (147879 ms)
[----------] 1 test from ProcessSnapshotTest (147880 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (147884 ms total)
[ PASSED ] 1 test.
real: 2m27.907s
qpc: 147914874us
[f497e54...]d:\src\crashpad\crashpad>git checkout slow-debug
Previous HEAD position was f497e54... win: Fix indirectly gathered memory cap
Switched to branch 'slow-debug'
Your branch is ahead of 'origin/master' by 2 commits.
(use "git push" to publish your local commits)
[slow-debug]d:\src\crashpad\crashpad>ninja -C out\Debug
ninja: Entering directory `out\Debug'
[0->23/23 ~0] STAMP obj\All.actions_depends.stamp
[slow-debug]d:\src\crashpad\crashpad>tim out\Debug\crashpad_snapshot_test --gtest_filter=ProcessSnapshotTest.CrashpadInfoChild
Running main() from gtest_main.cc
Note: Google Test filter = ProcessSnapshotTest.CrashpadInfoChild
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ProcessSnapshotTest
[ RUN ] ProcessSnapshotTest.CrashpadInfoChild
[ OK ] ProcessSnapshotTest.CrashpadInfoChild (4414 ms)
[----------] 1 test from ProcessSnapshotTest (4416 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (4420 ms total)
[ PASSED ] 1 test.
real: 0m4.453s
qpc: 4454559us
R=mark@chromium.org
BUG=crashpad:114
Change-Id: I9f18fe54a2711a483ced86ece0b261cdfffc6192
Reviewed-on: https://chromium-review.googlesource.com/346490
Reviewed-by: Mark Mentovai <mark@chromium.org>
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 <mark@chromium.org>
Kasko needs a way to read crash keys from out of process. This API
reuses the functionality of PEImageAnnotationsReader.
Change-Id: I2f3bbc358212e6f50235183e9dbb4e5a2cf989cf
This is a reupload of https://codereview.chromium.org/1586433003/ but
for gerrit.
Change-Id: I2f3bbc358212e6f50235183e9dbb4e5a2cf989cf
Reviewed-on: https://chromium-review.googlesource.com/322550
Reviewed-by: Scott Graham <scottmg@chromium.org>
Tested-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Scott Graham <scottmg@google.com>
This was done in Chromium’s local copy of Crashpad in 562827afb599. This
change is similar to that one, except more care was taken to avoid
including headers from a .cc or _test.cc when already included by the
associated .h. Rather than using <stddef.h> for size_t, Crashpad has
always used <sys/types.h>, so that’s used here as well.
This updates mini_chromium to 8a2363f486e3a0dc562a68884832d06d28d38dcc,
which removes base/basictypes.h.
e128dcf10122 Remove base/move.h; use std::move() instead of Pass()
8a2363f486e3 Move basictypes.h to macros.h
R=avi@chromium.org
Review URL: https://codereview.chromium.org/1566713002 .
This more-natural spelling doesn’t require Crashpad developers to have
to remember anything special when writing code in Crashpad. It’s easier
to grep for and it’s easier to remove the “compat” part when pre-C++11
libraries are no longer relevant.
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1513573005 .
MEMORY_BASIC_INFORMATION64 specifies an alignment of 16, but the
standard allocator used by containers doesn't honor this. Although 16
is the default alignment size used on Windows for x86_64, it's not for
32-bit x86. clang assumed that the alignment of the structure was as
declared, and used an SSE load sequence that required this alignment.
AlignedAllocator is a replacement for std::allocator that allows the
alignment to be specified. AlignedVector is an std::vector<> that uses
AlignedAllocator instead of std::allocator.
BUG=chromium:564691
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1498133002 .
Don't call GetFileVersionInfo(), which calls LoadLibrary() to be able to
access the module's resources. Loading modules from the crashy process
into the handler process can cause trouble. The Crashpad handler
definitely doesn't want to run arbitrary modules' module initializer
code.
Since the VS_FIXEDFILEINFO needed is already in memory in the remote
process' address space, just access it from there.
BUG=crashpad:78
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1475023004 .
This unifies several things that used a 16-character random string, and
a few other users of random identifiers where it also made sense to use
a 16-character random string.
TEST=crashpad_util_test RandomString.RandomString
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1451793002 .
The bug and linked code review has more of the history, but we’ve been
tempted to remove the loop outright a couple of times already before
realizing that it serves an important purpose. Hopefully this comment
will protect our future selves from going on the same fool’s errand.
BUG=crashpad:75
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1427643010 .
This is better because now end_to_end_test.py fails immediately with
[1180:9020:20151106,145204.830:ERROR registration_protocol_win.cc:39] CreateFile: The system cannot find the file specified. (0x2)
R=mark@chromium.org
BUG=crashpad:75
Review URL: https://codereview.chromium.org/1409693011 .
In 2eeaa3ac549a, I added a check to make sure that the expected CONOUT$
handle was found. Its omission seemed to be unintentional. The tests
passed for me on Windows 10, but failed on the bots. I can reproduce
the failures locally on Windows 7.
Doing the inheritance test with a file other than CONOUT$ fixes the
immediate problem, but we should find out why this CONOUT$ handle isn't
showing up in the handles list on Windows 7, fix it, and add back a
test.
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1428753009 .
This consolidates all of the twisted casts and comments that discuss how
HANDLEs are really only 32 bits wide even in 64-bit processes on 64-bit
operating systems into a single location.
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1422503015 .
Allowing the client to create its own pipe name string caused a race
between client and server. Instead, in this mode, the server now creates
the pipe name along with a pipe, and returns it to its client via a
--handshake-handle. This guarantees that by the time the client gets the
pipe name, the server has already created it.
Ephemeral mode is now implied by --handshake-handle. The --persistent
option is gone. --persistent mode is enabled when using --pipe-name.
BUG=crashpad:69
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1432563003 .
I considered writing the CodeView records to the minidump, but I didn't
find a ton of docs and debugging is only lightly supported (e.g.
http://www.debuginfo.com/articles/gendebuginfo.html#debuggersandformats
and it doesn't attempt to load at all on more recent Visual Studios).
As we won't be generating symbols in this format, and we don't expect to
have symbols for any weird modules that get injected into us in the
wild, it seems like we don't lose anything by just ignoring them.
R=mark@chromium.org
BUG=crashpad:47
Review URL: https://codereview.chromium.org/1430773003 .
Previously, crashpad_handler made its own receive right, and transferred
a corresponding send right to its client. There are two advantages to
making the receive right in the client:
- It is possible to monitor the receive right for a port-destroyed
notificaiton in the client, allowing the handler to be restarted if
it dies.
- For the future run-from-launchd mode (bug crashpad:25), the handler
will obtain its receive right from the bootstrap server instead of
making its own. Having the handler get its receive right from
different sources allows more code to be shared than if it were to
sometimes get a receive right and sometimes make a receive right and
transfer a send right.
This includes a restructuring in crashpad_client_mac.cc that will make
it easier to give it an option to restart crashpad_handler if it dies.
The handler starting logic should all behave the same as before.
BUG=crashpad:68
R=rsesek@chromium.org
Review URL: https://codereview.chromium.org/1409073013 .
This allows better code sharing in crashpad_handler’s main(). It doesn’t
look like much of an improvement now, but a separate change will cause
the Mac ExceptionHandlerServer() to be constructed with an argument. It
will be beneficial for Mac and Windows to be able to share the Run()
call.
R=scottmg@chromium.org
Review URL: https://codereview.chromium.org/1402333004 .
