Google Test has recently switched the default death test style from
"fast" to "threadsafe". This is a better default, and Chrome will adopt
it on all platforms except for Android.
In threadsafe mode, the death test in
client/simple_string_dictionary_test.cc consistently crashes with the
wrong expectation on Mac. Fortunately, breaking the test up into two
smaller tests makes the failures go away, and also adds a bit of clarity
into what is being tested.
Bug: crashpad:221
Change-Id: I2416647948815cfe46a003da8209af8b7278de2a
Reviewed-on: https://chromium-review.googlesource.com/936043
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
This ensures the symbol is not exposed in the binaries final symbol
table. .globl needs to be kept so that it can still be linked against
(in this case, by crashpad_info.cc.).
(Tested on Fuchsia, hopefully functional elsewhere...)
Bug: crashpad:196
Change-Id: I8c6b26cdd742a1c040779884fd97a8a34068dbdc
Reviewed-on: https://chromium-review.googlesource.com/924337
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
Fuchsia does not currently support any sort of file locking. Until a
lock server can be implemented, compile out the calls to flock(). In the
one current non-test user of locking (Settings) add a
pseudo-implementation that will DCHECK if there is ever contention on
the lock.
Bug: crashpad:217, crashpad:196
Change-Id: Ifdf7e00886ad7e7778745f1ae8f0ce2a86f0ae3b
Reviewed-on: https://chromium-review.googlesource.com/924312
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
These tests needed to be updated to expose CrashpadInfo in the same way
as the main CrashpadInfo g_crashpad_info is found on
Linux/Android/Fuchsia.
Unfortunately, while the tests pass on Fuchsia when run in isolation,
the implementation of dlclose() on Fuchsia currently does nothing. So,
if the full test suite is run, there's interference between the test
modules (i.e. the values in _small vs. the values in _large), so the
tests fail.
I filed ZX-1728 upstream about this to see if it might be implemented,
or if the test will need to spawn a clean child to do the module load
tests in.
Bug: crashpad:196
Change-Id: I9ee01b142a29c508c6967dc83da824afa254d379
Reviewed-on: https://chromium-review.googlesource.com/923182
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
Fuchsia errors out in rename() when source == dest. I believe this is
incorrect according to
http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html,
but it's also relatively easy to work around in our code, and this fixes
CrashReportDatabaseTest.RequestUpload.
This is ZX-1729 upstream.
Bug: crashpad:196
Change-Id: I27473183b04484e146a7bd9e87e60be3aeff1932
Reviewed-on: https://chromium-review.googlesource.com/923708
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
Placing a 32-bit pointer directly into a .quad results in either an
unsupported relocation error at link time (ARM) or an inability to
load the executable (x86).
Also, only attempt to read a module's CrashpadInfo if an info address
note was found.
Change-Id: I053af3d77eed70af66248be88547656d2b29878a
Reviewed-on: https://chromium-review.googlesource.com/922397
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
This CL, based on
https://chromium-review.googlesource.com/c/crashpad/crashpad/+/689745
adds a cross-platform database implementation side-by-side with the
existing macOS and Windows implementations. The generic implementation
is used for Linux, Android and Fuchsia.
The database uses the directory structure from the macOS
implementation, but stores report metadata in companion files for each
report, rather than using filesystem attributes. The database uses
lockfiles (companion files opened with O_EXCL) to protect report access
because they are widely supported across filesystems. Lost lockfiles
are removed after 3 days, along with any reports or metadata they were
protecting.
Bug: crashpad:206
Change-Id: I086e9001350e4446dd2f8c12fd3817377f509d3e
Reviewed-on: https://chromium-review.googlesource.com/919527
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Embeds the address of g_crashpad_info into a .note section (which is
readable by the generic code to read notes in ElfImageReader).
Unfortunately because the note section is in libclient.a, it would
normally be dropped at link time. To avoid that, GetCrashpadInfo() has
a reference *back* to that section, which in turn forces the linker to
include it, allowing the note reader to find it at runtime.
Previously, it was necessary to have the embedder of "client" figure out
how to cause `g_crashpad_info` to appear in the final module's dynamic
symbol table. With this new approach, there's no manual configuration
necessary, as it's not necessary for the symbol to be exported.
This is currently only implemented in the Linux module reader (and I
believe the current set of enabled tests aren't exercising it?) but it
will also be done this way for the Fuchsia implementation of
ModuleSnapshot.
Bug: crashpad:196
Change-Id: I599db5903bc98303130d11ad850ba9ceed3b801a
Reviewed-on: https://chromium-review.googlesource.com/912284
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
This change adds CrashReportDatabase::UploadReport which owns the
report's file handle during upload. An upload is recorded as a success
by calling RecordUploadComplete(). If RecordUploadComplete() is not
called, the operation is recorded as a failure when the UploadReport is
destroyed.
Bug: crashpad:206
Change-Id: I8385d08d52185ad30b06a3ed054de9812ae006a2
Reviewed-on: https://chromium-review.googlesource.com/917983
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
This change updates CrashReportDatbase::NewReport objects to own the
file handle associated with the new report, now accessible via a
FileWriter. NewReport's destructor closes its file handle and removes
its new report unless disarmed with FinishedWritingCrashReport,
eliminating the need for CallErrorWritingCrashReport.
Bug: crashpad:206
Change-Id: Iccb5bbc0ebadb07a237ff8eb938389afcfeae2a5
Reviewed-on: https://chromium-review.googlesource.com/916941
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Pulled out of jperaza's https://crrev.com/c/689745.
Future updates to the CrashReportDatabase would like to be decide on the
Settings location later than the constructor, but still keep the Settings
object embedded inline. To allow this, pass the location FilePath in
Initialize() rather than to the constructor.
Bug: crashpad:206
Change-Id: I8792188314541f6fd0bd04b168d22f8e445bc187
Reviewed-on: https://chromium-review.googlesource.com/916533
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Previously, the mac version was under client/ and win under util/win/.
This cl brings them all together under util/misc/ and combines common
test code.
Bug: crashpad:30
Change-Id: Idf0d0158b969d5aa9802dfc8c21f73041b2bcc6c
Reviewed-on: https://chromium-review.googlesource.com/907755
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
In setting up the gn build, slightly different optimization settings
were applied for release builds. This caused a couple things to happen,
1) the sketchy noinline declspec was ignored, and 2) the distance
between reading the IP and the actual crash exceeded the tolerance of 64
bytes in the parent.
To make the test more robust to this, use CaptureContext() (I think our
improved version didn't exist at the time the tests was originally
written). Also, switch from crashpad::CheckedWriteFile to Windows'
WriteFile(), which avoids inlining a whole lot of code at that point.
The return value is not checked, but the next thing that happens is that
the function crashes unconditionally, so this does not seem like a huge
problem.
Bug: crashpad:79
Change-Id: I8193d8ce8b01e1533c16b207813c36d6d6113d89
Reviewed-on: https://chromium-review.googlesource.com/902693
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Responsibility for creating argv_c has moved to DoubleForkAndExec().
Change-Id: Id663f0597ee1749df564cdacac1d877b5545750b
Reviewed-on: https://chromium-review.googlesource.com/898024
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
This change includes methods to install a signal handler to launch
the handler process at crash time or to launch the handler on behalf
of another process.
Bug: crashpad:30
Change-Id: I503c788cb3648852d09e9e8c1fe5099ca07a0277
Reviewed-on: https://chromium-review.googlesource.com/759406
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
In doing standalone bringup of Crashpad targeting Fuchsia, it seemed
tidy to keep the same literal paths to the dependencies that Chromium
needed and add stubs/forwarding to build/secondary in the Crashpad tree
as required to make those work.
However, when trying to build Crashpad in the Fuchsia tree itself, that
would require adding forwarding files to the Fuchsia tree to match the
Chromium directory structure, which would be awkward. Instead, have
explicit dependencies in the Crashpad tree that select the locations
for various dependencies.
Bug: crashpad:79, crashpad:196
Change-Id: Ib506839f9c97d8ef823663cdc733cbdcfa126139
Reviewed-on: https://chromium-review.googlesource.com/826025
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
This allows brace initializing a C array of StringAnnotation objects.
Bug: crashpad:192
Change-Id: Id1b187b67b24bb57251957e9d9c18c16579f1dd4
Reviewed-on: https://chromium-review.googlesource.com/807645
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Stubs a variety of classes (CrashReportExceptionHandler,
ExceptionHandlerServer, HTTPTransport, CrashReportDatabase).
Bug: crashpad:196
Change-Id: I4772f90d0d2ad07cc2f3c2ef119e92fde5c7acef
Reviewed-on: https://chromium-review.googlesource.com/809940
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
This avoids relying on set_sources_assignment_filter, and so gets closer
to a correct set of files to build on Fuchsia.
Bug: crashpad:79, crashpad:196
Change-Id: Ib7daa5137935113c6645b72eb1dedd943a9db96e
Reviewed-on: https://chromium-review.googlesource.com/797672
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
- Adds a .gn and a build/BUILDCONFIG.gn that uses mini_chromium's
build/BUILD.gn.
- Adds some stub BUILD.gn files in locations where Chromium expects them
(in //build, //testing, //third_party) containing empty targets/configs.
These are no-ops in standalone builds, but add functionality when
building in Chromium. This is in preference to having a global bool
that conditionally does Chromium-y things in the Crashpad build files.
These stub files are all contained in a secondary source root in
build/chromium_compatibility, referred to by //.gn.
- Adds //base/BUILD.gn which forwards to mini_chromium/base. This is
only used when building standalone so that both Chromium and Crashpad
can refer to it as "//base".
- Changes references to other Crashpad targets to be relatively
specified so that they work when the root of the project is //, and also
when it's //third_party/crashpad/crashpad as it is in Chromium.
- Moves any error-causing Mac/Win-specific files into explicit if (is_mac)
or if (is_win) blocks as part of removing the dependency on
set_sources_assignment_filter().
As yet unresolved:
- CRASHPAD_IN_CHROMIUM needs to be removed when standalone; to be tackled
in a follow up.
- Not sure what to do with zlib yet, the build file currently assumes
"in Chromium" too, and similarly having Crashpad //third_party/zlib:zlib
pointing at itself doesn't work.
Bug: crashpad:79
Change-Id: I6a7dda214e4b3b14a60c1ed285267ab97432a1a8
Reviewed-on: https://chromium-review.googlesource.com/777410
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
In Chromium, the AnnotationList is registered in the main executable
module. However, when using the component build, the individual shared
libraries do not explicitly initialize the CrashpadInfo nor
AnnotationList. This causes annotations to NULL-dereference the
uninitialized AnnotationList when using the component build.
By using the Register method instead, the AnnotationList will be lazily
created. In Chromium's static/release build, the AnnotationList will
still be initialized deterministically during startup.
Bug: crashpad:192
Change-Id: I8599b52630f4d7608e5028b14264a8eed49a9176
Reviewed-on: https://chromium-review.googlesource.com/793981
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
I have no idea if this will work as not much is building yet, but it
seems plausible for the time being.
Bug: crashpad:196
Change-Id: Ie3a358512a968e9e777ed03c0bffc5e273a0f12e
Reviewed-on: https://chromium-review.googlesource.com/786777
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Change-Id: I4b247d7fae1a212350f8ffcf2bf5ba1fa730f5c1
Reviewed-on: https://chromium-review.googlesource.com/780339
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Crashpad has many tests that crash intentionally. Some of these are
gtest death tests, and others arrange for intentional crashes to test
Crashpad’s own crash-catching logic. On macOS, all of the gtest death
tests and some of the other intentional crashes were being logged by
ReportCrash, the system’s crash reporter. Since these reports
corresponded to intentional crashes, they were never useful, and served
only to clutter ~/Library/Logs/DiagnosticReports.
Since Crashpad is adept at handling exceptions on its own, this
introduces the “exception swallowing server”,
crashpad_exception_swallower, which is a Mach exception server that
implements a no-op exception handler routine for all exceptions
received. The exception swallowing server is established as the task
handler for EXC_CRASH and EXC_CORPSE_NOTIFY exceptions during gtest
death tests invoked by {ASSERT,EXPECT}_DEATH_{CHECK,CRASH}, and for all
child processes invoked by the Multiprocess test infrastructure. The
exception swallowing server is not in effect at other times, so
unexpected crashes in test code can still be handled by ReportCrash or
another crash reporter.
With this change in place, no new reports are generated in the
user-level ~/Library/Logs/DiagnosticReports or the system’s
/Library/Logs/DiagnosticReports during a run of Crashpad’s full test
suite on macOS.
Bug: crashpad:33
Change-Id: I13891853a7e25accc30da21fa7ea8bd7d1f3bd2f
Reviewed-on: https://chromium-review.googlesource.com/777859
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Unreferenced, and not working at all in Crashpad-standalone.
Copied from Chromium at 52a9831d81f2099ef9f50fcdaca5853019262c35 to have
a point where a roll back into Chromium should be a no-op (with Chromium's
build/secondary/third_party/crashpad/... removed).
I'm not sure what we want to do about the various gni references into
Chromium (e.g. //build/config/sanitizers/sanitizers.gni, //testing/test.gni,
etc.) but I guess the sooner they live in Crashpad rather than in Chromium
the sooner we can figure out the sort of knobs and dials we need.
Bug: crashpad:79
Change-Id: Id99c29123bcd4174ee2bcc128c2be87e3c94fa3f
Reviewed-on: https://chromium-review.googlesource.com/777819
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Scott Graham <scottmg@chromium.org>
The handler will now be less strict about checking CrashpadInfo struct
sizes. Assuming the signature and version fields match:
- If the handler sees a struct smaller than it’s expecting, the module
was likely built with an earlier version of the client library, and
it’s safe to treat the unknown fields as though they were zero or
other suitable default values.
- If the handler sees a struct larger than it’s expecting, the module
was likely built with a later version of the client library. In that
case, actions desired by the client will not be performed, but this
is not otherwise an error condition.
The CrashpadInfo struct must always be at least large enough to contain
at least the size field. The signature and version fields are always
checked.
The section size must be at least as large as the size carried within
the struct. To account for possible section padding, strict equality is
not required.
Bug: chromium:784427
Test: crashpad_snapshot_test CrashpadInfoSizes_ClientOptions/*.*
Change-Id: Ibb0690ca6ed5e7619d1278a68ba7e893d55f19fb
Reviewed-on: https://chromium-review.googlesource.com/767709
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
This introduces the Annotation object, used to declare typed
annotations, and the AnnotationList object, used to reference these. The
AnnotationList is referenced by the CrashpadInfo structure. Currently
nothing reads these.
The AnnotationList implements a lock-free linked list, into which
Annotation objects are added exactly once, when they are first set.
Clearing an Annotation merely marks it internally as such, rather than
removing it from the list.
Bug: crashpad:192
Change-Id: I72414b1f83d624c4ae323e09ecea8cfb69a68c5e
Reviewed-on: https://chromium-review.googlesource.com/547135
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Update mini_chromium to 7d6697ceb5cb5ca02fde3813496f48b9b1d76d0c
47ff9691450e Switch the language standard to C++14
7d6697ceb5cb Remove base/memory/ptr_util.h and base::WrapUnique
base::WrapUnique and std::make_unique are similar, but the latter is
standardized and preferred.
Most of the mechanical changes were made with this sed:
for f in $(git grep -l base::WrapUnique | uniq); do
sed -E \
-e 's%base::WrapUnique\(new ([^(]+)\((.*)\)\);%std::make_unique<\1>(\2);%g' \
-e 's%base::WrapUnique\(new ([^(]+)\);%std::make_unique<\1>();%g' \
-e 's%^#include "base/memory/ptr_util.h"$%#include <memory>%' \
-i '' "${f}"
done
Several uses of base::WrapUnique that did not fit on a single line and
were not matched by this sed were adjusted manually. All #include
changes were audited manually, to at least move <memory> into the
correct section. Where <memory> was already #included by a file (or its
corresponding header), the extra #include was removed. Where <memory>
should have been #included by a header, it was added. Other similar
adjustments to other #includes were also made.
Change-Id: Id4e0baad8b3652646bede4c3f30f41fcabfdbd4f
Reviewed-on: https://chromium-review.googlesource.com/714658
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
CrashpadClient::DumpAndCrashTargetProcess() suspends the target process
and injects a thread to raise an exception. The injected thread is not
suspended, and may proceed to the point that the system recognizes the
process as terminating by the time the overall process suspension is
lifted. Previously, if this happened, an extraneous error was logged for
the attempt to resume a terminating process.
This introduces “termination tolerance” to ScopedProcessSuspend, which
allows an object to be configured to ignore this error and not log any
messages when this condition is expected.
This resolves log messages such as this one, produced frequently during
calls to CrashpadClient::DumpAndCrashTargetProcess() (including in
end_to_end_test.py):
> [pid:tid:yyyymmdd,hhmmss.mmm:ERROR scoped_process_suspend.cc:39]
> NtResumeProcess: An attempt was made to access an exiting process.
> (0xc000010a)
0xc000010a = STATUS_PROCESS_IS_TERMINATING
Test: end_to_end_test.py
Change-Id: Iab4c50fb21adce5502080ad25a6f734ec566d65c
Reviewed-on: https://chromium-review.googlesource.com/700715
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
The binary crashpad_handler.com is used by crashpad_client_win_test.cc,
but is not currently built when building crashpad_client_test.
Bug: crashpad:
Change-Id: I7a440774e49be9e821bca57c154a67b968a4bfbd
Reviewed-on: https://chromium-review.googlesource.com/695832
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
To enable clang-cl's printf format string mismatch checking, a few
mismatch errors need to be fixed where DWORD (unsigned long) is printed
with %u, %d or %x (an 'l' is needed).
Change-Id: I2cbfafe823a186bfe3a555aec3a7ca03e85466f8
Reviewed-on: https://chromium-review.googlesource.com/598651
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
This is essentially based on a search for “^const .*=”.
Change-Id: I9332c1f0cf7c891ba1ae373dc537f700f9a1d956
Reviewed-on: https://chromium-review.googlesource.com/585452
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
This is essentially based on a search for “^ *const [^*&]*=[^(]*$”
Change-Id: Id571119d0b9a64c6f387eccd51cea7c9eb530e13
Reviewed-on: https://chromium-review.googlesource.com/585555
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
This uses “static” at function scope to avoid making local copies, even
in cases where the compiler can’t see that the local copy is
unnecessary. “constexpr” adds additional safety in that it prevents
global state from being initialized from any runtime dependencies, which
would be undesirable.
At namespace scope, “constexpr” is also used where appropriate.
For the most part, this was a mechanical transformation for things
matching '(^| )const [^=]*\['.
Similar transformations could be applied to non-arrays in some cases,
but there’s limited practical impact in most non-array cases relative to
arrays, there are far more use sites, and much more manual intervention
would be required.
Change-Id: I3513b739ee8b0be026f8285475cddc5f9cc81152
Reviewed-on: https://chromium-review.googlesource.com/583997
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>
In the 10.12 SDK, x86_state_hdr from <mach/i386/thread_status.h> was
defined as:
struct x86_state_hdr {
int flavor;
int count;
};
This has changed in the 10.13 SDK to:
struct x86_state_hdr {
uint32_t flavor;
uint32_t count;
};
This triggers signedness mismatch errors where these values are used
with CHECK/DCHECK macros and gtest EXPECT/ASSERT macros.
Compatibility with existing and new SDKs must be maintained, so more
casts must be used.
Bug: crashpad:185, crashpad:188
Change-Id: I8844d6a78520430a8b5b90a35403896c3c6cfa37
Reviewed-on: https://chromium-review.googlesource.com/533375
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
I opted to leave casts to types that were definitely the same size
alone. reinterpret_cast<uintptr_t>(pointer) and
reinterpret_cast<intptr_t>(pointer) should always be safe, for example.
Casts to other integral types have been replaced with
FromPointerCast<>(), which does zero-extension or sign-extension based
on the target type.
To make it possible to use FromPointerCast<>() with some use sites that
were already using checked_cast<>(), FromPointerCast<>() now uses
check_cast<>() when converting to a narrower type.
Test: crashpad_util_test FromPointerCast*, others
Change-Id: I4a71b4aa2d87f545c75524290a702f5f3138d675
Reviewed-on: https://chromium-review.googlesource.com/489701
Reviewed-by: Scott Graham <scottmg@chromium.org>
Includes mini_chromium ef0ded8717340c9fe48e8e0f34f3e0e74d10a392.
1d2a024fdb1d android: Use _FILE_OFFSET_BITS after all (undo
dc3d480305b2)
ef0ded871734 win: MSVS 2017 (15)/C++ 14.1/C 19.10 compatibility
Change-Id: I5c814669a0ef8577872bddff9112ce28ec628ba3
Reviewed-on: https://chromium-review.googlesource.com/482639
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
TerminateProcess(), like most of the Windows API, is declared WINAPI,
which is __stdcall on 32-bit x86. That means that the callee,
TerminateProcess() itself, is responsible for cleaning up parameters on
the stack on return. In https://crashpad.chromium.org/bug/179, crashes
in ExceptionHandlerServer::OnNonCrashDumpEvent() were observed in ways
that make it evident that TerminateProcess() has been patched with a
__cdecl routine. The crucial difference between __stdcall and __cdecl is
that the caller is responsible for stack parameter cleanup in __cdecl.
The mismatch means that nobody cleans parameters from the stack, and the
stack pointer has an unexpected value, which in the case of the Crashpad
handler crash, results in TerminateProcess()’s second argument
erroneously being used as the lock address in the call to
ReleaseSRWLockExclusive() or LeaveCriticalSection().
As a workaround, on 32-bit x86, call through SafeTerminateProcess(), a
custom assembly routine that’s compatible with either __stdcall or
__cdecl implementations of TerminateProcess() by not trusting the value
of the stack pointer on return from that function. Instead, the stack
pointer is restored directly from the frame pointer.
Bug: crashpad:179
Test: crashpad_util_test SafeTerminateProcess.*, others
Change-Id: If9508f4eb7631020ea69ddbbe4a22eb335cdb325
Reviewed-on: https://chromium-review.googlesource.com/481180
Reviewed-by: Scott Graham <scottmg@chromium.org>
With multiple crashpad_handlers running out of the same database, it was
possible for more than one to attempt to upload the same report. Nothing
ensured that the reports remained pending between the calls to
CrashReportDatabaseMac::GetPendingReports() and
CrashReportDatabaseMac::GetReportForUploading().
The Windows equivalent did not share this bug, but it would return
kBusyError. kReportNotFound is a better code.
Test: crashpad_client_test CrashReportDatabaseTest.*
Change-Id: Ieaee7f94ca8e6f2606d000bd2ba508d3cfa2fe07
Reviewed-on: https://chromium-review.googlesource.com/473928
Reviewed-by: Robert Sesek <rsesek@chromium.org>
gtest used to require (expected, actual) ordering for arguments to
EXPECT_EQ and ASSERT_EQ, and in failed test assertions would identify
each side as “expected” or “actual.” Tests in Crashpad adhered to this
traditional ordering. After a gtest change in February 2016, it is now
agnostic with respect to the order of these arguments.
This change mechanically updates all uses of these macros to (actual,
expected) by reversing them. This provides consistency with our use of
the logging CHECK_EQ and DCHECK_EQ macros, and makes for better
readability by ordinary native speakers. The rough (but working!)
conversion tool is
https://chromium-review.googlesource.com/c/466727/1/rewrite_expectassert_eq.py,
and “git cl format” cleaned up its output.
EXPECT_NE and ASSERT_NE never had a preferred ordering. gtest never made
a judgment that one side or the other needed to provide an “unexpected”
value. Consequently, some code used (unexpected, actual) while other
code used (actual, unexpected). For consistency with the new EXPECT_EQ
and ASSERT_EQ usage, as well as consistency with CHECK_NE and DCHECK_NE,
this change also updates these use sites to (actual, unexpected) where
one side can be called “unexpected” as, for example, std::string::npos
can be. Unfortunately, this portion was a manual conversion.
References:
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#binary-comparison77d6b17338https://github.com/google/googletest/pull/713
Change-Id: I978fef7c94183b8b1ef63f12f5ab4d6693626be3
Reviewed-on: https://chromium-review.googlesource.com/466727
Reviewed-by: Scott Graham <scottmg@chromium.org>
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>
Bug: crashpad:81
Change-Id: I3cb115440638df909d1c0cdfd01c824ac0d0b073
Reviewed-on: https://chromium-review.googlesource.com/458592
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Previously on macOS, the test used an OS-specific library function to
recover the original argc and argv. On Linux/Android, it essentially
reimplemented the very code it was testing, which didn’t make for a very
good test. The new approach is to save argc and argv in main() and base
the comparison on that.
Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.*, crashpad_test_test MainArguments.*
Change-Id: I578abed3b04ae10a22f79a193bbb8b6589276c97
Reviewed-on: https://chromium-review.googlesource.com/456798
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Joshua Peraza <jperaza@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>
Includes an update of mini_chromium to 3a2d52d74c9a:
3a2d52d74c9a Use O_CLOEXEC (and O_NOCTTY) when calling open()
BUG=chromium:688362
Change-Id: I2bdf86efe4e6559ecb77492ac5bdc728aa035889
Reviewed-on: https://chromium-review.googlesource.com/447999
Reviewed-by: Scott Graham <scottmg@chromium.org>
As brought up in https://codereview.chromium.org/2475863004/, there's
the potential for failed startup if StartHandlerProcess() hangs for
whatever reason. Add a timeout to the wait function so that this case
can attempt to log an error.
R=mark@chromium.org
BUG=655788, 656800, 565063
Change-Id: Ib08cd0641daa6a6cefabb773ffe470227b51958c
Reviewed-on: https://chromium-review.googlesource.com/419060
Reviewed-by: Mark Mentovai <mark@chromium.org>
__has_feature() is a Clang-ism not implemented by GCC.
base/compiler_specific.h provides a HAS_FEATURE() macro that always
returns 0 when __has_feature() is not implemented. Use this macro for
compatibility with GCC and other compilers that do not implement this
Clang extension.
http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension
For GCC’s Address Sanitizer implementation, test the
__SANITIZE_ADDRESS__ macro that it provides as an alternative to
__has_feature(address_sanitizer).
Note that in Chrome builds, ADDRESS_SANITIZER is pushed in by the build
system. The definition of ADDRESS_SANITIZER provides another way for
that macro to be set. It’s supplementary, not exclusive.
cb33b24372/build/config/BUILD.gn (118)
BUG=crashpad:30
Change-Id: I5c3145d29bbc966925369c03a37b1ecb5622a004
Reviewed-on: https://chromium-review.googlesource.com/413109
Reviewed-by: Robert Sesek <rsesek@chromium.org>
The database settings object’s last_upload_attempt_time (time_t) field
is switched from uint64_t to int64_t, for better compatibility with
time_t, which is normally a signed type. This change should be
transparent, as there should be no valid high-bit-set 64-bit timestamps
in this field in the wild.
A number of improvements are made to crashpad_database_util’s time
handling. Errors are checked during time conversion.
--set-last-upload-attempt-time=now is a new supported (and documented)
option.
A StringToNumber() overload for int64_t, along with a test, is added to
aid in crashpad_database_util’s time conversions from numeric strings. A
test is also added for the previously-untested uint64_t implementation.
TEST=crashpad_util_test StringNumberConversion.*
Change-Id: I089c4bf7b95f5df0982bdbb3c27b4f6a89db966e
Reviewed-on: https://chromium-review.googlesource.com/410068
Reviewed-by: Robert Sesek <rsesek@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>
Use “macOS” as the generic unversioned name of the operating system in
comments. For version-specific references, use Mac OS X through 10.6, OS
X from 10.7 through 10.11, and macOS for 10.12.
Change-Id: I1ebee64fbf79200bc799d4a351725dd73257b54d
Reviewed-on: https://chromium-review.googlesource.com/408269
Reviewed-by: Robert Sesek <rsesek@chromium.org>
This defines the global (per-module) CrashpadInfo structure properly on
Linux/Android, located via the “crashpad_info” section name.
Per the ELF specification, section names with a leading dot are reserved
for the system. Reading that, I realized that the same is true of Mach-O
sections with leading underscores, so this renames the section as used
on Mach-O from __DATA,__crashpad_info to __DATA,crashpad_info.
This change is sufficient to successfully build crashpad_client as a
static library on Linux/Android, but the library is incomplete. There’s
no platform-specific database implementation, no CaptureContext() or
CRASHPAD_SIMULATE_CRASH() implementation, and most notably, no
CrashpadClient implementation.
BUG=crashpad:30
Change-Id: I29df7b0f8ee1c79bf8a19502812f59d4b1577b85
Reviewed-on: https://chromium-review.googlesource.com/406427
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>
The default filename rules do not match .S or .asm, so the
platform-specific assembler implementations of CaptureContext() were not
being affirmatively excluded from other platforms’ builds. This
previously worked without causing problems because the Mac build
environment didn’t know what to do with .asm files, and the Windows
build environment didn’t know what to do with .S files. Now that another
platform that may understand .S files is being added, the rules for when
to build these files must be tailored a bit more tightly.
BUG=crashpad:30
Change-Id: Ib62e619c007320d45279c104b3e229d92698aa72
Reviewed-on: https://chromium-review.googlesource.com/406348
Reviewed-by: Robert Sesek <rsesek@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>
Three new metrics:
- counting upload success/failure;
- enum tracking the reason upload was skipped;
- enum describing how an upload got to the pending state.
R=mark@chromium.org, asvitkine@chromium.org
BUG=crashpad:100
Change-Id: I5e0cbc1ac3424e974f3a51560e5cdad484ffc038
Reviewed-on: https://chromium-review.googlesource.com/388855
Reviewed-by: Mark Mentovai <mark@chromium.org>
Solves two problems with having the macros inline:
1. Deduplicates some of the logic (in this case, the name of the
histogram, and whether it should be divided by 1024);
2. More useful check for compilation. As the macros are no-ops in
Crashpad, it was easy to use the wrong name for a variable in the
arguments to the macros (see .mm!)
This way, we have some better chance of at least having code that
compiles when built in Chromium if all the arguments are passed to
Metrics::Something() in a standalone build.
Also rolls mini_chromium DEPS to include:
99213eb Mark histogram arguments as unused to avoid warnings
R=mark@chromium.org
BUG=crashpad:100
Change-Id: I9f7fc3b85854fd61c1ebdf0084d728a7b690c2f1
Reviewed-on: https://chromium-review.googlesource.com/380445
Reviewed-by: Mark Mentovai <mark@chromium.org>
Add a first example of a UMA entry to have it available to try to plumb
through to Chromium.
Adds LoggingFileSizeByHandle() to util/file/file_io.* to retrieve the
size of on disk file to report to UMA.
Also rolls DEPS for mini_chromium to include:
b5ec9ce Add stub versions of histogram_macros.h
R=mark@chromium.org
BUG=crashpad:100
Change-Id: Ib8e96ad4b7d715b46d2c71810c95c92965a89821
Reviewed-on: https://chromium-review.googlesource.com/338821
Reviewed-by: Mark Mentovai <mark@chromium.org>
In order to allow on-demand uploads for crash reports, adding a
upload_explicitly_requested bit on 'pending' state and necessary support
for it.
BUG=chromium:620762
Change-Id: Ida38e483fe8d0e48eb5cbe95e8b8bfd96a2f8f00
Reviewed-on: https://chromium-review.googlesource.com/367328
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
This switches the default behaviour of crashpad_handler.exe to be a
/subsystem:windows app, so that normal usage won't cause a console to be
popped up. At the same time, creates a copy of crashpad_handler.exe in
the output dir named crashpad_handler.com. The .com doesn't affect
normal operation, as the way StartHandler() uses CreateProcess()
requires a real path to a file. However, when run from a command prompt,
.com are found before .exe, so editbin the .com to be to a console app,
which will be run in preference to the exe when run as just
"crashpad_handler", as one tends to do from a command prompt when
debugging. That is:
d:\src\crashpad\crashpad\out\Debug>where crashpad_handler
d:\src\crashpad\crashpad\out\Debug\crashpad_handler.com
d:\src\crashpad\crashpad\out\Debug\crashpad_handler.exe
d:\src\crashpad\crashpad\out\Debug>crashpad_handler --help
Usage: crashpad_handler [OPTION]...
...
d:\src\crashpad\crashpad\out\Debug>crashpad_handler.exe --help
<no output>
d:\src\crashpad\crashpad\out\Debug>crashpad_handler.com --help
Usage: crashpad_handler.com [OPTION]...
...
We also use the .com file in test invocations so that output streams
will be visible.
R=mark@chromium.org
Change-Id: I1a27f88472d491b2a1d76e63c45e6415d9f679c0
Reviewed-on: https://chromium-review.googlesource.com/371578
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>
Add a user-configurable cap on the amount of memory that is gathered by
dereferencing thread stacks. (SyzyAsan stores a tremendously large
number of pointers on the stack, so the dumps were ending up in the ~25M
range.)
Also reduce the range around pointers somewhat.
Change-Id: I6bce57d86bd2f6a796e1580c530909e089ec00ed
Reviewed-on: https://chromium-review.googlesource.com/338463
Reviewed-by: Mark Mentovai <mark@chromium.org>
Forgot some u number suffixes.
BUG=crashpad:94
Change-Id: I2a3b47b4eab07bf5b9ced3859f5a8b388a840b35
Reviewed-on: https://chromium-review.googlesource.com/329760
Reviewed-by: Robert Sesek <rsesek@chromium.org>
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 .
FILE_TYPE_CHAR handles can't be inherited via
PROC_THREAD_ATTRIBUTE_HANDLE_LIST, or CreateProcess() fails with
GetLastError() == 1450 on Windows 7.
I confirmed that an fprintf(stderr, ...) in HandlerMain() does make it
to the console when running tests even after this.
See bug for more discussion.
R=mark@chromium.org
BUG=crashpad:77
Review URL: https://codereview.chromium.org/1473793002 .
