933 Commits

Author SHA1 Message Date
Joshua Peraza
fa8ef92dc7 linux: Add ProcessMemory which reads another process' memory
Provides Read, ReadCString, and ReadCStringSizeLimited. Does not provide
ReadMapped because Linux does not support mmap on /proc/pid/mem.

Bug: crashpad:30
Change-Id: Ia319c0107b1f138aeb8e5d0ee480c77310df7202
Reviewed-on: https://chromium-review.googlesource.com/459700
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>
2017-04-03 21:41:51 +00:00
Mark Mentovai
4688351623 “Promote” test::Paths::Executable() to Paths::Executable()
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>
2017-04-03 18:58:01 +00:00
Mark Mentovai
c39e4dc976 mac: Remove obsolete comment about mach_msg_header_t::msgh_reserved
mig-generated server dispatch routines used to not clear this field in
reply messages prepared from request messages. This oversight was
corrected in the migcom in bootstrap_cmds-96 (macOS 10.12 and Xcode
8.0). Maybe someone at Apple saw the admonishing comment that we had
left here. This comment can now be removed.

Change-Id: I73d965705a2ff5788afb59dd8ecdf4afe58ee47e
Reviewed-on: https://chromium-review.googlesource.com/465687
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-04-03 15:55:30 +00:00
Mark Mentovai
385fe6615f posix: DCHECK for (addr + len) overflow in ScopedMmap::ResetAddrLen()
This also enhances ScopedMmapDeathTest.Mprotect to better ensure that
ScopedMmap::Mprotect() works properly.

Bug: crashpad:30
Test: crashpad_util_test ScopedMmap*.*
Change-Id: Iff35dba9fa993086f3f4cd8f4a862d802e637bb1
Reviewed-on: https://chromium-review.googlesource.com/464547
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-31 16:23:21 +00:00
Mark Mentovai
aa2bc55777 posix: Allow ScopedMmap::ResetAddrLen() to deal with overlap
It should be possible to shrink a region already supervised by
ScopedMmap, or in rare cases when ScopedMmap is supervising only a
smaller portion of an overall larger region, increase the size of the
region it supervises. This is now equivalent to the operation of
base::mac::ScopedMachVM::reset().

The Reset() and ResetAddrLen() methods are upgraded from a void return
to a bool return to indicate their success.

Bug: crashpad:30
Test: crashpad_util_test ScopedMmap*.ResetAddrLen_*
Change-Id: I564e154cd2387e8df3f83b416ecc1c83c9bcf71d
Reviewed-on: https://chromium-review.googlesource.com/464286
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-31 02:54:23 +00:00
Mark Mentovai
1a6ae8ce0b posix: Don’t accumulate zombies in ProcessInfo.Forked test
Use test::Multiprocess, which ensures that waitpid() is called to reap
child processes.

Previously, after a several thousand iterations (using --gtest_repeat),
fork() would begin failing with EAGAIN:

[ RUN      ] ProcessInfo.Forked
../../util/posix/process_info_test.cc:165: Failure
Expected: (pid) >= (0), actual: -1 vs 0
fork: Resource temporarily unavailable (35)
[  FAILED  ] ProcessInfo.Forked (0 ms)

Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.Forked
Change-Id: Ia95c9297d5eeb02894f58844ced1b50981870cbc
Reviewed-on: https://chromium-review.googlesource.com/461482
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-28 22:18:38 +00:00
Mark Mentovai
3127026a95 Convert generate_doxygen.sh to generate_doxygen.py
This makes it easier to generate Doxygen documentation on Windows.

Change-Id: I14c203d2618d8321d5a94d836de434bbaa21c3c9
Reviewed-on: https://chromium-review.googlesource.com/461403
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-28 19:24:57 +00:00
Mark Mentovai
96dc950eaf posix: Add ScopedMmap for managing memory-mapped regions
This wraps mmap(), munmap(), and mprotect().

Bug: crashpad:30
Test: crashpad_util_test ScopedMmap.*
Change-Id: If14363dfd00e314482cc91e53c7f4e3df737b0d3
Reviewed-on: https://chromium-review.googlesource.com/461361
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-28 18:30:14 +00:00
Mark Mentovai
d6837b2b86 mac: Fix SystemSnapshotMacTest.TimeZone on macOS 10.12.4
macOS 10.12.4 includes an updated timezone database. Abbreviations for
Australia/Eucla (formerly ACWST, now +0845) and Australia/Lord_Howe
(formerly LHST/LHDT, now +1030/+11) were dropped in IANA TZ 2017a. The
test is updated so that the abbreviations for these two time zones are
no longer checked.

References:
a25d615495
https://mm.icann.org/pipermail/tz/2017-February/024837.html

Test: crashpad_snapshot_test SystemSnapshotMacTest.TimeZone
Change-Id: I2845c6aee7b7b6a8fcdc6faa4d5cefe5e0f72e5c
Reviewed-on: https://chromium-review.googlesource.com/461500
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-28 18:20:28 +00:00
Mark Mentovai
449dfc4b5d Remove BUG_LINE_FORMAT from codereview.settings
The Bug: style (a Gerrit footer) is used by git-cl for Gerrit changes as
of 3a16ed155e3f.

Bug: chromium:681184
Change-Id: I58c29b6908aee57c7f03374180148f241af91b22
Reviewed-on: https://chromium-review.googlesource.com/461481
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-28 17:49:50 +00:00
Mark Mentovai
f14eda221f win: Be more careful about exit codes in end_to_end_test.py
This is like 270490ff79df, but for things run by end_to_end_test.py, and
things run for it by crash_other_program.exe.

Change-Id: Iabf3c762c50f41eb61ab31f714c646364196e745
Reviewed-on: https://chromium-review.googlesource.com/458822
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-24 22:34:48 +00:00
Mark Mentovai
6c8a3f7007 win: Use a semaphore for synchronization in ProcessSnapshotTest
Bug: crashpad:160
Test: crashpad_snapshot_test ProcessSnapshotTest.CrashpadInfoChild*
Change-Id: Id9bd1ee6ec71c57ecab2ccba6106d41446d66902
Reviewed-on: https://chromium-review.googlesource.com/458879
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-24 21:54:17 +00:00
Mark Mentovai
270490ff79 win: Be more careful about child process exit codes
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>
2017-03-24 20:12:31 +00:00
Sigurdur Asgeirsson
542a91e20e Fix the race causing flaky CrashpadClient tests.
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>
2017-03-24 19:46:24 +00:00
Scott Graham
9104d53d1c Fix thread startup in snapshot reader test
The use of InterlockedCompareExchange() was very wrong. Improved error
checking coming in another CL from mark@, see linked bug for discussion.

Bug: crashpad:160
Change-Id: Id230af6f37c6cdce807dd4d8aba9d33e9bdeffd0
Reviewed-on: https://chromium-review.googlesource.com/459230
Commit-Queue: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
2017-03-24 19:32:04 +00:00
Mark Mentovai
8e37886d41 linux: Don’t make assumptions about the CPU revision
This code that works out the name of the CPU being built for is most
likely going to move out to be used more generally and for Android. It
should nail down the CPU name correctly when possible. Previously,
32-bit x86 always showed up as “i686” and 32-bit ARM always showed up as
“armv7l”.

Bug: crashpad:30
Change-Id: Ifd4b91f30062f5ef621a166f77a732dd8a88a58e
Reviewed-on: https://chromium-review.googlesource.com/458118
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-23 20:25:18 +00:00
Mark Mentovai
f8ef834ef5 android: Support “unified headers” with GCC build
Previously, the __ANDROID_API__ macro was provided by
<android/api-level.h>. With unified headers (expected to become the
default in the next NDK release and the sole option in the subsequent
release), this macro is not set properly by headers. When building with
Clang, the standalone toolchain’s clang and clang++ wrappers set the
macro properly. GCC isn’t accounted for in this way, so the build system
must assume the responsibility of setting it.

This change fishes the value of __ANDROID_API__ out of the standalone
toolchain’s clang wrapper and sets the android_api_level GYP variable
appropriately. From there, it will be picked up by common.gypi in
mini_chromium and used to define __ANDROID_API__.

This updates mini_chromium to 62e6015f633dd4acb1610db15a064889315cadaa
which understands this new GYP variable.

62e6015f633d android: Support “unified headers” with GCC build

https://android.googlesource.com/platform/ndk/+/master/docs/UnifiedHeaders.md
https://android.googlesource.com/platform/ndk/+/ndk-r14/CHANGELOG.md

Bug: crashpad:30
Change-Id: I33e66eba8394e32ced8dca80c8226b85e0e786f3
Reviewed-on: https://chromium-review.googlesource.com/458021
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-23 19:20:35 +00:00
Mark Mentovai
cedfd7b9cd android: Don’t use OPEN_MAX
NDK r13 and earlier provided a bogus definition of OPEN_MAX, but it was
removed from NDK r14 effective in a future API level. It is also not
available when using a standalone toolchain with unified headers.

ff5f17bc8a

Bug: crashpad:30
Change-Id: Ic89d6879cb1a4e5b9d20e9cb06bedd5176df0f2a
Reviewed-on: https://chromium-review.googlesource.com/458121
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-23 18:45:17 +00:00
Sigurdur Asgeirsson
810d4815df minidump: Allow for user extension streams computed at crash time
Bug: crashpad:167
Change-Id: I94c143353482f100a31d6afb0685b38757a662d6
Reviewed-on: https://chromium-review.googlesource.com/455976
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
2017-03-23 13:10:57 +00:00
Mark Mentovai
fa3413e14a GCC fix for -Wunused-but-set-variable
While compiling minidump_module_writer_test.cc:

minidump/minidump_module_writer_test.cc: In member function 'virtual void crashpad::test::{anonymous}::MinidumpModuleWriter_InitializeFromSnapshot_Test::TestBody()':
minidump/minidump_module_writer_test.cc:656:15: error: variable 'module_names' set but not used [-Werror=unused-but-set-variable]
   const char* module_names[arraysize(expect_modules)] = {};
               ^

Bug: crashpad:30
Change-Id: Ie6bcbced67c947ba6cca32a7057a8ac6de4d0e5a
Reviewed-on: https://chromium-review.googlesource.com/457958
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-23 02:52:26 +00:00
Mark Mentovai
7a7815506b GCC fix: Don’t use arraysize() on packed structs
These were previously fixed in f83530bf9a0b for some targets, but not
crashpad_minidump_test.

While compiling minidump_misc_info_writer_test.cc:

In file included from minidump/minidump_misc_info_writer.h:26:0,
                 from minidump/minidump_misc_info_writer_test.cc:15:
minidump/minidump_misc_info_writer_test.cc: In member function ‘virtual void crashpad::test::{anonymous}::MinidumpMiscInfoWriter_TimeZone_Test::TestBody()’:
minidump/minidump_misc_info_writer_test.cc:401:39: error: cannot bind packed field ‘expected.MINIDUMP_MISC_INFO_3::TimeZone.TIME_ZONE_INFORMATION::StandardName’ to ‘short unsigned int (&)[32]’
           arraysize(expected.TimeZone.StandardName));
                     ~~~~~~~~~~~~~~~~~~^
third_party/mini_chromium/mini_chromium/base/macros.h:41:50: note: in definition of macro ‘arraysize’
 #define arraysize(array) (sizeof(ArraySizeHelper(array)))
                                                  ^~~~~

Bug: crashpad:30
Change-Id: I2a1c3b356c0064e8161ec70a9ac156053fc28df7
Reviewed-on: https://chromium-review.googlesource.com/457881
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-23 02:40:32 +00:00
Mark Mentovai
013d5e14a3 #include <stddef.h> where offsetof() is used
Bug: crashpad:30
Change-Id: If23ca9ea3141d3d34dc494aa29a1bd1dc8f83130
Reviewed-on: https://chromium-review.googlesource.com/458079
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-23 02:15:32 +00:00
Mark Mentovai
db8c54e142 android: Add gyp_crashpad_android.py for easier Android development
Bug: crashpad:30
Change-Id: Idf7e6db944bf946e6571064306897848222cd36f
Reviewed-on: https://chromium-review.googlesource.com/458078
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-23 00:15:26 +00:00
Mark Mentovai
cca10659c7 android: Fix gmock-using tests’ use of MOCK_METHODn() with clang
This was already addressed by disabling a warning, but was only
effective for macOS and non-Android Linux. The comment for the existing
fix, which is now being applied to Android:

> The MOCK_METHODn() macros do not specify “override”, which triggers
> this warning in users: “error: 'Method' overrides a member function
> but is not marked 'override'
> [-Werror,-Winconsistent-missing-override]”.  Suppress these warnings,
> and add -Wno-unknown-warning-option because only recent versions of
> clang (trunk r220703 and later, version 3.6 and later) recognize it.

Also see https://crbug.com/428099.

The errors being encountered since 3983b80ca2fc were:

util/file/file_reader_test.cc:48:23: error: 'Read' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  FileOperationResult Read(void* data, size_t size) {
                      ^
util/file/file_reader.h:39:31: note: overridden virtual function is here
  virtual FileOperationResult Read(void* data, size_t size) = 0;
                              ^
util/file/file_reader_test.cc:53:16: error: 'Seek' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
  MOCK_METHOD2(Seek, FileOffset(FileOffset, int));
               ^
util/file/file_seeker.h:31:22: note: overridden virtual function is here
  virtual FileOffset Seek(FileOffset offset, int whence) = 0;
                     ^

Bug: crashpad:30
Test: crashpad_util_test FileReader.*
Change-Id: I10894efdafc0da965e3780219f2e4c1f13f9b99e
Reviewed-on: https://chromium-review.googlesource.com/458060
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-22 23:49:31 +00:00
Mark Mentovai
af66c4b740 Add overview design doc link to root README.md
Use an underscore instead of a hyphen in the overview design doc’s
filename for consistency with the rest of the files in the repository.

Change-Id: I15a76a00709a43dfec60e7f4f6ee64a3cd031b2c
Reviewed-on: https://chromium-review.googlesource.com/458537
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
2017-03-22 19:33:51 +00:00
Sigurdur Asgeirsson
c1157e87f1 Crashpad overview design doc
Change-Id: I4901025443885025bbf77f60b7c787f02eaadf60
Reviewed-on: https://chromium-review.googlesource.com/458259
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
2017-03-22 19:01:23 +00:00
Mark Mentovai
e4cad9e514 doc: Standardize on “macOS” in comments
952f787f4aab missed two occurrences that should have been updated.

Change-Id: I425367689eb19edfd309a2210a79ed400e190673
Reviewed-on: https://chromium-review.googlesource.com/458116
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-22 17:26:14 +00:00
Mark Mentovai
3983b80ca2 util/file: Handle oversized reads and writes gracefully
file_io and the FileReader family had a few loose ends regarding big
reads and writes. It’s not likely that we’ve experienced these
conditions yet, but they’d be likely to appear in a potential future
involving full memory dumps. This specifies the behavior with large
reads and writes, consolidates some logic, and improves some interfaces.

ReadFile() should always return without retrying after a short read, and
in fact does return after short reads since 00b64427523b. It is
straightforward to limit the maximum read size based on a parameter
limitation of the underlying operation, or a limitation of the type used
for FileOperationResult.

In contrast, WriteFile() should always retry after a short write,
including a write shortened because of a parameter limitation of the
underlying operation, or a limitation of the type used for
FileOperationResult. This allows its return value to be simplified to a
“bool”.

The platform-specific WriteFile() code has been moved to
internal::NativeWriteFile(), and the platform-independent loop that
retries following a short write has been refactored into
internal::WriteAllInternal so that it can be used by a new test.

The platform-agnostic ReadFileExactlyInternal() implementation has been
refactored into internal::ReadExactlyInternal so that it can be used by
a new test and by FileReaderInterface::ReadExactly(), which had a nearly
identical implementation.

Test: crashpad_util_test FileIO.ReadExactly_*:FileIO.WriteAll_*:FileReader.ReadExactly_*
Change-Id: I487450322ab049c6f2acd4061ea814037cc9a864
Reviewed-on: https://chromium-review.googlesource.com/456824
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-22 02:34:41 +00:00
Mark Mentovai
39f13a77a4 Make run_tests.py work with paths instead of configuration names
This is co-dependent with
https://chromium-review.googlesource.com/457736.

I’ve been trying to share a source tree between different platforms,
using gyp_crashpad.py --generator-output to put the build output in the
right place. On Windows, it winds up in
out\win\out\{Debug,Release}{,_x64}. This makes it tricky to use
run_tests.py, which expects to find build output right in the “out”
directory. It’s not impossible to use, because you can tell it
“win\out\Debug_x64”, but it’s really awkward to use that path when we
all know that it’s not relative to anything that makes sense, like the
current directory.

This simplifies run_tests.py to work directly with the
configuration-specific output directory. For most users, this means
including “out/” or “out\” when running the script.

Bug: chromium:703890
Change-Id: Ic7de82fabd2adda7ae00558844cb3ce91aa4a5ed
Reviewed-on: https://chromium-review.googlesource.com/457716
Commit-Queue: Mark Mentovai <mark@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
2017-03-22 01:45:16 +00:00
Mark Mentovai
542306626d minidump: Make the MemoryListStream the caboose once again
After b10d9118dea4, the MemoryListStream was moved from its preferred
position as the last stream in the file to precede user minidump
streams, in an effort to prevent it from being preempted by a user
minidump stream that specified the memory list stream’s type. A better
solution, which keeps all streams where they want to be, is to put the
MemoryListStream at the end, put user streams before it, and omit user
streams that purport to be a MemoryListStream.

Bug: crashpad:171
Change-Id: I6974fbd4c9ec67284f86c593c553af7adf73601b
Reviewed-on: https://chromium-review.googlesource.com/456823
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
2017-03-20 17:32:01 +00:00
Mark Mentovai
bc5b7b06db Fix racy WorkerThread test
WorkDelegate::DoWork() can be called more times than the value set by
WorkDelegate::SetDesiredWorkCount(). The main test thread may not be
able to “squeeze” its call to WorkerThread::Stop() in after its
WorkDelegate::WaitForWorkCount() returns. If the worker thread cannot be
stopped in time, one or more additional iterations of
WorkDelegate::DoWork() can run. WorkDelegate::DoWork() should take care
to not increment work_count_ beyond the desired value.

Bug: crashpad:169
Test: crashpad_util_test WorkerThread.*
Change-Id: I9e261a2a8a57420e12c0f1c9abd0ee6304dacd53
Reviewed-on: https://chromium-review.googlesource.com/456821
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-20 17:16:50 +00:00
Mark Mentovai
14138936b5 test: Compare ProcessInfo::Arguments() to main()’s argc/argv on POSIX
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>
2017-03-20 17:05:30 +00:00
Mark Mentovai
88bc09fb86 posix: Fix StdioFileHandle() for GCC
With GCC 6.3:

util/file/file_io_posix.cc: In function ‘crashpad::FileHandle crashpad::StdioFileHandle(crashpad::StdioStream)’:
util/file/file_io_posix.cc:193:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

Bug: crashpad:30
Change-Id: I03111b672ab7f796103ef61ea3d126fc25571390
Reviewed-on: https://chromium-review.googlesource.com/456820
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-20 16:39:45 +00:00
Mark Mentovai
4f90f15146 Remove WeakStdioFileReader and WeakStdioFileWriter
These classes were a bit of a hack, and one of the the reasons that
WeakStdioFileReader was introduced, accurate detection of EOF when stdin
is a terminal, will be obsolete once
https://chromium-review.googlesource.com/456676/ lands. In fact,
WeakStdioFileReader didn’t even work properly for this purpose on
Windows.

Use WeakFile{Reader,Writer} in place of these classes (there were only
two use sites). Provide a StdioFileHandle() function to access the
proper values to use as a FileHandle for native file I/O given each OS’
own interface.

Change-Id: I35e8d49982162bb9813855f41739cc77597ea74d
Reviewed-on: https://chromium-review.googlesource.com/456358
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-16 20:21:19 +00:00
Mark Mentovai
00b6442752 Make file_io reads more rational and predictable
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>
2017-03-16 20:07:43 +00:00
Mark Mentovai
51b21d8874 Add DelimitedFileReader and use it in Linux/Android’s ProcessInfo
This implements a non-stdio-based getline() equivalent. getline() is not
in the Android NDK until API 21 (Android 5.0.0), while Chrome builds for
32-bit platforms with API 16 (Android 4.1.0). Although a getline()
declaration could be provided in compat for use with older NDK headers,
it’s desirable to move away from stdio entirely. The C++
DelimitedFileReader interface is also a bit more comfortable to use than
getline().

A getdelim() equivalent is also provided, and is also used in the
Linux/Android ProcessInfo implementation.

Bug: crashpad:30
Test: crashpad_util_test FileLineReader.*:ProcessInfo.*
Change-Id: Ic1664758a87cfe4953ab22bd3ae190761404b22c
Reviewed-on: https://chromium-review.googlesource.com/455998
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-16 17:42:09 +00:00
Mark Mentovai
b10d9118de minidump: Ignore attempts to add user streams with type collisions
The unconditional CHECK() in MinidumpFileWriter::AddStream() made sense
when all streams were under the Minidump class family’s control, but
became hazardous upon the introduction of user streams with arbitrary
types under the crashy process’ control.

Bug: crashpad:171
Test: crashpad_minidump_test MinidumpFileWriter.SameStreamType
Change-Id: Iba5be08b330261286d11d22d8e9a2fef5fcc1070
Reviewed-on: https://chromium-review.googlesource.com/456056
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
2017-03-15 19:48:38 +00:00
Mark Mentovai
82009cd14d android: Support builds with NDK API versions older than 21 (compat)
The new Linux ProcessInfo implementation uses two macros not readily
available in NDK API versions older than 21 (Android 5.0.0): NT_PRSTATUS
and PR_GETREGSET.

Chrome uses API 21 for 64-bit builds, but uses API 16 for 32-bit builds.

NT_PRSTATUS is normally defined by <elf.h> or by <linux/elf.h>, included
by <elf.h>. Although the definition in <linux/elf.h> is available in
older NDK API versions, this internal header does not mix well with
<elf.h> unless <elf.h> contemplates this combination. As of NDK API 21,
<elf.h> actually delegates most of its work to <linux/elf.h>.

PR_GETREGSET is not available in the NDK at all until API 21. Its
definition is in <linux/ptrace.h>. Most user code should #include
<sys/ptrace.h> instead, which includes <linux/ptrace.h>.

Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.*
Change-Id: I4d07a9964db4665a49bde490e905ae9126880bc5
Reviewed-on: https://chromium-review.googlesource.com/455659
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-15 17:48:12 +00:00
Mark Mentovai
5938c6e993 linux: Support ProcessInfo::Is64Bit() for ARM on pre-3.5.0 Linux
The PTRACE_GETREGSET ptrace() request is not supported on ARM before
Linux 3.5.0. This request was only used to determine the bitness of the
target process. Since 64-bit ARM is only supported as of Linux 3.7.0,
when this request is not supported on 32-bit ARM, 64-bit is also not
supported, and the target process must be a 32-bit process.

Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.*
Change-Id: Ib004d24858f146df898dfa6796926d97e2510541
Reviewed-on: https://chromium-review.googlesource.com/455398
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-15 17:36:52 +00:00
Mark Mentovai
d7467ba7e4 linux: Use user_regs instead of pt_regs for 32-bit ARM in ProcessInfo
Not all libc implementations reliably expose pt_regs from
<sys/ptrace.h>. glibc-2.25/sysdeps/generic/sys/ptrace.h, for example,
does not #include <asm/ptrace.h> (which defines the structure) or
anything else that would #include that file such as <linux/ptrace.h>. On
the other hand, Android 7.1.1 bionic/libc/include/sys/ptrace.h does
#include <linux/ptrace.h>.

It is not viable to #include <asm/ptrace.h> or <linux/ptrace.h>
directly: it would be natural to #include them, sorted, before
<sys/ptrace.h> but this causes problems for glibc’s <sys/ptrace.h>.
Constants like PTRACE_GETREGS and PTRACE_TRACEME are simple macros in
<asm/ptrace.h> and <linux/ptrace.h>, respectively, but are defined in
enums in glibc’s <sys/ptrace.h>, and this doesn’t mix well. It is
possible to #include <asm/ptrace.h> (but not <linux/ptrace.h>) after
<sys/ptrace.h>, but because this involves same-value macro redefinitions
and because it reaches into internal headers, it’s not preferred.

The alternative approach taken here is to use the user_regs structure
from <sys/user.h>, which is reliably defined by both Bionic and glibc,
and has the same layout as the kernel’s pt_regs structure. (All that
matters in this code is the size of the structure.) See Android 7.1.1
bionic/libc/include/sys/user.h,
glibc-2.25/sysdeps/unix/sysv/linux/arm/sys/user.h, and
linux-4.9.15/arch/arm/include/asm/ptrace.h for the various equivalent
definitions.

Take the same approach for 64-bit ARM: use user_regs_struct from
<sys/user.h> in preference to hoping for a C library’s <sys/ptrace.h> to
somehow provide the kernel’s user_pt_regs.

This mirrors the approach already being used for x86 and x86_64, which
use the C library’s <sys/user.h> user_regs_struct.

Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.*
Change-Id: I3067e32c7fa4d6c8f4f2d5b63df141a0f490cd13
Reviewed-on: https://chromium-review.googlesource.com/455558
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
2017-03-15 17:11:48 +00:00
Mark Mentovai
9be4745be0 linux: Lazily initialize ProcessInfo’s Is64Bit() and StartTime()
Lazy initialization is particularly beneficial for Is64Bit(), which uses
a different (ptrace()-based) approach than the rest of the class (which
is /proc-based). It is possible for the /proc-based Initialize() to
succeed while ptrace() would fail, as it typically would in the
ProcessInfo.Pid1 test. Because this test does not call Is64Bit(),
permission to ptrace() shouldn’t be necessary, and in fact ptrace()
shouldn’t even be called.

This enables the ProcessInfo.Pid1 test on Android (due to ptrace(), it
was actually failing on any Linux, not just Android). It also enables
the ProcessInfo.Forked test on non-Linux, as the prctl(PR_SET_DUMPABLE)
Linux-ism can be removed from it.

Bug: crashpad:30
Test: crashpad_util_test ProcessInfo.*
Change-Id: Ic883733a6aed7e7de9a0f070a5a3544126c7e976
Reviewed-on: https://chromium-review.googlesource.com/455656
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-15 16:01:27 +00:00
Mark Mentovai
48781dc182 linux: Fix process start time computation
The process start time in ticks was being converted to an integer from a
temporary string that had gone out of scope by the time the conversion
was performed.

It was possible for a format error in /proc/pid/stat to go undetected
and result in a buffer overflow.

Bug: crashpad:30
Change-Id: I03566dda797bc1f23543bfffcfdb2c5ffe1eca66
Reviewed-on: https://chromium-review.googlesource.com/455378
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-15 02:08:12 +00:00
Mark Mentovai
bad4fd0011 linux: Fix ProcessInfo for x86[_64]
This configuration uses user_regs_struct, which is declared in
<sys/user.h>.

Bug: crashpad:30
Change-Id: Ibdcc60c6719fc2bad9fbeef116efbe764229e14b
Reviewed-on: https://chromium-review.googlesource.com/455197
Reviewed-by: Joshua Peraza <jperaza@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-14 22:20:33 +00:00
Joshua Peraza
87c75552ad Implement ProcessInfo for Linux/Android
Bug: crashpad:30
Change-Id: I45853a96cdbe94a2dbf3fa265b015170badb1bbb
Reviewed-on: https://chromium-review.googlesource.com/446903
Reviewed-by: Mark Mentovai <mark@chromium.org>
2017-03-14 20:10:17 +00:00
Mark Mentovai
3eaee58970 doc: Update Android developer documentation for NDK 14
NDK 14 is current. Update the developer documentation to reflect this.

Recommend using subdirectories of out/ as the Android build output
directory, so that they’ll be ignored by .gitignore.

Bug: crashpad:30
Change-Id: Id1508215b924a3e0cae2c11a61c9c685363c50f6
Reviewed-on: https://chromium-review.googlesource.com/454202
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-14 18:18:07 +00:00
Mark Mentovai
18d70acf81 doc: Link to crashpad_http_upload(1) from the man page index
Change-Id: Iced22f556a08d87a69f589555ad39763fe417805
Reviewed-on: https://chromium-review.googlesource.com/452318
Reviewed-by: Robert Sesek <rsesek@chromium.org>
2017-03-13 18:13:48 +00:00
Mark Mentovai
f590383096 doc: Document the try server and commit queue
Bug: crashpad:162
Change-Id: I9b5dd8c281117c24d79ce6b81269c87d42a21e13
Reviewed-on: https://chromium-review.googlesource.com/452438
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-09 17:41:40 +00:00
Mark Mentovai
a47a512ea6 Use “Bug: crashpad:###” git footer format in change descriptions
This requires depot_tools 57c4721d81da or later. Run “gclient” to
update.

When git-cl opens an editor for a change description that doesn’t
already have a bug line, the default bug line will now be a “Bug:
crashpad:” git footer field.

git footers are a more Gerrit-y way of handling things. It didn’t make
sense to have two distinct metadata footer sections (or more, if they
wound up interleaved). Standardize on the newer format. Bye-bye, BUG=.

Change-Id: I7dade51703f9eff471a49510793d37686ce5fc97
Reviewed-on: https://chromium-review.googlesource.com/452557
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-09 16:57:59 +00:00
Mark Mentovai
b5284cdcba mac: Add CrashpadClient::GetHandlerMachPort()
Bug: chromium:699607
Change-Id: Ib1886550fe81787cb1ffc8d8853f6969cc96831e
Reviewed-on: https://chromium-review.googlesource.com/451127
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-09 00:44:58 +00:00
Mark Mentovai
8e82f6fde0 mac: Update test and comments with feedback from Apple bug
Apple has responded to their bug 29079442 with a resolution stating that
these are not corpse ports but task ports that have changed after
execve(), as part of the large task port and execve() strategy rewrite
from 10.12.1. The comments being replaced were written before we had
10.12.1 source code. Now that we can see what’s going on, revise the
comments, and re-enable the task port check for the non-execve() test
variants.

https://openradar.appspot.com/29079442
https://googleprojectzero.blogspot.com/2016/10/taskt-considered-harmful.html

Bug: crashpad:137
Test: crashpad_snapshot_test MachOImageAnnotationsReader.CrashDyld
Change-Id: I463637816085f4165b92b85a5b98bfeddcdf4094
Reviewed-on: https://chromium-review.googlesource.com/451120
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
2017-03-08 00:35:54 +00:00