From 18d70acf81df49cc10b00bcc67c1ec64e16bd9d0 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 9 Mar 2017 12:28:43 -0500 Subject: [PATCH 01/19] 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 --- doc/man.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/man.md b/doc/man.md index ff5fd413..0344fe7e 100644 --- a/doc/man.md +++ b/doc/man.md @@ -19,6 +19,7 @@ limitations under the License. ## Section 1: User Commands * [crashpad_database_util](../tools/crashpad_database_util.md) + * [crashpad_http_upload](../tools/crashpad_http_upload.md) * [generate_dump](../tools/generate_dump.md) ### macOS-Specific From 3eaee58970c278dfe2dd072bd68e672ee7e811fa Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 13 Mar 2017 23:33:18 -0400 Subject: [PATCH 02/19] doc: Update Android developer documentation for NDK 14 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Mark Mentovai --- doc/developing.md | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/doc/developing.md b/doc/developing.md index 4505d610..e091b894 100644 --- a/doc/developing.md +++ b/doc/developing.md @@ -112,7 +112,7 @@ Kit)](https://developer.android.com/ndk/) runs on. If it’s not already present on your system, [download the NDK package for your system](https://developer.android.com/ndk/downloads/) and expand it to a suitable location. These instructions assume that it’s been expanded to -`~/android-ndk-r13`. +`~/android-ndk-r14`. To build Crashpad, portions of the NDK must be reassembled into a [standalone toolchain](https://developer.android.com/ndk/guides/standalone_toolchain.html). @@ -126,8 +126,8 @@ desired. To build a standalone toolchain targeting 64-bit ARM and API level 21 ``` $ cd ~ -$ python android-ndk-r13/build/tools/make_standalone_toolchain.py \ - --arch=arm64 --api=21 --install-dir=android-ndk-r13_arm64_api21 +$ python android-ndk-r14/build/tools/make_standalone_toolchain.py \ + --arch=arm64 --api=21 --install-dir=android-ndk-r14_arm64_api21 ``` Note that Chrome uses Android API level 21 for 64-bit platforms and 16 for @@ -144,14 +144,14 @@ not be permanent. ``` $ cd ~/crashpad/crashpad -$ CC_target=~/android-ndk-r13_arm64_api21/bin/clang \ - CXX_target=~/android-ndk-r13_arm64_api21/bin/clang++ \ - AR_target=~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-ar \ - NM_target=~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-nm \ - READELF_target=~/android-ndk-r13_arm64_api21/bin/aarch64-linux-android-readelf \ +$ CC_target=~/android-ndk-r14_arm64_api21/bin/clang \ + CXX_target=~/android-ndk-r14_arm64_api21/bin/clang++ \ + AR_target=~/android-ndk-r14_arm64_api21/bin/aarch64-linux-android-ar \ + NM_target=~/android-ndk-r14_arm64_api21/bin/aarch64-linux-android-nm \ + READELF_target=~/android-ndk-r14_arm64_api21/bin/aarch64-linux-android-readelf \ python build/gyp_crashpad.py \ -DOS=android -Dtarget_arch=arm64 -Dclang=1 \ - --generator-output=out_android_arm64_api21 -f ninja-android + --generator-output=out/android_arm64_api21 -f ninja-android ``` It is also possible to use GCC instead of Clang by making the appropriate @@ -174,7 +174,7 @@ build, direct `ninja` to the specific `out` directory chosen by `--generator-output` above. ``` -$ ninja -C out_android_arm64_api21/out/Debug crashpad_test_test +$ ninja -C out/android_arm64_api21/out/Debug crashpad_test_test ``` ## Testing @@ -216,10 +216,10 @@ transferred to the device prior to running the test. ``` $ cd ~/crashpad/crashpad -$ adb push out_android_arm64_api21/out/Debug/crashpad_test_test /data/local/tmp/ +$ adb push out/android_arm64_api21/out/Debug/crashpad_test_test /data/local/tmp/ [100%] /data/local/tmp/crashpad_test_test $ adb push \ - out_android_arm64_api21/out/Debug/crashpad_test_test_multiprocess_exec_test_child \ + out/android_arm64_api21/out/Debug/crashpad_test_test_multiprocess_exec_test_child \ /data/local/tmp/ [100%] /data/local/tmp/crashpad_test_test_multiprocess_exec_test_child $ adb shell mkdir -p /data/local/tmp/crashpad_test_data_root/test From 87c75552ad70cdafc6d32a69d5f6a15e97cfd078 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 14 Mar 2017 13:05:40 -0700 Subject: [PATCH 03/19] Implement ProcessInfo for Linux/Android Bug: crashpad:30 Change-Id: I45853a96cdbe94a2dbf3fa265b015170badb1bbb Reviewed-on: https://chromium-review.googlesource.com/446903 Reviewed-by: Mark Mentovai --- util/posix/process_info.h | 12 + util/posix/process_info_linux.cc | 443 +++++++++++++++++++++++++++++++ util/posix/process_info_test.cc | 42 ++- util/util.gyp | 8 + util/util_test.gyp | 12 + 5 files changed, 513 insertions(+), 4 deletions(-) create mode 100644 util/posix/process_info_linux.cc diff --git a/util/posix/process_info.h b/util/posix/process_info.h index 1aa23d8c..10cee96d 100644 --- a/util/posix/process_info.h +++ b/util/posix/process_info.h @@ -141,6 +141,18 @@ class ProcessInfo { private: #if defined(OS_MACOSX) kinfo_proc kern_proc_info_; +#elif defined(OS_LINUX) || defined(OS_ANDROID) + std::set supplementary_groups_; + timeval start_time_; + pid_t pid_; + pid_t ppid_; + uid_t uid_; + uid_t euid_; + uid_t suid_; + gid_t gid_; + gid_t egid_; + gid_t sgid_; + bool is_64_bit_; #endif InitializationStateDcheck initialized_; diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc new file mode 100644 index 00000000..9921b65b --- /dev/null +++ b/util/posix/process_info_linux.cc @@ -0,0 +1,443 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/posix/process_info.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "base/files/scoped_file.h" +#include "base/logging.h" +#include "base/posix/eintr_wrapper.h" +#include "base/strings/string_number_conversions.h" +#include "base/strings/string_piece.h" +#include "util/file/file_reader.h" +#include "util/string/split_string.h" + +namespace crashpad { + +namespace { + +// If the string |pattern| is matched exactly at the start of |input|, advance +// |input| past |pattern| and return true. +bool AdvancePastPrefix(const char** input, const char* pattern) { + size_t length = strlen(pattern); + if (strncmp(*input, pattern, length) == 0) { + *input += length; + return true; + } + return false; +} + +#define MAKE_ADAPTER(type, function) \ + bool ConvertStringToNumber(const base::StringPiece& input, type* value) { \ + return function(input, value); \ + } +MAKE_ADAPTER(int, base::StringToInt) +MAKE_ADAPTER(unsigned int, base::StringToUint) +MAKE_ADAPTER(uint64_t, base::StringToUint64) +#undef MAKE_ADAPTER + +// Attempt to convert a prefix of |input| to numeric type T. On success, set +// |value| to the number, advance |input| past the number, and return true. +template +bool AdvancePastNumber(const char** input, T* value) { + size_t length = 0; + if (std::numeric_limits::is_signed && **input == '-') { + ++length; + } + while (isdigit((*input)[length])) { + ++length; + } + bool success = ConvertStringToNumber(base::StringPiece(*input, length), + value); + if (success) { + *input += length; + return true; + } + return false; +} + +struct BufferFreer { + void operator()(char** buffer_ptr) const { + free(*buffer_ptr); + *buffer_ptr = nullptr; + } +}; +using ScopedBufferPtr = std::unique_ptr; + +bool ReadEntireFile(const char* path, std::string* contents) { + FileReader file; + if (!file.Open(base::FilePath(path))) { + return false; + } + + char buffer[4096]; + FileOperationResult length; + while ((length = file.Read(buffer, sizeof(buffer))) > 0) { + contents->append(buffer, length); + } + return length >= 0; +} + +void SubtractTimespec(const timespec& t1, const timespec& t2, + timespec* result) { + result->tv_sec = t1.tv_sec - t2.tv_sec; + result->tv_nsec = t1.tv_nsec - t2.tv_nsec; + if (result->tv_nsec < 0) { + result->tv_sec -= 1; + result->tv_nsec += static_cast(1E9); + } +} + +void TimespecToTimeval(const timespec& ts, timeval* tv) { + tv->tv_sec = ts.tv_sec; + tv->tv_usec = ts.tv_nsec / 1000; +} + +} // namespace + +ProcessInfo::ProcessInfo() + : supplementary_groups_(), + start_time_(), + pid_(-1), + ppid_(-1), + uid_(-1), + euid_(-1), + suid_(-1), + gid_(-1), + egid_(-1), + sgid_(-1), + is_64_bit_(false), + initialized_() {} + +ProcessInfo::~ProcessInfo() {} + +bool ProcessInfo::Initialize(pid_t pid) { + INITIALIZATION_STATE_SET_INITIALIZING(initialized_); + + pid_ = pid; + + { + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/status", pid_); + base::ScopedFILE status_file(fopen(path, "re")); + if (!status_file.get()) { + PLOG(ERROR) << "fopen " << path; + return false; + } + + size_t buffer_size = 0; + char* buffer = nullptr; + ScopedBufferPtr buffer_owner(&buffer); + + bool have_ppid = false; + bool have_uids = false; + bool have_gids = false; + bool have_groups = false; + ssize_t len; + while ((len = getline(&buffer, &buffer_size, status_file.get())) > 0) { + const char* line = buffer; + + if (AdvancePastPrefix(&line, "PPid:\t")) { + if (have_ppid) { + LOG(ERROR) << "format error: multiple PPid lines"; + return false; + } + have_ppid = AdvancePastNumber(&line, &ppid_); + if (!have_ppid) { + LOG(ERROR) << "format error: unrecognized PPid format"; + return false; + } + } else if (AdvancePastPrefix(&line, "Uid:\t")) { + if (have_uids) { + LOG(ERROR) << "format error: multiple Uid lines"; + return false; + } + have_uids = + AdvancePastNumber(&line, &uid_) && + AdvancePastPrefix(&line, "\t") && + AdvancePastNumber(&line, &euid_) && + AdvancePastPrefix(&line, "\t") && + AdvancePastNumber(&line, &suid_); + if (!have_uids) { + LOG(ERROR) << "format error: unrecognized Uid format"; + return false; + } + } else if (AdvancePastPrefix(&line, "Gid:\t")) { + if (have_gids) { + LOG(ERROR) << "format error: multiple Gid lines"; + return false; + } + have_gids = + AdvancePastNumber(&line, &gid_) && + AdvancePastPrefix(&line, "\t") && + AdvancePastNumber(&line, &egid_) && + AdvancePastPrefix(&line, "\t") && + AdvancePastNumber(&line, &sgid_); + if (!have_gids) { + LOG(ERROR) << "format error: unrecognized Gid format"; + return false; + } + } else if (AdvancePastPrefix(&line, "Groups:\t")) { + if (have_groups) { + LOG(ERROR) << "format error: multiple Groups lines"; + return false; + } + gid_t group; + while (AdvancePastNumber(&line, &group)) { + supplementary_groups_.insert(group); + if (!AdvancePastPrefix(&line, " ")) { + LOG(ERROR) << "format error"; + return false; + } + } + if (!AdvancePastPrefix(&line, "\n") || line != buffer + len) { + LOG(ERROR) << "format error: unrecognized Groups format"; + return false; + } + have_groups = true; + } + } + if (!feof(status_file.get())) { + PLOG(ERROR) << "getline"; + return false; + } + if (!have_ppid || !have_uids || !have_gids || !have_groups) { + LOG(ERROR) << "format error: missing fields"; + return false; + } + } + + { + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/stat", pid_); + std::string stat_contents; + if (!ReadEntireFile(path, &stat_contents)) { + return false; + } + + // The process start time is the 22nd column. + // The second column is the executable name in parentheses. + // The executable name may have parentheses itself, so find the end of the + // second column by working backwards to find the last closing parens and + // then count forward to the 22nd column. + size_t stat_pos = stat_contents.rfind(')'); + if (stat_pos == std::string::npos) { + LOG(ERROR) << "format error"; + return false; + } + + for (int index = 1; + index < 21 && stat_pos < stat_contents.size(); + ++index) { + stat_pos = stat_contents.find(" ", stat_pos); + ++stat_pos; + } + + const char* ticks_ptr = stat_contents.substr(stat_pos).c_str(); + + // start time is in jiffies instead of clock ticks pre 2.6. + uint64_t ticks_after_boot; + if (!AdvancePastNumber(&ticks_ptr, &ticks_after_boot)) { + LOG(ERROR) << "format error"; + return false; + } + long clock_ticks_per_s = sysconf(_SC_CLK_TCK); + if (clock_ticks_per_s <= 0) { + PLOG(ERROR) << "sysconf"; + return false; + } + timeval time_after_boot; + time_after_boot.tv_sec = ticks_after_boot / clock_ticks_per_s; + time_after_boot.tv_usec = + (ticks_after_boot % clock_ticks_per_s) * + (static_cast(1E6) / clock_ticks_per_s); + + timespec uptime; + if (clock_gettime(CLOCK_BOOTTIME, &uptime) != 0) { + PLOG(ERROR) << "clock_gettime"; + return false; + } + + timespec current_time; + if (clock_gettime(CLOCK_REALTIME, ¤t_time) != 0) { + PLOG(ERROR) << "clock_gettime"; + return false; + } + + timespec boot_time_ts; + SubtractTimespec(current_time, uptime, &boot_time_ts); + timeval boot_time_tv; + TimespecToTimeval(boot_time_ts, &boot_time_tv); + timeradd(&boot_time_tv, &time_after_boot, &start_time_); + } + +#if defined(ARCH_CPU_64_BITS) + const bool am_64_bit = true; +#else + const bool am_64_bit = false; +#endif + + if (pid_ == getpid()) { + is_64_bit_ = am_64_bit; + } else { + if (ptrace(PTRACE_ATTACH, pid_, nullptr, nullptr) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + + if (HANDLE_EINTR(waitpid(pid_, nullptr, __WALL)) < 0) { + PLOG(ERROR) << "waitpid"; + return false; + } + + // Allocate more buffer space than is required to hold registers for this + // process. If the kernel fills the extra space, the target process uses + // more/larger registers than this process. If the kernel fills less space + // than sizeof(regs) then the target process uses smaller/fewer registers. + struct { +#if defined(ARCH_CPU_X86_FAMILY) + using PrStatusType = user_regs_struct; +#elif defined(ARCH_CPU_ARMEL) + using PrStatusType = pt_regs; +#elif defined(ARCH_CPU_ARM64) + using PrStatusType = user_pt_regs; +#endif + PrStatusType regs; + char extra; + } regbuf; + + iovec iov; + iov.iov_base = ®buf; + iov.iov_len = sizeof(regbuf); + if (ptrace(PTRACE_GETREGSET, + pid_, + reinterpret_cast(NT_PRSTATUS), + &iov) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + + is_64_bit_ = am_64_bit == (iov.iov_len == sizeof(regbuf.regs)); + + if (ptrace(PTRACE_DETACH, pid_, nullptr, nullptr) != 0) { + PLOG(ERROR) << "ptrace"; + } + } + + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; +} + +pid_t ProcessInfo::ProcessID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return pid_; +} + +pid_t ProcessInfo::ParentProcessID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return ppid_; +} + +uid_t ProcessInfo::RealUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return uid_; +} + +uid_t ProcessInfo::EffectiveUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return euid_; +} + +uid_t ProcessInfo::SavedUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return suid_; +} + +gid_t ProcessInfo::RealGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return gid_; +} + +gid_t ProcessInfo::EffectiveGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return egid_; +} + +gid_t ProcessInfo::SavedGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return sgid_; +} + +std::set ProcessInfo::SupplementaryGroups() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return supplementary_groups_; +} + +std::set ProcessInfo::AllGroups() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + std::set all_groups = SupplementaryGroups(); + all_groups.insert(RealGroupID()); + all_groups.insert(EffectiveGroupID()); + all_groups.insert(SavedGroupID()); + return all_groups; +} + +bool ProcessInfo::DidChangePrivileges() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + // TODO(jperaza): Is this possible to determine? + return false; +} + +bool ProcessInfo::Is64Bit() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return is_64_bit_; +} + +void ProcessInfo::StartTime(timeval* start_time) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + *start_time = start_time_; +} + +bool ProcessInfo::Arguments(std::vector* argv) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/cmdline", pid_); + std::string command; + if (!ReadEntireFile(path, &command)) { + return false; + } + + if (command.size() == 0 || command.back() != '\0') { + LOG(ERROR) << "format error"; + return false; + } + + command.pop_back(); + std::vector local_argv = SplitString(command, '\0'); + argv->swap(local_argv); + return true; +} + +} // namespace crashpad diff --git a/util/posix/process_info_test.cc b/util/posix/process_info_test.cc index c07b1ef7..6b693966 100644 --- a/util/posix/process_info_test.cc +++ b/util/posix/process_info_test.cc @@ -32,14 +32,15 @@ #include #endif +#if defined(OS_LINUX) || defined(OS_ANDROID) +#include +#endif + namespace crashpad { namespace test { namespace { -void TestSelfProcess(const ProcessInfo& process_info) { - EXPECT_EQ(getpid(), process_info.ProcessID()); - EXPECT_EQ(getppid(), process_info.ParentProcessID()); - +void TestProcessClone(const ProcessInfo& process_info) { // There’s no system call to obtain the saved set-user ID or saved set-group // ID in an easy way. Normally, they are the same as the effective user ID and // effective group ID, so just check against those. @@ -47,6 +48,7 @@ void TestSelfProcess(const ProcessInfo& process_info) { const uid_t euid = geteuid(); EXPECT_EQ(euid, process_info.EffectiveUserID()); EXPECT_EQ(euid, process_info.SavedUserID()); + const gid_t gid = getgid(); EXPECT_EQ(gid, process_info.RealGroupID()); const gid_t egid = getegid(); @@ -141,6 +143,12 @@ void TestSelfProcess(const ProcessInfo& process_info) { EXPECT_EQ(std::string(expect_argv[0]), argv[0]); } +void TestSelfProcess(const ProcessInfo& process_info) { + EXPECT_EQ(getpid(), process_info.ProcessID()); + EXPECT_EQ(getppid(), process_info.ParentProcessID()); + + TestProcessClone(process_info); +} TEST(ProcessInfo, Self) { ProcessInfo process_info; @@ -156,6 +164,9 @@ TEST(ProcessInfo, SelfTask) { } #endif +// Applications cannot ptrace PID 1 on Android, which is required for Initialize +// to succeed. +#if !defined(OS_ANDROID) TEST(ProcessInfo, Pid1) { // PID 1 is expected to be init or the system’s equivalent. This tests reading // information about another process. @@ -172,6 +183,29 @@ TEST(ProcessInfo, Pid1) { EXPECT_EQ(implicit_cast(0), process_info.SavedGroupID()); EXPECT_FALSE(process_info.AllGroups().empty()); } +#endif + +#if defined(OS_LINUX) || defined(OS_ANDROID) +TEST(ProcessInfo, ForkedSelf) { + ASSERT_EQ(0, prctl(PR_SET_DUMPABLE, 1, 0, 0, 0)) << ErrnoMessage("prctl"); + + pid_t pid = fork(); + if (pid == 0) { + raise(SIGSTOP); + _exit(0); + } + ASSERT_GE(pid, 0) << ErrnoMessage("fork"); + + ProcessInfo process_info; + ASSERT_TRUE(process_info.Initialize(pid)); + + EXPECT_EQ(pid, process_info.ProcessID()); + EXPECT_EQ(getpid(), process_info.ParentProcessID()); + + TestProcessClone(process_info); + kill(pid, SIGKILL); +} +#endif } // namespace } // namespace test diff --git a/util/util.gyp b/util/util.gyp index f1f004c7..c715233b 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -135,6 +135,7 @@ 'posix/drop_privileges.cc', 'posix/drop_privileges.h', 'posix/process_info.h', + 'posix/process_info_linux.cc', 'posix/process_info_mac.cc', 'posix/signals.cc', 'posix/signals.h', @@ -313,6 +314,13 @@ ], }], ], + 'target_conditions': [ + ['OS=="android"', { + 'sources/': [ + ['include', '^posix/process_info_linux\\.cc$'], + ], + }], + ], }, ], } diff --git a/util/util_test.gyp b/util/util_test.gyp index a33e474e..45ac6bd5 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -120,6 +120,18 @@ ], }, }], + ['OS=="android"', { + # Things not yet ported to Android + 'sources/' : [ + ['exclude', '^net/http_transport_test\\.cc$'], + ] + }], + ['OS=="android" or OS=="linux"' , { + # Things not yet ported to Android or Linux + 'sources/' : [ + ['exclude', '^numeric/checked_address_range_test\\.cc$'], + ] + }], ], }, ], From bad4fd00113a65d3c41ebba122d3e00efd198e6f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 14 Mar 2017 18:01:55 -0400 Subject: [PATCH 04/19] linux: Fix ProcessInfo for x86[_64] This configuration uses user_regs_struct, which is declared in . Bug: crashpad:30 Change-Id: Ibdcc60c6719fc2bad9fbeef116efbe764229e14b Reviewed-on: https://chromium-review.googlesource.com/455197 Reviewed-by: Joshua Peraza Commit-Queue: Mark Mentovai --- util/posix/process_info_linux.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 9921b65b..861c8031 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include From 48781dc182c24b3b41b0c9a16209663786855a1f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 14 Mar 2017 21:25:50 -0400 Subject: [PATCH 05/19] 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 Commit-Queue: Mark Mentovai --- util/posix/process_info_linux.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 861c8031..6389e765 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -247,14 +247,19 @@ bool ProcessInfo::Initialize(pid_t pid) { return false; } - for (int index = 1; - index < 21 && stat_pos < stat_contents.size(); - ++index) { - stat_pos = stat_contents.find(" ", stat_pos); + for (int index = 1; index < 21; ++index) { + stat_pos = stat_contents.find(' ', stat_pos); + if (stat_pos == std::string::npos) { + break; + } ++stat_pos; } + if (stat_pos >= stat_contents.size()) { + LOG(ERROR) << "format error"; + return false; + } - const char* ticks_ptr = stat_contents.substr(stat_pos).c_str(); + const char* ticks_ptr = &stat_contents[stat_pos]; // start time is in jiffies instead of clock ticks pre 2.6. uint64_t ticks_after_boot; From 9be4745be0f7b38d7ea481c81444c4fb18acff4b Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Mar 2017 00:09:28 -0400 Subject: [PATCH 06/19] =?UTF-8?q?linux:=20Lazily=20initialize=20ProcessInf?= =?UTF-8?q?o=E2=80=99s=20Is64Bit()=20and=20StartTime()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Mark Mentovai --- snapshot/mac/process_reader.cc | 9 +- snapshot/mac/process_reader.h | 4 +- util/posix/process_info.h | 26 ++- util/posix/process_info_linux.cc | 285 +++++++++++++++++-------------- util/posix/process_info_mac.cc | 8 +- util/posix/process_info_test.cc | 28 ++- 6 files changed, 208 insertions(+), 152 deletions(-) diff --git a/snapshot/mac/process_reader.cc b/snapshot/mac/process_reader.cc index 46b83c13..46083ab3 100644 --- a/snapshot/mac/process_reader.cc +++ b/snapshot/mac/process_reader.cc @@ -116,7 +116,9 @@ bool ProcessReader::Initialize(task_t task) { return false; } - is_64_bit_ = process_info_.Is64Bit(); + if (!process_info_.Is64Bit(&is_64_bit_)) { + return false; + } task_memory_.reset(new TaskMemory(task)); task_ = task; @@ -125,6 +127,11 @@ bool ProcessReader::Initialize(task_t task) { return true; } +void ProcessReader::StartTime(timeval* start_time) const { + bool rv = process_info_.StartTime(start_time); + DCHECK(rv); +} + bool ProcessReader::CPUTimes(timeval* user_time, timeval* system_time) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); diff --git a/snapshot/mac/process_reader.h b/snapshot/mac/process_reader.h index c3696110..72bd1dc6 100644 --- a/snapshot/mac/process_reader.h +++ b/snapshot/mac/process_reader.h @@ -124,9 +124,7 @@ class ProcessReader { //! \brief Determines the target process’ start time. //! //! \param[out] start_time The time that the process started. - void StartTime(timeval* start_time) const { - process_info_.StartTime(start_time); - } + void StartTime(timeval* start_time) const; //! \brief Determines the target process’ execution time. //! diff --git a/util/posix/process_info.h b/util/posix/process_info.h index 10cee96d..5e1a6108 100644 --- a/util/posix/process_info.h +++ b/util/posix/process_info.h @@ -25,6 +25,7 @@ #include "base/macros.h" #include "build/build_config.h" +#include "util/misc/initialization_state.h" #include "util/misc/initialization_state_dcheck.h" #if defined(OS_MACOSX) @@ -113,13 +114,21 @@ class ProcessInfo { //! `execve()` as a result of executing a setuid or setgid executable. bool DidChangePrivileges() const; - //! \return `true` if the target task is a 64-bit process. - bool Is64Bit() const; + //! \brief Determines the target process’ bitness. + //! + //! \param[out] is_64_bit `true` if the target task is a 64-bit process. + //! + //! \return `true` on success, with \a is_64_bit set. Otherwise, `false` with + //! a message logged. + bool Is64Bit(bool* is_64_bit) const; //! \brief Determines the target process’ start time. //! //! \param[out] start_time The time that the process started. - void StartTime(timeval* start_time) const; + //! + //! \return `true` on success, with \a start_time set. Otherwise, `false` with + //! a message logged. + bool StartTime(timeval* start_time) const; //! \brief Obtains the arguments used to launch a process. //! @@ -142,8 +151,13 @@ class ProcessInfo { #if defined(OS_MACOSX) kinfo_proc kern_proc_info_; #elif defined(OS_LINUX) || defined(OS_ANDROID) + // Some members are marked mutable so that they can be lazily initialized by + // const methods. These are always InitializationState-protected so that + // multiple successive calls will always produce the same return value and out + // parameters. This is necessary for intergration with the Snapshot interface. + // See https://crashpad.chromium.org/bug/9. std::set supplementary_groups_; - timeval start_time_; + mutable timeval start_time_; pid_t pid_; pid_t ppid_; uid_t uid_; @@ -152,7 +166,9 @@ class ProcessInfo { gid_t gid_; gid_t egid_; gid_t sgid_; - bool is_64_bit_; + mutable InitializationState start_time_initialized_; + mutable InitializationState is_64_bit_initialized_; + mutable bool is_64_bit_; #endif InitializationStateDcheck initialized_; diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 6389e765..9147e715 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -114,6 +114,21 @@ void TimespecToTimeval(const timespec& ts, timeval* tv) { tv->tv_usec = ts.tv_nsec / 1000; } +class ScopedPtraceDetach { + public: + explicit ScopedPtraceDetach(pid_t pid) : pid_(pid) {} + ~ScopedPtraceDetach() { + if (ptrace(PTRACE_DETACH, pid_, nullptr, nullptr) != 0) { + PLOG(ERROR) << "ptrace"; + } + } + + private: + pid_t pid_; + + DISALLOW_COPY_AND_ASSIGN(ScopedPtraceDetach); +}; + } // namespace ProcessInfo::ProcessInfo() @@ -127,6 +142,8 @@ ProcessInfo::ProcessInfo() gid_(-1), egid_(-1), sgid_(-1), + start_time_initialized_(), + is_64_bit_initialized_(), is_64_bit_(false), initialized_() {} @@ -228,7 +245,145 @@ bool ProcessInfo::Initialize(pid_t pid) { } } - { + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; +} + +pid_t ProcessInfo::ProcessID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return pid_; +} + +pid_t ProcessInfo::ParentProcessID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return ppid_; +} + +uid_t ProcessInfo::RealUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return uid_; +} + +uid_t ProcessInfo::EffectiveUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return euid_; +} + +uid_t ProcessInfo::SavedUserID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return suid_; +} + +gid_t ProcessInfo::RealGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return gid_; +} + +gid_t ProcessInfo::EffectiveGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return egid_; +} + +gid_t ProcessInfo::SavedGroupID() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return sgid_; +} + +std::set ProcessInfo::SupplementaryGroups() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return supplementary_groups_; +} + +std::set ProcessInfo::AllGroups() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + std::set all_groups = SupplementaryGroups(); + all_groups.insert(RealGroupID()); + all_groups.insert(EffectiveGroupID()); + all_groups.insert(SavedGroupID()); + return all_groups; +} + +bool ProcessInfo::DidChangePrivileges() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + // TODO(jperaza): Is this possible to determine? + return false; +} + +bool ProcessInfo::Is64Bit(bool* is_64_bit) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + if (is_64_bit_initialized_.is_uninitialized()) { + is_64_bit_initialized_.set_invalid(); + +#if defined(ARCH_CPU_64_BITS) + const bool am_64_bit = true; +#else + const bool am_64_bit = false; +#endif + + if (pid_ == getpid()) { + is_64_bit_ = am_64_bit; + } else { + if (ptrace(PTRACE_ATTACH, pid_, nullptr, nullptr) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + + ScopedPtraceDetach ptrace_detach(pid_); + + if (HANDLE_EINTR(waitpid(pid_, nullptr, __WALL)) < 0) { + PLOG(ERROR) << "waitpid"; + return false; + } + + // Allocate more buffer space than is required to hold registers for this + // process. If the kernel fills the extra space, the target process uses + // more/larger registers than this process. If the kernel fills less space + // than sizeof(regs) then the target process uses smaller/fewer registers. + struct { +#if defined(ARCH_CPU_X86_FAMILY) + using PrStatusType = user_regs_struct; +#elif defined(ARCH_CPU_ARMEL) + using PrStatusType = pt_regs; +#elif defined(ARCH_CPU_ARM64) + using PrStatusType = user_pt_regs; +#endif + PrStatusType regs; + char extra; + } regbuf; + + iovec iov; + iov.iov_base = ®buf; + iov.iov_len = sizeof(regbuf); + if (ptrace(PTRACE_GETREGSET, + pid_, + reinterpret_cast(NT_PRSTATUS), + &iov) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + + is_64_bit_ = am_64_bit == (iov.iov_len == sizeof(regbuf.regs)); + } + + is_64_bit_initialized_.set_valid(); + } + + if (!is_64_bit_initialized_.is_valid()) { + return false; + } + + *is_64_bit = is_64_bit_; + return true; +} + +bool ProcessInfo::StartTime(timeval* start_time) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + if (start_time_initialized_.is_uninitialized()) { + start_time_initialized_.set_invalid(); + char path[32]; snprintf(path, sizeof(path), "/proc/%d/stat", pid_); std::string stat_contents; @@ -295,134 +450,16 @@ bool ProcessInfo::Initialize(pid_t pid) { timeval boot_time_tv; TimespecToTimeval(boot_time_ts, &boot_time_tv); timeradd(&boot_time_tv, &time_after_boot, &start_time_); + + start_time_initialized_.set_valid(); } -#if defined(ARCH_CPU_64_BITS) - const bool am_64_bit = true; -#else - const bool am_64_bit = false; -#endif - - if (pid_ == getpid()) { - is_64_bit_ = am_64_bit; - } else { - if (ptrace(PTRACE_ATTACH, pid_, nullptr, nullptr) != 0) { - PLOG(ERROR) << "ptrace"; - return false; - } - - if (HANDLE_EINTR(waitpid(pid_, nullptr, __WALL)) < 0) { - PLOG(ERROR) << "waitpid"; - return false; - } - - // Allocate more buffer space than is required to hold registers for this - // process. If the kernel fills the extra space, the target process uses - // more/larger registers than this process. If the kernel fills less space - // than sizeof(regs) then the target process uses smaller/fewer registers. - struct { -#if defined(ARCH_CPU_X86_FAMILY) - using PrStatusType = user_regs_struct; -#elif defined(ARCH_CPU_ARMEL) - using PrStatusType = pt_regs; -#elif defined(ARCH_CPU_ARM64) - using PrStatusType = user_pt_regs; -#endif - PrStatusType regs; - char extra; - } regbuf; - - iovec iov; - iov.iov_base = ®buf; - iov.iov_len = sizeof(regbuf); - if (ptrace(PTRACE_GETREGSET, - pid_, - reinterpret_cast(NT_PRSTATUS), - &iov) != 0) { - PLOG(ERROR) << "ptrace"; - return false; - } - - is_64_bit_ = am_64_bit == (iov.iov_len == sizeof(regbuf.regs)); - - if (ptrace(PTRACE_DETACH, pid_, nullptr, nullptr) != 0) { - PLOG(ERROR) << "ptrace"; - } + if (!start_time_initialized_.is_valid()) { + return false; } - INITIALIZATION_STATE_SET_VALID(initialized_); - return true; -} - -pid_t ProcessInfo::ProcessID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return pid_; -} - -pid_t ProcessInfo::ParentProcessID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return ppid_; -} - -uid_t ProcessInfo::RealUserID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return uid_; -} - -uid_t ProcessInfo::EffectiveUserID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return euid_; -} - -uid_t ProcessInfo::SavedUserID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return suid_; -} - -gid_t ProcessInfo::RealGroupID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return gid_; -} - -gid_t ProcessInfo::EffectiveGroupID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return egid_; -} - -gid_t ProcessInfo::SavedGroupID() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return sgid_; -} - -std::set ProcessInfo::SupplementaryGroups() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return supplementary_groups_; -} - -std::set ProcessInfo::AllGroups() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - - std::set all_groups = SupplementaryGroups(); - all_groups.insert(RealGroupID()); - all_groups.insert(EffectiveGroupID()); - all_groups.insert(SavedGroupID()); - return all_groups; -} - -bool ProcessInfo::DidChangePrivileges() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - // TODO(jperaza): Is this possible to determine? - return false; -} - -bool ProcessInfo::Is64Bit() const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return is_64_bit_; -} - -void ProcessInfo::StartTime(timeval* start_time) const { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); *start_time = start_time_; + return true; } bool ProcessInfo::Arguments(std::vector* argv) const { diff --git a/util/posix/process_info_mac.cc b/util/posix/process_info_mac.cc index 17d205a6..4a1fc58a 100644 --- a/util/posix/process_info_mac.cc +++ b/util/posix/process_info_mac.cc @@ -132,14 +132,16 @@ bool ProcessInfo::DidChangePrivileges() const { return kern_proc_info_.kp_proc.p_flag & P_SUGID; } -bool ProcessInfo::Is64Bit() const { +bool ProcessInfo::Is64Bit(bool* is_64_bit) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - return kern_proc_info_.kp_proc.p_flag & P_LP64; + *is_64_bit = kern_proc_info_.kp_proc.p_flag & P_LP64; + return true; } -void ProcessInfo::StartTime(timeval* start_time) const { +bool ProcessInfo::StartTime(timeval* start_time) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); *start_time = kern_proc_info_.kp_proc.p_starttime; + return true; } bool ProcessInfo::Arguments(std::vector* argv) const { diff --git a/util/posix/process_info_test.cc b/util/posix/process_info_test.cc index 6b693966..8d63b722 100644 --- a/util/posix/process_info_test.cc +++ b/util/posix/process_info_test.cc @@ -14,8 +14,9 @@ #include "util/posix/process_info.h" -#include +#include #include +#include #include #include @@ -40,7 +41,7 @@ namespace crashpad { namespace test { namespace { -void TestProcessClone(const ProcessInfo& process_info) { +void TestProcessSelfOrClone(const ProcessInfo& process_info) { // There’s no system call to obtain the saved set-user ID or saved set-group // ID in an easy way. Normally, they are the same as the effective user ID and // effective group ID, so just check against those. @@ -80,15 +81,18 @@ void TestProcessClone(const ProcessInfo& process_info) { // The test executable isn’t expected to change privileges. EXPECT_FALSE(process_info.DidChangePrivileges()); + bool is_64_bit; + ASSERT_TRUE(process_info.Is64Bit(&is_64_bit)); #if defined(ARCH_CPU_64_BITS) - EXPECT_TRUE(process_info.Is64Bit()); + EXPECT_TRUE(is_64_bit); #else - EXPECT_FALSE(process_info.Is64Bit()); + EXPECT_FALSE(is_64_bit); #endif // Test StartTime(). This program must have started at some time in the past. timeval start_time; - process_info.StartTime(&start_time); + ASSERT_TRUE(process_info.StartTime(&start_time)); + EXPECT_FALSE(start_time.tv_sec == 0 && start_time.tv_usec == 0); time_t now; time(&now); EXPECT_LE(start_time.tv_sec, now); @@ -147,7 +151,7 @@ void TestSelfProcess(const ProcessInfo& process_info) { EXPECT_EQ(getpid(), process_info.ProcessID()); EXPECT_EQ(getppid(), process_info.ParentProcessID()); - TestProcessClone(process_info); + TestProcessSelfOrClone(process_info); } TEST(ProcessInfo, Self) { @@ -164,9 +168,6 @@ TEST(ProcessInfo, SelfTask) { } #endif -// Applications cannot ptrace PID 1 on Android, which is required for Initialize -// to succeed. -#if !defined(OS_ANDROID) TEST(ProcessInfo, Pid1) { // PID 1 is expected to be init or the system’s equivalent. This tests reading // information about another process. @@ -183,12 +184,8 @@ TEST(ProcessInfo, Pid1) { EXPECT_EQ(implicit_cast(0), process_info.SavedGroupID()); EXPECT_FALSE(process_info.AllGroups().empty()); } -#endif - -#if defined(OS_LINUX) || defined(OS_ANDROID) -TEST(ProcessInfo, ForkedSelf) { - ASSERT_EQ(0, prctl(PR_SET_DUMPABLE, 1, 0, 0, 0)) << ErrnoMessage("prctl"); +TEST(ProcessInfo, Forked) { pid_t pid = fork(); if (pid == 0) { raise(SIGSTOP); @@ -202,10 +199,9 @@ TEST(ProcessInfo, ForkedSelf) { EXPECT_EQ(pid, process_info.ProcessID()); EXPECT_EQ(getpid(), process_info.ParentProcessID()); - TestProcessClone(process_info); + TestProcessSelfOrClone(process_info); kill(pid, SIGKILL); } -#endif } // namespace } // namespace test From d7467ba7e48566d751cc0e79c2590af5c2c06923 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Mar 2017 12:05:02 -0400 Subject: [PATCH 07/19] linux: Use user_regs instead of pt_regs for 32-bit ARM in ProcessInfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Not all libc implementations reliably expose pt_regs from . glibc-2.25/sysdeps/generic/sys/ptrace.h, for example, does not #include (which defines the structure) or anything else that would #include that file such as . On the other hand, Android 7.1.1 bionic/libc/include/sys/ptrace.h does #include . It is not viable to #include or directly: it would be natural to #include them, sorted, before but this causes problems for glibc’s . Constants like PTRACE_GETREGS and PTRACE_TRACEME are simple macros in and , respectively, but are defined in enums in glibc’s , and this doesn’t mix well. It is possible to #include (but not ) after , 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 , 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 in preference to hoping for a C library’s 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 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 --- util/posix/process_info_linux.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 9147e715..82f58889 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -342,12 +342,10 @@ bool ProcessInfo::Is64Bit(bool* is_64_bit) const { // more/larger registers than this process. If the kernel fills less space // than sizeof(regs) then the target process uses smaller/fewer registers. struct { -#if defined(ARCH_CPU_X86_FAMILY) +#if defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARM64) using PrStatusType = user_regs_struct; #elif defined(ARCH_CPU_ARMEL) - using PrStatusType = pt_regs; -#elif defined(ARCH_CPU_ARM64) - using PrStatusType = user_pt_regs; + using PrStatusType = user_regs; #endif PrStatusType regs; char extra; From 5938c6e9933aac711fa5cfc5415771841603aa98 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Mar 2017 13:12:35 -0400 Subject: [PATCH 08/19] 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 --- util/posix/process_info_linux.cc | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 82f58889..f553cdac 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -358,11 +359,31 @@ bool ProcessInfo::Is64Bit(bool* is_64_bit) const { pid_, reinterpret_cast(NT_PRSTATUS), &iov) != 0) { - PLOG(ERROR) << "ptrace"; - return false; + switch (errno) { +#if defined(ARCH_CPU_ARMEL) + case EIO: + // PTRACE_GETREGSET, introduced in Linux 2.6.34 (2225a122ae26), + // requires kernel support enabled by HAVE_ARCH_TRACEHOOK. This has + // been set for x86 (including x86_64) since Linux 2.6.28 + // (99bbc4b1e677a), but for ARM only since Linux 3.5.0 + // (0693bf68148c4). Fortunately, 64-bit ARM support only appeared in + // Linux 3.7.0, so if PTRACE_GETREGSET fails on ARM with EIO, + // indicating that the request is not supported, the kernel must be + // old enough that 64-bit ARM isn’t supported either. + // + // TODO(mark): Once helpers to interpret the kernel version are + // available, add a DCHECK to ensure that the kernel is older than + // 3.5. + is_64_bit_ = false; + break; +#endif + default: + PLOG(ERROR) << "ptrace"; + return false; + } + } else { + is_64_bit_ = am_64_bit == (iov.iov_len == sizeof(regbuf.regs)); } - - is_64_bit_ = am_64_bit == (iov.iov_len == sizeof(regbuf.regs)); } is_64_bit_initialized_.set_valid(); From 82009cd14de59f5d2b063e6c5681bd9399de3596 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Mar 2017 13:47:31 -0400 Subject: [PATCH 09/19] 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 or by , included by . Although the definition in is available in older NDK API versions, this internal header does not mix well with unless contemplates this combination. As of NDK API 21, actually delegates most of its work to . PR_GETREGSET is not available in the NDK at all until API 21. Its definition is in . Most user code should #include instead, which includes . Bug: crashpad:30 Test: crashpad_util_test ProcessInfo.* Change-Id: I4d07a9964db4665a49bde490e905ae9126880bc5 Reviewed-on: https://chromium-review.googlesource.com/455659 Reviewed-by: Joshua Peraza --- compat/android/elf.h | 25 +++++++++++++++++++++++++ compat/android/linux/ptrace.h | 25 +++++++++++++++++++++++++ compat/compat.gyp | 10 ++++++++++ 3 files changed, 60 insertions(+) create mode 100644 compat/android/elf.h create mode 100644 compat/android/linux/ptrace.h diff --git a/compat/android/elf.h b/compat/android/elf.h new file mode 100644 index 00000000..a3fc2a9a --- /dev/null +++ b/compat/android/elf.h @@ -0,0 +1,25 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_COMPAT_ANDROID_ELF_H_ +#define CRASHPAD_COMPAT_ANDROID_ELF_H_ + +#include_next + +// Android 5.0.0 (API 21) NDK +#if !defined(NT_PRSTATUS) +#define NT_PRSTATUS 1 +#endif + +#endif // CRASHPAD_COMPAT_ANDROID_ELF_H_ diff --git a/compat/android/linux/ptrace.h b/compat/android/linux/ptrace.h new file mode 100644 index 00000000..7db46aa3 --- /dev/null +++ b/compat/android/linux/ptrace.h @@ -0,0 +1,25 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_COMPAT_ANDROID_LINUX_PTRACE_H_ +#define CRASHPAD_COMPAT_ANDROID_LINUX_PTRACE_H_ + +#include_next + +// Android 5.0.0 (API 21) NDK +#if !defined(PTRACE_GETREGSET) +#define PTRACE_GETREGSET 0x4204 +#endif + +#endif // CRASHPAD_COMPAT_ANDROID_LINUX_PTRACE_H_ diff --git a/compat/compat.gyp b/compat/compat.gyp index f1cd4933..56daa3f8 100644 --- a/compat/compat.gyp +++ b/compat/compat.gyp @@ -83,6 +83,16 @@ ], }, }], + ['OS=="android"', { + 'include_dirs': [ + 'android', + ], + 'direct_dependent_settings': { + 'include_dirs': [ + 'android', + ], + }, + }], ], }, ], From b10d9118dea443d0f3d5a3fe9755fe045ca06f25 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 15 Mar 2017 15:35:36 -0400 Subject: [PATCH 10/19] minidump: Ignore attempts to add user streams with type collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../minidump_crashpad_info_writer_test.cc | 10 +-- minidump/minidump_exception_writer_test.cc | 8 +-- minidump/minidump_file_writer.cc | 70 +++++++++++++------ minidump/minidump_file_writer.h | 11 ++- minidump/minidump_file_writer_test.cc | 56 ++++++++++----- minidump/minidump_handle_writer_test.cc | 6 +- minidump/minidump_memory_info_writer_test.cc | 6 +- minidump/minidump_memory_writer_test.cc | 12 ++-- minidump/minidump_misc_info_writer_test.cc | 30 ++++---- minidump/minidump_module_writer_test.cc | 14 ++-- minidump/minidump_system_info_writer_test.cc | 14 ++-- minidump/minidump_thread_writer_test.cc | 16 ++--- .../minidump_unloaded_module_writer_test.cc | 6 +- minidump/minidump_user_stream_writer_test.cc | 4 +- 14 files changed, 162 insertions(+), 101 deletions(-) diff --git a/minidump/minidump_crashpad_info_writer_test.cc b/minidump/minidump_crashpad_info_writer_test.cc index 85eec274..0b620663 100644 --- a/minidump/minidump_crashpad_info_writer_test.cc +++ b/minidump/minidump_crashpad_info_writer_test.cc @@ -70,7 +70,7 @@ TEST(MinidumpCrashpadInfoWriter, Empty) { base::WrapUnique(new MinidumpCrashpadInfoWriter()); EXPECT_FALSE(crashpad_info_writer->IsUseful()); - minidump_file_writer.AddStream(std::move(crashpad_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(crashpad_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -106,7 +106,7 @@ TEST(MinidumpCrashpadInfoWriter, ReportAndClientID) { EXPECT_TRUE(crashpad_info_writer->IsUseful()); - minidump_file_writer.AddStream(std::move(crashpad_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(crashpad_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -148,7 +148,7 @@ TEST(MinidumpCrashpadInfoWriter, SimpleAnnotations) { EXPECT_TRUE(crashpad_info_writer->IsUseful()); - minidump_file_writer.AddStream(std::move(crashpad_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(crashpad_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -189,7 +189,7 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) { EXPECT_TRUE(crashpad_info_writer->IsUseful()); - minidump_file_writer.AddStream(std::move(crashpad_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(crashpad_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -265,7 +265,7 @@ TEST(MinidumpCrashpadInfoWriter, InitializeFromSnapshot) { EXPECT_TRUE(info_writer->IsUseful()); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_exception_writer_test.cc b/minidump/minidump_exception_writer_test.cc index 8635190b..613c5dd1 100644 --- a/minidump/minidump_exception_writer_test.cc +++ b/minidump/minidump_exception_writer_test.cc @@ -102,7 +102,7 @@ TEST(MinidumpExceptionWriter, Minimal) { InitializeMinidumpContextX86(context_x86_writer->context(), kSeed); exception_writer->SetContext(std::move(context_x86_writer)); - minidump_file_writer.AddStream(std::move(exception_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(exception_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -161,7 +161,7 @@ TEST(MinidumpExceptionWriter, Standard) { exception_information.push_back(kExceptionInformation2); exception_writer->SetExceptionInformation(exception_information); - minidump_file_writer.AddStream(std::move(exception_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(exception_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -234,7 +234,7 @@ TEST(MinidumpExceptionWriter, InitializeFromSnapshot) { exception_writer->InitializeFromSnapshot(&exception_snapshot, thread_id_map); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(exception_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(exception_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -256,7 +256,7 @@ TEST(MinidumpExceptionWriterDeathTest, NoContext) { MinidumpFileWriter minidump_file_writer; auto exception_writer = base::WrapUnique(new MinidumpExceptionWriter()); - minidump_file_writer.AddStream(std::move(exception_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(exception_writer))); StringFile string_file; ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), diff --git a/minidump/minidump_file_writer.cc b/minidump/minidump_file_writer.cc index 6a91ca6b..819e807f 100644 --- a/minidump/minidump_file_writer.cc +++ b/minidump/minidump_file_writer.cc @@ -74,11 +74,13 @@ void MinidumpFileWriter::InitializeFromSnapshot( const SystemSnapshot* system_snapshot = process_snapshot->System(); auto system_info = base::WrapUnique(new MinidumpSystemInfoWriter()); system_info->InitializeFromSnapshot(system_snapshot); - AddStream(std::move(system_info)); + bool add_stream_result = AddStream(std::move(system_info)); + DCHECK(add_stream_result); auto misc_info = base::WrapUnique(new MinidumpMiscInfoWriter()); misc_info->InitializeFromSnapshot(process_snapshot); - AddStream(std::move(misc_info)); + add_stream_result = AddStream(std::move(misc_info)); + DCHECK(add_stream_result); auto memory_list = base::WrapUnique(new MinidumpMemoryListWriter()); auto thread_list = base::WrapUnique(new MinidumpThreadListWriter()); @@ -86,33 +88,29 @@ void MinidumpFileWriter::InitializeFromSnapshot( MinidumpThreadIDMap thread_id_map; thread_list->InitializeFromSnapshot(process_snapshot->Threads(), &thread_id_map); - AddStream(std::move(thread_list)); + add_stream_result = AddStream(std::move(thread_list)); + DCHECK(add_stream_result); const ExceptionSnapshot* exception_snapshot = process_snapshot->Exception(); if (exception_snapshot) { auto exception = base::WrapUnique(new MinidumpExceptionWriter()); exception->InitializeFromSnapshot(exception_snapshot, thread_id_map); - AddStream(std::move(exception)); + add_stream_result = AddStream(std::move(exception)); + DCHECK(add_stream_result); } auto module_list = base::WrapUnique(new MinidumpModuleListWriter()); module_list->InitializeFromSnapshot(process_snapshot->Modules()); - AddStream(std::move(module_list)); - - for (const auto& module : process_snapshot->Modules()) { - for (const UserMinidumpStream* stream : module->CustomMinidumpStreams()) { - auto user_stream = base::WrapUnique(new MinidumpUserStreamWriter()); - user_stream->InitializeFromSnapshot(stream); - AddStream(std::move(user_stream)); - } - } + add_stream_result = AddStream(std::move(module_list)); + DCHECK(add_stream_result); auto unloaded_modules = process_snapshot->UnloadedModules(); if (!unloaded_modules.empty()) { auto unloaded_module_list = base::WrapUnique(new MinidumpUnloadedModuleListWriter()); unloaded_module_list->InitializeFromSnapshot(unloaded_modules); - AddStream(std::move(unloaded_module_list)); + add_stream_result = AddStream(std::move(unloaded_module_list)); + DCHECK(add_stream_result); } auto crashpad_info = base::WrapUnique(new MinidumpCrashpadInfoWriter()); @@ -121,7 +119,8 @@ void MinidumpFileWriter::InitializeFromSnapshot( // Since the MinidumpCrashpadInfo stream is an extension, it’s safe to not add // it to the minidump file if it wouldn’t carry any useful information. if (crashpad_info->IsUseful()) { - AddStream(std::move(crashpad_info)); + add_stream_result = AddStream(std::move(crashpad_info)); + DCHECK(add_stream_result); } std::vector memory_map_snapshot = @@ -130,21 +129,46 @@ void MinidumpFileWriter::InitializeFromSnapshot( auto memory_info_list = base::WrapUnique(new MinidumpMemoryInfoListWriter()); memory_info_list->InitializeFromSnapshot(memory_map_snapshot); - AddStream(std::move(memory_info_list)); + add_stream_result = AddStream(std::move(memory_info_list)); + DCHECK(add_stream_result); } std::vector handles_snapshot = process_snapshot->Handles(); if (!handles_snapshot.empty()) { auto handle_data_writer = base::WrapUnique(new MinidumpHandleDataWriter()); handle_data_writer->InitializeFromSnapshot(handles_snapshot); - AddStream(std::move(handle_data_writer)); + add_stream_result = AddStream(std::move(handle_data_writer)); + DCHECK(add_stream_result); } memory_list->AddFromSnapshot(process_snapshot->ExtraMemory()); - if (exception_snapshot) + if (exception_snapshot) { memory_list->AddFromSnapshot(exception_snapshot->ExtraMemory()); + } - AddStream(std::move(memory_list)); + // The memory list stream should be added last. This keeps the “extra memory” + // at the end so that if the minidump file is truncated, other, more critical + // data is more likely to be preserved. Note that non-“extra” memory regions + // will not have to ride at the end of the file. Thread stack memory, for + // example, exists as a children of threads, and appears alongside them in the + // file. + // + // It would be nice if this followed the user streams, but that would be + // hazardous. See below. + add_stream_result = AddStream(std::move(memory_list)); + DCHECK(add_stream_result); + + // These user streams must be added last. Otherwise, a user stream with the + // same type as a well-known stream could preempt the well-known stream. As it + // stands now, earlier-discovered user streams can still preempt + // later-discovered ones. + for (const auto& module : process_snapshot->Modules()) { + for (const UserMinidumpStream* stream : module->CustomMinidumpStreams()) { + auto user_stream = base::WrapUnique(new MinidumpUserStreamWriter()); + user_stream->InitializeFromSnapshot(stream); + AddStream(std::move(user_stream)); + } + } } void MinidumpFileWriter::SetTimestamp(time_t timestamp) { @@ -153,18 +177,22 @@ void MinidumpFileWriter::SetTimestamp(time_t timestamp) { internal::MinidumpWriterUtil::AssignTimeT(&header_.TimeDateStamp, timestamp); } -void MinidumpFileWriter::AddStream( +bool MinidumpFileWriter::AddStream( std::unique_ptr stream) { DCHECK_EQ(state(), kStateMutable); MinidumpStreamType stream_type = stream->StreamType(); auto rv = stream_types_.insert(stream_type); - CHECK(rv.second) << "stream_type " << stream_type << " already present"; + if (!rv.second) { + LOG(WARNING) << "discarding duplicate stream of type " << stream_type; + return false; + } streams_.push_back(stream.release()); DCHECK_EQ(streams_.size(), stream_types_.size()); + return true; } bool MinidumpFileWriter::WriteEverything(FileWriterInterface* file_writer) { diff --git a/minidump/minidump_file_writer.h b/minidump/minidump_file_writer.h index b093dade..05cecb14 100644 --- a/minidump/minidump_file_writer.h +++ b/minidump/minidump_file_writer.h @@ -84,11 +84,16 @@ class MinidumpFileWriter final : public internal::MinidumpWritable { //! //! At most one object of each stream type (as obtained from //! internal::MinidumpStreamWriter::StreamType()) may be added to a - //! MinidumpFileWriter object. It is an error to attempt to add multiple - //! streams with the same stream type. + //! MinidumpFileWriter object. If an attempt is made to add a stream whose + //! type matches an existing stream’s type, this method discards the new + //! stream. //! //! \note Valid in #kStateMutable. - void AddStream(std::unique_ptr stream); + //! + //! \return `true` on success. `false` on failure, as occurs when an attempt + //! is made to add a stream whose type matches an existing stream’s type, + //! with a message logged. + bool AddStream(std::unique_ptr stream); // MinidumpWritable: diff --git a/minidump/minidump_file_writer_test.cc b/minidump/minidump_file_writer_test.cc index f3432fb8..d8d0f394 100644 --- a/minidump/minidump_file_writer_test.cc +++ b/minidump/minidump_file_writer_test.cc @@ -98,7 +98,7 @@ TEST(MinidumpFileWriter, OneStream) { const uint8_t kStreamValue = 0x5a; auto stream = base::WrapUnique(new TestStream(kStreamType, kStreamSize, kStreamValue)); - minidump_file.AddStream(std::move(stream)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream))); StringFile string_file; ASSERT_TRUE(minidump_file.WriteEverything(&string_file)); @@ -137,7 +137,7 @@ TEST(MinidumpFileWriter, ThreeStreams) { const uint8_t kStream0Value = 0x5a; auto stream0 = base::WrapUnique( new TestStream(kStream0Type, kStream0Size, kStream0Value)); - minidump_file.AddStream(std::move(stream0)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream0))); // Make the second stream’s type be a smaller quantity than the first stream’s // to test that the streams show up in the order that they were added, not in @@ -147,14 +147,14 @@ TEST(MinidumpFileWriter, ThreeStreams) { const uint8_t kStream1Value = 0xa5; auto stream1 = base::WrapUnique( new TestStream(kStream1Type, kStream1Size, kStream1Value)); - minidump_file.AddStream(std::move(stream1)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream1))); const size_t kStream2Size = 1; const MinidumpStreamType kStream2Type = static_cast(0x7e); const uint8_t kStream2Value = 0x36; auto stream2 = base::WrapUnique( new TestStream(kStream2Type, kStream2Size, kStream2Value)); - minidump_file.AddStream(std::move(stream2)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream2))); StringFile string_file; ASSERT_TRUE(minidump_file.WriteEverything(&string_file)); @@ -221,7 +221,7 @@ TEST(MinidumpFileWriter, ZeroLengthStream) { const size_t kStreamSize = 0; const MinidumpStreamType kStreamType = static_cast(0x4d); auto stream = base::WrapUnique(new TestStream(kStreamType, kStreamSize, 0)); - minidump_file.AddStream(std::move(stream)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream))); StringFile string_file; ASSERT_TRUE(minidump_file.WriteEverything(&string_file)); @@ -439,24 +439,48 @@ TEST(MinidumpFileWriter, InitializeFromSnapshot_CrashpadInfo) { string_file.string(), directory[6].Location)); } -TEST(MinidumpFileWriterDeathTest, SameStreamType) { +TEST(MinidumpFileWriter, SameStreamType) { MinidumpFileWriter minidump_file; - const size_t kStream0Size = 5; - const MinidumpStreamType kStream0Type = static_cast(0x4d); + const size_t kStream0Size = 3; + const MinidumpStreamType kStreamType = static_cast(0x4d); const uint8_t kStream0Value = 0x5a; auto stream0 = base::WrapUnique( - new TestStream(kStream0Type, kStream0Size, kStream0Value)); - minidump_file.AddStream(std::move(stream0)); + new TestStream(kStreamType, kStream0Size, kStream0Value)); + ASSERT_TRUE(minidump_file.AddStream(std::move(stream0))); - // It is an error to add a second stream of the same type. - const size_t kStream1Size = 3; - const MinidumpStreamType kStream1Type = static_cast(0x4d); + // An attempt to add a second stream of the same type should fail. + const size_t kStream1Size = 5; const uint8_t kStream1Value = 0xa5; auto stream1 = base::WrapUnique( - new TestStream(kStream1Type, kStream1Size, kStream1Value)); - ASSERT_DEATH_CHECK(minidump_file.AddStream(std::move(stream1)), - "already present"); + new TestStream(kStreamType, kStream1Size, kStream1Value)); + ASSERT_FALSE(minidump_file.AddStream(std::move(stream1))); + + StringFile string_file; + ASSERT_TRUE(minidump_file.WriteEverything(&string_file)); + + const size_t kDirectoryOffset = sizeof(MINIDUMP_HEADER); + const size_t kStream0Offset = kDirectoryOffset + sizeof(MINIDUMP_DIRECTORY); + const size_t kFileSize = kStream0Offset + kStream0Size; + + ASSERT_EQ(kFileSize, string_file.string().size()); + + const MINIDUMP_DIRECTORY* directory; + const MINIDUMP_HEADER* header = + MinidumpHeaderAtStart(string_file.string(), &directory); + ASSERT_NO_FATAL_FAILURE(VerifyMinidumpHeader(header, 1, 0)); + ASSERT_TRUE(directory); + + EXPECT_EQ(kStreamType, directory[0].StreamType); + EXPECT_EQ(kStream0Size, directory[0].Location.DataSize); + EXPECT_EQ(kStream0Offset, directory[0].Location.Rva); + + const uint8_t* stream_data = MinidumpWritableAtLocationDescriptor( + string_file.string(), directory[0].Location); + ASSERT_TRUE(stream_data); + + std::string expected_stream(kStream0Size, kStream0Value); + EXPECT_EQ(0, memcmp(stream_data, expected_stream.c_str(), kStream0Size)); } } // namespace diff --git a/minidump/minidump_handle_writer_test.cc b/minidump/minidump_handle_writer_test.cc index 982da713..db4cae5b 100644 --- a/minidump/minidump_handle_writer_test.cc +++ b/minidump/minidump_handle_writer_test.cc @@ -59,7 +59,7 @@ void GetHandleDataStream( TEST(MinidumpHandleDataWriter, Empty) { MinidumpFileWriter minidump_file_writer; auto handle_data_writer = base::WrapUnique(new MinidumpHandleDataWriter()); - minidump_file_writer.AddStream(std::move(handle_data_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(handle_data_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -92,7 +92,7 @@ TEST(MinidumpHandleDataWriter, OneHandle) { handle_data_writer->InitializeFromSnapshot(snapshot); - minidump_file_writer.AddStream(std::move(handle_data_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(handle_data_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -150,7 +150,7 @@ TEST(MinidumpHandleDataWriter, RepeatedTypeName) { handle_data_writer->InitializeFromSnapshot(snapshot); - minidump_file_writer.AddStream(std::move(handle_data_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(handle_data_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_memory_info_writer_test.cc b/minidump/minidump_memory_info_writer_test.cc index ef6535b7..8debff56 100644 --- a/minidump/minidump_memory_info_writer_test.cc +++ b/minidump/minidump_memory_info_writer_test.cc @@ -60,7 +60,8 @@ TEST(MinidumpMemoryInfoWriter, Empty) { MinidumpFileWriter minidump_file_writer; auto memory_info_list_writer = base::WrapUnique(new MinidumpMemoryInfoListWriter()); - minidump_file_writer.AddStream(std::move(memory_info_list_writer)); + ASSERT_TRUE( + minidump_file_writer.AddStream(std::move(memory_info_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -97,7 +98,8 @@ TEST(MinidumpMemoryInfoWriter, OneRegion) { memory_map.push_back(memory_map_region.get()); memory_info_list_writer->InitializeFromSnapshot(memory_map); - minidump_file_writer.AddStream(std::move(memory_info_list_writer)); + ASSERT_TRUE( + minidump_file_writer.AddStream(std::move(memory_info_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_memory_writer_test.cc b/minidump/minidump_memory_writer_test.cc index d9e87aee..65fcca28 100644 --- a/minidump/minidump_memory_writer_test.cc +++ b/minidump/minidump_memory_writer_test.cc @@ -79,7 +79,7 @@ TEST(MinidumpMemoryWriter, EmptyMemoryList) { MinidumpFileWriter minidump_file_writer; auto memory_list_writer = base::WrapUnique(new MinidumpMemoryListWriter()); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -107,7 +107,7 @@ TEST(MinidumpMemoryWriter, OneMemoryRegion) { new TestMinidumpMemoryWriter(kBaseAddress, kSize, kValue)); memory_list_writer->AddMemory(std::move(memory_writer)); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -148,7 +148,7 @@ TEST(MinidumpMemoryWriter, TwoMemoryRegions) { new TestMinidumpMemoryWriter(kBaseAddress1, kSize1, kValue1)); memory_list_writer->AddMemory(std::move(memory_writer_1)); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -247,7 +247,7 @@ TEST(MinidumpMemoryWriter, ExtraMemory) { auto memory_list_writer = base::WrapUnique(new MinidumpMemoryListWriter()); memory_list_writer->AddExtraMemory(test_memory_stream->memory()); - minidump_file_writer.AddStream(std::move(test_memory_stream)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(test_memory_stream))); const uint64_t kBaseAddress1 = 0x2000; const size_t kSize1 = 0x0400; @@ -257,7 +257,7 @@ TEST(MinidumpMemoryWriter, ExtraMemory) { new TestMinidumpMemoryWriter(kBaseAddress1, kSize1, kValue1)); memory_list_writer->AddMemory(std::move(memory_writer)); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -335,7 +335,7 @@ TEST(MinidumpMemoryWriter, AddFromSnapshot) { memory_list_writer->AddFromSnapshot(memory_snapshots); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_misc_info_writer_test.cc b/minidump/minidump_misc_info_writer_test.cc index 9a0c7654..8f2b5517 100644 --- a/minidump/minidump_misc_info_writer_test.cc +++ b/minidump/minidump_misc_info_writer_test.cc @@ -193,7 +193,7 @@ TEST(MinidumpMiscInfoWriter, Empty) { MinidumpFileWriter minidump_file_writer; auto misc_info_writer = base::WrapUnique(new MinidumpMiscInfoWriter()); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -214,7 +214,7 @@ TEST(MinidumpMiscInfoWriter, ProcessId) { misc_info_writer->SetProcessID(kProcessId); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -240,7 +240,7 @@ TEST(MinidumpMiscInfoWriter, ProcessTimes) { misc_info_writer->SetProcessTimes( kProcessCreateTime, kProcessUserTime, kProcessKernelTime); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -273,7 +273,7 @@ TEST(MinidumpMiscInfoWriter, ProcessorPowerInfo) { kProcessorMaxIdleState, kProcessorCurrentIdleState); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -300,7 +300,7 @@ TEST(MinidumpMiscInfoWriter, ProcessIntegrityLevel) { misc_info_writer->SetProcessIntegrityLevel(kProcessIntegrityLevel); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -323,7 +323,7 @@ TEST(MinidumpMiscInfoWriter, ProcessExecuteFlags) { misc_info_writer->SetProcessExecuteFlags(kProcessExecuteFlags); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -346,7 +346,7 @@ TEST(MinidumpMiscInfoWriter, ProtectedProcess) { misc_info_writer->SetProtectedProcess(kProtectedProcess); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -383,7 +383,7 @@ TEST(MinidumpMiscInfoWriter, TimeZone) { kDaylightDate, kDaylightBias); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -444,7 +444,7 @@ TEST(MinidumpMiscInfoWriter, TimeZoneStringsOverflow) { kSystemTimeZero, kDaylightBias); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -485,7 +485,7 @@ TEST(MinidumpMiscInfoWriter, BuildStrings) { misc_info_writer->SetBuildString(kBuildString, kDebugBuildString); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -522,7 +522,7 @@ TEST(MinidumpMiscInfoWriter, BuildStringsOverflow) { misc_info_writer->SetBuildString(build_string, debug_build_string); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -565,7 +565,7 @@ TEST(MinidumpMiscInfoWriter, XStateData) { misc_info_writer->SetXStateData(kXStateData); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -587,7 +587,7 @@ TEST(MinidumpMiscInfoWriter, ProcessCookie) { misc_info_writer->SetProcessCookie(kProcessCookie); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -649,7 +649,7 @@ TEST(MinidumpMiscInfoWriter, Everything) { kDaylightBias); misc_info_writer->SetBuildString(kBuildString, kDebugBuildString); - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -789,7 +789,7 @@ TEST(MinidumpMiscInfoWriter, InitializeFromSnapshot) { misc_info_writer->InitializeFromSnapshot(&process_snapshot); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(misc_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(misc_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_module_writer_test.cc b/minidump/minidump_module_writer_test.cc index 7c9dcdaa..f5484744 100644 --- a/minidump/minidump_module_writer_test.cc +++ b/minidump/minidump_module_writer_test.cc @@ -67,7 +67,7 @@ TEST(MinidumpModuleWriter, EmptyModuleList) { MinidumpFileWriter minidump_file_writer; auto module_list_writer = base::WrapUnique(new MinidumpModuleListWriter()); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -276,7 +276,7 @@ TEST(MinidumpModuleWriter, EmptyModule) { module_writer->SetName(kModuleName); module_list_writer->AddModule(std::move(module_writer)); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -367,7 +367,7 @@ TEST(MinidumpModuleWriter, OneModule) { module_writer->SetMiscDebugRecord(std::move(misc_debug_writer)); module_list_writer->AddModule(std::move(module_writer)); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -442,7 +442,7 @@ TEST(MinidumpModuleWriter, OneModule_CodeViewUsesPDB20_MiscUsesUTF16) { module_writer->SetMiscDebugRecord(std::move(misc_debug_writer)); module_list_writer->AddModule(std::move(module_writer)); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -534,7 +534,7 @@ TEST(MinidumpModuleWriter, ThreeModules) { module_list_writer->AddModule(std::move(module_writer_2)); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -727,7 +727,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { module_list_writer->InitializeFromSnapshot(module_snapshots); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -759,7 +759,7 @@ TEST(MinidumpModuleWriterDeathTest, NoModuleName) { auto module_list_writer = base::WrapUnique(new MinidumpModuleListWriter()); auto module_writer = base::WrapUnique(new MinidumpModuleWriter()); module_list_writer->AddModule(std::move(module_writer)); - minidump_file_writer.AddStream(std::move(module_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(module_list_writer))); StringFile string_file; ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), diff --git a/minidump/minidump_system_info_writer_test.cc b/minidump/minidump_system_info_writer_test.cc index 2a869c6b..4f35d3d1 100644 --- a/minidump/minidump_system_info_writer_test.cc +++ b/minidump/minidump_system_info_writer_test.cc @@ -82,7 +82,7 @@ TEST(MinidumpSystemInfoWriter, Empty) { system_info_writer->SetCSDVersion(std::string()); - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -153,7 +153,7 @@ TEST(MinidumpSystemInfoWriter, X86_Win) { system_info_writer->SetCPUX86VersionAndFeatures(kCPUVersion, kCPUFeatures); system_info_writer->SetCPUX86AMDExtendedFeatures(kAMDFeatures); - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -213,7 +213,7 @@ TEST(MinidumpSystemInfoWriter, AMD64_Mac) { system_info_writer->SetCSDVersion(kCSDVersion); system_info_writer->SetCPUOtherFeatures(kCPUFeatures[0], kCPUFeatures[1]); - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -255,7 +255,7 @@ TEST(MinidumpSystemInfoWriter, X86_CPUVendorFromRegisters) { kCPUVendor[0], kCPUVendor[1], kCPUVendor[2]); system_info_writer->SetCSDVersion(std::string()); - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -335,7 +335,7 @@ TEST(MinidumpSystemInfoWriter, InitializeFromSnapshot_X86) { system_info_writer->InitializeFromSnapshot(&system_snapshot); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -431,7 +431,7 @@ TEST(MinidumpSystemInfoWriter, InitializeFromSnapshot_AMD64) { system_info_writer->InitializeFromSnapshot(&system_snapshot); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -469,7 +469,7 @@ TEST(MinidumpSystemInfoWriter, InitializeFromSnapshot_AMD64) { TEST(MinidumpSystemInfoWriterDeathTest, NoCSDVersion) { MinidumpFileWriter minidump_file_writer; auto system_info_writer = base::WrapUnique(new MinidumpSystemInfoWriter()); - minidump_file_writer.AddStream(std::move(system_info_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(system_info_writer))); StringFile string_file; ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), diff --git a/minidump/minidump_thread_writer_test.cc b/minidump/minidump_thread_writer_test.cc index 1384efe3..61332363 100644 --- a/minidump/minidump_thread_writer_test.cc +++ b/minidump/minidump_thread_writer_test.cc @@ -81,7 +81,7 @@ TEST(MinidumpThreadWriter, EmptyThreadList) { MinidumpFileWriter minidump_file_writer; auto thread_list_writer = base::WrapUnique(new MinidumpThreadListWriter()); - minidump_file_writer.AddStream(std::move(thread_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -161,7 +161,7 @@ TEST(MinidumpThreadWriter, OneThread_x86_NoStack) { thread_writer->SetContext(std::move(context_x86_writer)); thread_list_writer->AddThread(std::move(thread_writer)); - minidump_file_writer.AddStream(std::move(thread_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -231,7 +231,7 @@ TEST(MinidumpThreadWriter, OneThread_AMD64_Stack) { thread_writer->SetContext(std::move(context_amd64_writer)); thread_list_writer->AddThread(std::move(thread_writer)); - minidump_file_writer.AddStream(std::move(thread_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -363,8 +363,8 @@ TEST(MinidumpThreadWriter, ThreeThreads_x86_MemoryList) { thread_list_writer->AddThread(std::move(thread_writer_2)); - minidump_file_writer.AddStream(std::move(thread_list_writer)); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -623,8 +623,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) { thread_list_writer->InitializeFromSnapshot(thread_snapshots, &thread_id_map); MinidumpFileWriter minidump_file_writer; - minidump_file_writer.AddStream(std::move(thread_list_writer)); - minidump_file_writer.AddStream(std::move(memory_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(memory_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -702,7 +702,7 @@ TEST(MinidumpThreadWriterDeathTest, NoContext) { auto thread_writer = base::WrapUnique(new MinidumpThreadWriter()); thread_list_writer->AddThread(std::move(thread_writer)); - minidump_file_writer.AddStream(std::move(thread_list_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(thread_list_writer))); StringFile string_file; ASSERT_DEATH_CHECK(minidump_file_writer.WriteEverything(&string_file), diff --git a/minidump/minidump_unloaded_module_writer_test.cc b/minidump/minidump_unloaded_module_writer_test.cc index dba46918..002c82bb 100644 --- a/minidump/minidump_unloaded_module_writer_test.cc +++ b/minidump/minidump_unloaded_module_writer_test.cc @@ -82,7 +82,8 @@ TEST(MinidumpUnloadedModuleWriter, EmptyModule) { unloaded_module_list_writer->AddUnloadedModule( std::move(unloaded_module_writer)); - minidump_file_writer.AddStream(std::move(unloaded_module_list_writer)); + ASSERT_TRUE( + minidump_file_writer.AddStream(std::move(unloaded_module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -128,7 +129,8 @@ TEST(MinidumpUnloadedModuleWriter, OneModule) { unloaded_module_list_writer->AddUnloadedModule( std::move(unloaded_module_writer)); - minidump_file_writer.AddStream(std::move(unloaded_module_list_writer)); + ASSERT_TRUE( + minidump_file_writer.AddStream(std::move(unloaded_module_list_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); diff --git a/minidump/minidump_user_stream_writer_test.cc b/minidump/minidump_user_stream_writer_test.cc index 97d41eec..e4e8cccb 100644 --- a/minidump/minidump_user_stream_writer_test.cc +++ b/minidump/minidump_user_stream_writer_test.cc @@ -57,7 +57,7 @@ TEST(MinidumpUserStreamWriter, NoData) { auto stream = base::WrapUnique(new UserMinidumpStream(kTestStreamId, nullptr)); user_stream_writer->InitializeFromSnapshot(stream.get()); - minidump_file_writer.AddStream(std::move(user_stream_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(user_stream_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); @@ -84,7 +84,7 @@ TEST(MinidumpUserStreamWriter, OneStream) { auto stream = base::WrapUnique(new UserMinidumpStream(kTestStreamId, test_data)); user_stream_writer->InitializeFromSnapshot(stream.get()); - minidump_file_writer.AddStream(std::move(user_stream_writer)); + ASSERT_TRUE(minidump_file_writer.AddStream(std::move(user_stream_writer))); StringFile string_file; ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); From 51b21d887467983280ece88f717956b4d1ed6dc8 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 16 Mar 2017 10:52:27 -0400 Subject: [PATCH 11/19] =?UTF-8?q?Add=20DelimitedFileReader=20and=20use=20i?= =?UTF-8?q?t=20in=20Linux/Android=E2=80=99s=20ProcessInfo?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Mark Mentovai --- util/file/delimited_file_reader.cc | 109 +++++++ util/file/delimited_file_reader.h | 93 ++++++ util/file/delimited_file_reader_test.cc | 363 ++++++++++++++++++++++++ util/posix/process_info_linux.cc | 117 ++++---- util/util.gyp | 2 + util/util_test.gyp | 1 + 6 files changed, 634 insertions(+), 51 deletions(-) create mode 100644 util/file/delimited_file_reader.cc create mode 100644 util/file/delimited_file_reader.h create mode 100644 util/file/delimited_file_reader_test.cc diff --git a/util/file/delimited_file_reader.cc b/util/file/delimited_file_reader.cc new file mode 100644 index 00000000..a55c2a7c --- /dev/null +++ b/util/file/delimited_file_reader.cc @@ -0,0 +1,109 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/file/delimited_file_reader.h" + +#include + +#include +#include + +#include "base/logging.h" +#include "base/numerics/safe_conversions.h" + +namespace crashpad { + +DelimitedFileReader::DelimitedFileReader(FileReaderInterface* file_reader) + : file_reader_(file_reader), buf_pos_(0), buf_len_(0), eof_(false) { + static_assert(sizeof(buf_) <= std::numeric_limits::max(), + "buf_pos_ must cover buf_"); + static_assert(sizeof(buf_) <= std::numeric_limits::max(), + "buf_len_ must cover buf_"); +} + +DelimitedFileReader::~DelimitedFileReader() {} + +DelimitedFileReader::Result DelimitedFileReader::GetDelim(char delimiter, + std::string* field) { + if (eof_) { + DCHECK_EQ(buf_pos_, buf_len_); + + // Allow subsequent calls to attempt to read more data from the file. If the + // file is still at EOF in the future, the read will return 0 and cause + // kEndOfFile to be returned anyway. + eof_ = false; + + return Result::kEndOfFile; + } + + std::string local_field; + while (true) { + if (buf_pos_ == buf_len_) { + // buf_ is empty. Refill it. + FileOperationResult read_result = file_reader_->Read(buf_, sizeof(buf_)); + if (read_result < 0) { + return Result::kError; + } else if (read_result == 0) { + if (!local_field.empty()) { + // The file ended with a field that wasn’t terminated by a delimiter + // character. + // + // This is EOF, but EOF can’t be returned because there’s a field that + // needs to be returned to the caller. Cache the detected EOF so it + // can be returned next time. This is done to support proper semantics + // for weird “files” like terminal input that can reach EOF and then + // “grow”, allowing subsequent reads past EOF to block while waiting + // for more data. Once EOF is detected by a read that returns 0, that + // EOF signal should propagate to the caller before attempting a new + // read. Here, it will be returned on the next call to this method + // without attempting to read more data. + eof_ = true; + field->swap(local_field); + return Result::kSuccess; + } + return Result::kEndOfFile; + } + + DCHECK_LE(static_cast(read_result), arraysize(buf_)); + DCHECK( + base::IsValueInRangeForNumericType(read_result)); + buf_len_ = static_cast(read_result); + buf_pos_ = 0; + } + + const char* const start = buf_ + buf_pos_; + const char* const end = buf_ + buf_len_; + const char* const found = std::find(start, end, delimiter); + + local_field.append(start, found); + buf_pos_ = static_cast(found - buf_); + DCHECK_LE(buf_pos_, buf_len_); + + if (found != end) { + // A real delimiter character was found. Append it to the field being + // built and return it. + local_field.push_back(delimiter); + ++buf_pos_; + DCHECK_LE(buf_pos_, buf_len_); + field->swap(local_field); + return Result::kSuccess; + } + } +} + +DelimitedFileReader::Result DelimitedFileReader::GetLine(std::string* line) { + return GetDelim('\n', line); +} + +} // namespace crashpad diff --git a/util/file/delimited_file_reader.h b/util/file/delimited_file_reader.h new file mode 100644 index 00000000..93fd7380 --- /dev/null +++ b/util/file/delimited_file_reader.h @@ -0,0 +1,93 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_FILE_DELIMITED_FILE_READER_H_ +#define CRASHPAD_UTIL_FILE_DELIMITED_FILE_READER_H_ + +#include + +#include + +#include "base/macros.h" +#include "util/file/file_reader.h" + +namespace crashpad { + +//! \brief Reads a file one field or line at a time. +//! +//! The file is interpreted as a series of fields separated by delimiter +//! characters. When the delimiter character is the newline character +//! ('\\n'), the file is interpreted as a series of lines. +//! +//! It is safe to mix GetDelim() and GetLine() calls, if appropriate for the +//! format being interpreted. +//! +//! This is a replacement for the standard library’s `getdelim()` and +//! `getline()` functions, adapted to work with FileReaderInterface objects +//! instead of `FILE*` streams. +class DelimitedFileReader { + public: + //! \brief The result of a GetDelim() or GetLine() call. + enum class Result { + //! \brief An error occurred, and a message was logged. + kError = -1, + + //! \brief A field or line was read from the file. + kSuccess, + + //! \brief The end of the file was encountered. + kEndOfFile, + }; + + explicit DelimitedFileReader(FileReaderInterface* file_reader); + ~DelimitedFileReader(); + + //! \brief Reads a single field from the file. + //! + //! \param[in] delimiter The delimiter character that terminates the field. + //! It is safe to call this method multiple times while changing the value + //! of this parameter, if appropriate for the format being interpreted. + //! \param[out] field The field read from the file. This parameter will + //! include the field’s terminating delimiter character unless the field + //! was at the end of the file and was read without such a character. + //! This parameter will not be empty. + //! + //! \return a #Result value. \a field is only valid when Result::kSuccess is + //! returned. + Result GetDelim(char delimiter, std::string* field); + + //! \brief Reads a single line from the file. + //! + //! \param[out] line The line read from the file. This parameter will include + //! the line terminating delimiter character unless the line was at the + //! end of the file and was read without such a character. This parameter + //! will not be empty. + //! + //! \return a #Result value. \a line is only valid when Result::kSuccess is + //! returned. + Result GetLine(std::string* line); + + private: + char buf_[4096]; + FileReaderInterface* file_reader_; // weak + uint16_t buf_pos_; // Index into buf_ of the start of the next field. + uint16_t buf_len_; // The size of buf_ that’s been filled. + bool eof_; // Caches the EOF signal when detected following a partial field. + + DISALLOW_COPY_AND_ASSIGN(DelimitedFileReader); +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_FILE_DELIMITED_FILE_READER_H_ diff --git a/util/file/delimited_file_reader_test.cc b/util/file/delimited_file_reader_test.cc new file mode 100644 index 00000000..63b50836 --- /dev/null +++ b/util/file/delimited_file_reader_test.cc @@ -0,0 +1,363 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/file/delimited_file_reader.h" + +#include + +#include "base/format_macros.h" +#include "base/strings/stringprintf.h" +#include "gtest/gtest.h" +#include "util/file/string_file.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(DelimitedFileReader, EmptyFile) { + StringFile string_file; + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, EmptyOneLineFile) { + StringFile string_file; + string_file.SetString("\n"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(string_file.string(), line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, SmallOneLineFile) { + StringFile string_file; + string_file.SetString("one line\n"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(string_file.string(), line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, SmallOneLineFileWithoutNewline) { + StringFile string_file; + string_file.SetString("no newline"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(string_file.string(), line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, SmallMultiLineFile) { + StringFile string_file; + string_file.SetString("first\nsecond line\n3rd\n"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("first\n", line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("second line\n", line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("3rd\n", line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, SmallMultiFieldFile) { + StringFile string_file; + string_file.SetString("first,second field\ntwo lines,3rd,"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string field; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim(',', &field)); + EXPECT_EQ("first,", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim(',', &field)); + EXPECT_EQ("second field\ntwo lines,", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim(',', &field)); + EXPECT_EQ("3rd,", field); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetDelim(',', &field)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetDelim(',', &field)); +} + +TEST(DelimitedFileReader, SmallMultiFieldFile_MixedDelimiters) { + StringFile string_file; + string_file.SetString("first,second, still 2nd\t3rd\nalso\tnewline\n55555$"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string field; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim(',', &field)); + EXPECT_EQ("first,", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('\t', &field)); + EXPECT_EQ("second, still 2nd\t", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&field)); + EXPECT_EQ("3rd\n", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('\n', &field)); + EXPECT_EQ("also\tnewline\n", field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('$', &field)); + EXPECT_EQ("55555$", field); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetDelim('?', &field)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&field)); +} + +TEST(DelimitedFileReader, EmptyLineMultiLineFile) { + StringFile string_file; + string_file.SetString("first\n\n\n4444\n"); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("first\n", line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("\n", line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("\n", line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ("4444\n", line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, LongOneLineFile) { + std::string contents(50000, '!'); + contents[1] = '?'; + contents.push_back('\n'); + + StringFile string_file; + string_file.SetString(contents); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(contents, line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +void TestLongMultiLineFile(int base_length) { + std::vector lines; + std::string contents; + for (size_t line_index = 0; line_index <= 'z' - 'a'; ++line_index) { + char c = 'a' + static_cast(line_index); + + // Mix up the lengths a little. + std::string line(base_length + line_index * ((line_index % 3) - 1), c); + + // Mix up the data a little too. + ASSERT_LT(line_index, line.size()); + line[line_index] -= ('a' - 'A'); + + line.push_back('\n'); + contents.append(line); + lines.push_back(line); + } + + StringFile string_file; + string_file.SetString(contents); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + for (size_t line_index = 0; line_index < lines.size(); ++line_index) { + SCOPED_TRACE(base::StringPrintf("line_index %" PRIuS, line_index)); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(lines[line_index], line); + } + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, LongMultiLineFile) { + TestLongMultiLineFile(500); +} + +TEST(DelimitedFileReader, ReallyLongMultiLineFile) { + TestLongMultiLineFile(5000); +} + +TEST(DelimitedFileReader, EmbeddedNUL) { + const char kString[] = "embedded\0NUL\n"; + StringFile string_file; + string_file.SetString(std::string(kString, arraysize(kString) - 1)); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(string_file.string(), line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); +} + +TEST(DelimitedFileReader, NULDelimiter) { + const char kString[] = "aa\0b\0ccc\0"; + StringFile string_file; + string_file.SetString(std::string(kString, arraysize(kString) - 1)); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string field; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('\0', &field)); + EXPECT_EQ(std::string("aa\0", 3), field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('\0', &field)); + EXPECT_EQ(std::string("b\0", 2), field); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetDelim('\0', &field)); + EXPECT_EQ(std::string("ccc\0", 4), field); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetDelim('\0', &field)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetDelim('\0', &field)); +} + +TEST(DelimitedFileReader, EdgeCases) { + const size_t kSizes[] = {4094, 4095, 4096, 4097, 8190, 8191, 8192, 8193}; + for (size_t index = 0; index < arraysize(kSizes); ++index) { + size_t size = kSizes[index]; + SCOPED_TRACE( + base::StringPrintf("index %" PRIuS ", size %" PRIuS, index, size)); + + std::string line_0(size, '$'); + line_0.push_back('\n'); + + StringFile string_file; + string_file.SetString(line_0); + DelimitedFileReader delimited_file_reader(&string_file); + + std::string line; + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(line_0, line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + std::string line_1(size, '@'); + line_1.push_back('\n'); + + string_file.SetString(line_0 + line_1); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(line_0, line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(line_1, line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + line_1[size] = '?'; + + string_file.SetString(line_0 + line_1); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(line_0, line); + ASSERT_EQ(DelimitedFileReader::Result::kSuccess, + delimited_file_reader.GetLine(&line)); + EXPECT_EQ(line_1, line); + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + + // The file is still at EOF. + EXPECT_EQ(DelimitedFileReader::Result::kEndOfFile, + delimited_file_reader.GetLine(&line)); + } +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index f553cdac..bd4202a8 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -17,7 +17,6 @@ #include #include #include -#include #include #include #include @@ -26,13 +25,14 @@ #include #include +#include "base/files/file_path.h" #include "base/files/scoped_file.h" #include "base/logging.h" #include "base/posix/eintr_wrapper.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_piece.h" +#include "util/file/delimited_file_reader.h" #include "util/file/file_reader.h" -#include "util/string/split_string.h" namespace crashpad { @@ -78,14 +78,6 @@ bool AdvancePastNumber(const char** input, T* value) { return false; } -struct BufferFreer { - void operator()(char** buffer_ptr) const { - free(*buffer_ptr); - *buffer_ptr = nullptr; - } -}; -using ScopedBufferPtr = std::unique_ptr; - bool ReadEntireFile(const char* path, std::string* contents) { FileReader file; if (!file.Open(base::FilePath(path))) { @@ -158,86 +150,98 @@ bool ProcessInfo::Initialize(pid_t pid) { { char path[32]; snprintf(path, sizeof(path), "/proc/%d/status", pid_); - base::ScopedFILE status_file(fopen(path, "re")); - if (!status_file.get()) { - PLOG(ERROR) << "fopen " << path; + FileReader status_file; + if (!status_file.Open(base::FilePath(path))) { return false; } - size_t buffer_size = 0; - char* buffer = nullptr; - ScopedBufferPtr buffer_owner(&buffer); + DelimitedFileReader status_file_line_reader(&status_file); bool have_ppid = false; bool have_uids = false; bool have_gids = false; bool have_groups = false; - ssize_t len; - while ((len = getline(&buffer, &buffer_size, status_file.get())) > 0) { - const char* line = buffer; + std::string line; + DelimitedFileReader::Result result; + while ((result = status_file_line_reader.GetLine(&line)) == + DelimitedFileReader::Result::kSuccess) { + if (line.back() != '\n') { + LOG(ERROR) << "format error: unterminated line at EOF"; + return false; + } - if (AdvancePastPrefix(&line, "PPid:\t")) { + bool understood_line = false; + const char* line_c = line.c_str(); + if (AdvancePastPrefix(&line_c, "PPid:\t")) { if (have_ppid) { LOG(ERROR) << "format error: multiple PPid lines"; return false; } - have_ppid = AdvancePastNumber(&line, &ppid_); + have_ppid = AdvancePastNumber(&line_c, &ppid_); if (!have_ppid) { LOG(ERROR) << "format error: unrecognized PPid format"; return false; } - } else if (AdvancePastPrefix(&line, "Uid:\t")) { + understood_line = true; + } else if (AdvancePastPrefix(&line_c, "Uid:\t")) { if (have_uids) { LOG(ERROR) << "format error: multiple Uid lines"; return false; } - have_uids = - AdvancePastNumber(&line, &uid_) && - AdvancePastPrefix(&line, "\t") && - AdvancePastNumber(&line, &euid_) && - AdvancePastPrefix(&line, "\t") && - AdvancePastNumber(&line, &suid_); + uid_t fsuid; + have_uids = AdvancePastNumber(&line_c, &uid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &euid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &suid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &fsuid); if (!have_uids) { LOG(ERROR) << "format error: unrecognized Uid format"; return false; } - } else if (AdvancePastPrefix(&line, "Gid:\t")) { + understood_line = true; + } else if (AdvancePastPrefix(&line_c, "Gid:\t")) { if (have_gids) { LOG(ERROR) << "format error: multiple Gid lines"; return false; } - have_gids = - AdvancePastNumber(&line, &gid_) && - AdvancePastPrefix(&line, "\t") && - AdvancePastNumber(&line, &egid_) && - AdvancePastPrefix(&line, "\t") && - AdvancePastNumber(&line, &sgid_); + gid_t fsgid; + have_gids = AdvancePastNumber(&line_c, &gid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &egid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &sgid_) && + AdvancePastPrefix(&line_c, "\t") && + AdvancePastNumber(&line_c, &fsgid); if (!have_gids) { LOG(ERROR) << "format error: unrecognized Gid format"; return false; } - } else if (AdvancePastPrefix(&line, "Groups:\t")) { + understood_line = true; + } else if (AdvancePastPrefix(&line_c, "Groups:\t")) { if (have_groups) { LOG(ERROR) << "format error: multiple Groups lines"; return false; } gid_t group; - while (AdvancePastNumber(&line, &group)) { + while (AdvancePastNumber(&line_c, &group)) { supplementary_groups_.insert(group); - if (!AdvancePastPrefix(&line, " ")) { - LOG(ERROR) << "format error"; + if (!AdvancePastPrefix(&line_c, " ")) { + LOG(ERROR) << "format error: unrecognized Groups format"; return false; } } - if (!AdvancePastPrefix(&line, "\n") || line != buffer + len) { - LOG(ERROR) << "format error: unrecognized Groups format"; - return false; - } have_groups = true; + understood_line = true; + } + + if (understood_line && line_c != &line.back()) { + LOG(ERROR) << "format error: unconsumed trailing data"; + return false; } } - if (!feof(status_file.get())) { - PLOG(ERROR) << "getline"; + if (result != DelimitedFileReader::Result::kEndOfFile) { return false; } if (!have_ppid || !have_uids || !have_gids || !have_groups) { @@ -486,18 +490,29 @@ bool ProcessInfo::Arguments(std::vector* argv) const { char path[32]; snprintf(path, sizeof(path), "/proc/%d/cmdline", pid_); - std::string command; - if (!ReadEntireFile(path, &command)) { + FileReader cmdline_file; + if (!cmdline_file.Open(base::FilePath(path))) { return false; } - if (command.size() == 0 || command.back() != '\0') { - LOG(ERROR) << "format error"; + DelimitedFileReader cmdline_file_field_reader(&cmdline_file); + + std::vector local_argv; + std::string argument; + DelimitedFileReader::Result result; + while ((result = cmdline_file_field_reader.GetDelim('\0', &argument)) == + DelimitedFileReader::Result::kSuccess) { + if (argument.back() != '\0') { + LOG(ERROR) << "format error"; + return false; + } + argument.pop_back(); + local_argv.push_back(argument); + } + if (result != DelimitedFileReader::Result::kEndOfFile) { return false; } - command.pop_back(); - std::vector local_argv = SplitString(command, '\0'); argv->swap(local_argv); return true; } diff --git a/util/util.gyp b/util/util.gyp index c715233b..4890012a 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -30,6 +30,8 @@ '<(INTERMEDIATE_DIR)', ], 'sources': [ + 'file/delimited_file_reader.cc', + 'file/delimited_file_reader.h', 'file/file_io.cc', 'file/file_io.h', 'file/file_io_posix.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 45ac6bd5..bf579810 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -35,6 +35,7 @@ '..', ], 'sources': [ + 'file/delimited_file_reader_test.cc', 'file/file_io_test.cc', 'file/string_file_test.cc', 'mac/launchd_test.mm', From 00b64427523b2413a0f2711dcf2f9caf0c6d6c94 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 16 Mar 2017 13:36:38 -0400 Subject: [PATCH 12/19] Make file_io reads more rational and predictable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- client/crash_report_database_win.cc | 8 +- client/settings.cc | 11 +-- handler/win/crash_other_program.cc | 3 +- snapshot/api/module_annotations_win_test.cc | 4 +- .../mach_o_image_annotations_reader_test.cc | 4 +- snapshot/mac/process_reader_test.cc | 26 +++--- ...shpad_snapshot_test_extra_memory_ranges.cc | 2 +- ...ashpad_snapshot_test_simple_annotations.cc | 2 +- snapshot/win/exception_snapshot_win_test.cc | 12 +-- snapshot/win/extra_memory_ranges_test.cc | 2 +- .../win/pe_image_annotations_reader_test.cc | 2 +- snapshot/win/process_reader_win_test.cc | 4 +- snapshot/win/process_snapshot_win_test.cc | 3 +- test/multiprocess_exec_test.cc | 2 +- test/multiprocess_posix_test.cc | 6 +- test/win/win_child_process.cc | 2 +- tools/crashpad_database_util.cc | 2 +- util/file/file_io.cc | 59 ++++++++++--- util/file/file_io.h | 82 +++++++++++++------ util/file/file_io_posix.cc | 82 ++++++++----------- util/file/file_io_win.cc | 35 ++++---- util/file/file_reader.cc | 35 +++++--- util/file/file_reader.h | 2 +- util/mach/child_port_handshake.cc | 8 +- util/mach/exception_ports_test.cc | 4 +- util/mach/mach_message_server_test.cc | 6 +- util/net/http_transport_test.cc | 2 +- util/win/exception_handler_server.cc | 3 +- util/win/exception_handler_server_test.cc | 5 +- util/win/process_info_test.cc | 2 +- util/win/scoped_process_suspend_test.cc | 2 +- 31 files changed, 246 insertions(+), 176 deletions(-) diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index 4fccd3fd..6629d25d 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -77,8 +77,8 @@ std::string ReadRestOfFileAsString(FileHandle file) { DCHECK_GT(end, read_from); size_t data_length = static_cast(end - read_from); std::string buffer(data_length, '\0'); - return LoggingReadFile(file, &buffer[0], data_length) ? buffer - : std::string(); + return LoggingReadFileExactly(file, &buffer[0], data_length) ? buffer + : std::string(); } // Helper structures, and conversions ------------------------------------------ @@ -417,7 +417,7 @@ void Metadata::Read() { } MetadataFileHeader header; - if (!LoggingReadFile(handle_.get(), &header, sizeof(header))) { + if (!LoggingReadFileExactly(handle_.get(), &header, sizeof(header))) { LOG(ERROR) << "failed to read header"; return; } @@ -438,7 +438,7 @@ void Metadata::Read() { std::vector reports; if (header.num_records > 0) { std::vector records(header.num_records); - if (!LoggingReadFile( + if (!LoggingReadFileExactly( handle_.get(), &records[0], records_size.ValueOrDie())) { LOG(ERROR) << "failed to read records"; return; diff --git a/client/settings.cc b/client/settings.cc index 7757ecb0..15d16f2e 100644 --- a/client/settings.cc +++ b/client/settings.cc @@ -246,13 +246,10 @@ bool Settings::ReadSettings(FileHandle handle, if (LoggingSeekFile(handle, 0, SEEK_SET) != 0) return false; - bool read_result; - if (log_read_error) { - read_result = LoggingReadFile(handle, out_data, sizeof(*out_data)); - } else { - read_result = - ReadFile(handle, out_data, sizeof(*out_data)) == sizeof(*out_data); - } + bool read_result = + log_read_error + ? LoggingReadFileExactly(handle, out_data, sizeof(*out_data)) + : ReadFileExactly(handle, out_data, sizeof(*out_data)); if (!read_result) return false; diff --git a/handler/win/crash_other_program.cc b/handler/win/crash_other_program.cc index d191aac0..012b4ba3 100644 --- a/handler/win/crash_other_program.cc +++ b/handler/win/crash_other_program.cc @@ -90,7 +90,8 @@ int CrashOtherProgram(int argc, wchar_t* argv[]) { // Wait until it's ready. char c; - if (!LoggingReadFile(child.stdout_read_handle(), &c, sizeof(c)) || c != ' ') { + if (!LoggingReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)) || + c != ' ') { LOG(ERROR) << "failed child communication"; return EXIT_FAILURE; } diff --git a/snapshot/api/module_annotations_win_test.cc b/snapshot/api/module_annotations_win_test.cc index 30e880f3..ee028683 100644 --- a/snapshot/api/module_annotations_win_test.cc +++ b/snapshot/api/module_annotations_win_test.cc @@ -28,7 +28,7 @@ class ModuleAnnotationsMultiprocessTest final : public WinMultiprocess { void WinMultiprocessParent() override { // Read the child executable module. HMODULE module = nullptr; - CheckedReadFile(ReadPipeHandle(), &module, sizeof(module)); + CheckedReadFileExactly(ReadPipeHandle(), &module, sizeof(module)); // Reopen the child process with necessary access. HANDLE process_handle = @@ -70,7 +70,7 @@ class ModuleAnnotationsMultiprocessTest final : public WinMultiprocess { // Wait until a signal from the parent process to terminate. char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); } }; diff --git a/snapshot/mac/mach_o_image_annotations_reader_test.cc b/snapshot/mac/mach_o_image_annotations_reader_test.cc index 7a94eb2c..50e646ae 100644 --- a/snapshot/mac/mach_o_image_annotations_reader_test.cc +++ b/snapshot/mac/mach_o_image_annotations_reader_test.cc @@ -245,7 +245,7 @@ class TestMachOImageAnnotationsReader final // Wait for the child process to indicate that it’s done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); // Verify the “simple map” annotations set via the CrashpadInfo interface. const std::vector& modules = @@ -333,7 +333,7 @@ class TestMachOImageAnnotationsReader final CheckedWriteFile(WritePipeHandle(), &c, sizeof(c)); // Wait for the parent to indicate that it’s safe to crash. - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); // Direct an exception message to the exception server running in the // parent. diff --git a/snapshot/mac/process_reader_test.cc b/snapshot/mac/process_reader_test.cc index e679d2ef..87fb6394 100644 --- a/snapshot/mac/process_reader_test.cc +++ b/snapshot/mac/process_reader_test.cc @@ -105,7 +105,7 @@ class ProcessReaderChild final : public MachMultiprocess { FileHandle read_handle = ReadPipeHandle(); mach_vm_address_t address; - CheckedReadFile(read_handle, &address, sizeof(address)); + CheckedReadFileExactly(read_handle, &address, sizeof(address)); std::string read_string; ASSERT_TRUE(process_reader.Memory()->ReadCString(address, &read_string)); @@ -448,15 +448,15 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { thread_index < thread_count_ + 1; ++thread_index) { uint64_t thread_id; - CheckedReadFile(read_handle, &thread_id, sizeof(thread_id)); + CheckedReadFileExactly(read_handle, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; - CheckedReadFile(read_handle, - &expectation.stack_address, - sizeof(expectation.stack_address)); - CheckedReadFile(read_handle, - &expectation.suspend_count, - sizeof(expectation.suspend_count)); + CheckedReadFileExactly(read_handle, + &expectation.stack_address, + sizeof(expectation.stack_address)); + CheckedReadFileExactly(read_handle, + &expectation.suspend_count, + sizeof(expectation.suspend_count)); // There can’t be any duplicate thread IDs. EXPECT_EQ(0u, thread_map.count(thread_id)); @@ -730,7 +730,8 @@ class ProcessReaderModulesChild final : public MachMultiprocess { FileHandle read_handle = ReadPipeHandle(); uint32_t expect_modules; - CheckedReadFile(read_handle, &expect_modules, sizeof(expect_modules)); + CheckedReadFileExactly( + read_handle, &expect_modules, sizeof(expect_modules)); ASSERT_EQ(expect_modules, modules.size()); @@ -740,16 +741,17 @@ class ProcessReaderModulesChild final : public MachMultiprocess { "index %zu, name %s", index, modules[index].name.c_str())); uint32_t expect_name_length; - CheckedReadFile( + CheckedReadFileExactly( read_handle, &expect_name_length, sizeof(expect_name_length)); // The NUL terminator is not read. std::string expect_name(expect_name_length, '\0'); - CheckedReadFile(read_handle, &expect_name[0], expect_name_length); + CheckedReadFileExactly(read_handle, &expect_name[0], expect_name_length); EXPECT_EQ(expect_name, modules[index].name); mach_vm_address_t expect_address; - CheckedReadFile(read_handle, &expect_address, sizeof(expect_address)); + CheckedReadFileExactly( + read_handle, &expect_address, sizeof(expect_address)); ASSERT_TRUE(modules[index].reader); EXPECT_EQ(expect_address, modules[index].reader->Address()); diff --git a/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc b/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc index 58b145e5..e6875fdd 100644 --- a/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc +++ b/snapshot/win/crashpad_snapshot_test_extra_memory_ranges.cc @@ -43,7 +43,7 @@ int wmain(int argc, wchar_t* argv[]) { HANDLE in = GetStdHandle(STD_INPUT_HANDLE); PCHECK(in != INVALID_HANDLE_VALUE) << "GetStdHandle"; - CheckedReadFile(in, &c, sizeof(c)); + CheckedReadFileExactly(in, &c, sizeof(c)); CHECK(c == 'd' || c == ' '); // If 'd' we crash with a debug break, otherwise exit normally. diff --git a/snapshot/win/crashpad_snapshot_test_simple_annotations.cc b/snapshot/win/crashpad_snapshot_test_simple_annotations.cc index b66a19e1..11e7b4e7 100644 --- a/snapshot/win/crashpad_snapshot_test_simple_annotations.cc +++ b/snapshot/win/crashpad_snapshot_test_simple_annotations.cc @@ -42,7 +42,7 @@ int wmain(int argc, wchar_t* argv[]) { HANDLE in = GetStdHandle(STD_INPUT_HANDLE); PCHECK(in != INVALID_HANDLE_VALUE) << "GetStdHandle"; - crashpad::CheckedReadFile(in, &c, sizeof(c)); + crashpad::CheckedReadFileExactly(in, &c, sizeof(c)); CHECK(c == 'd' || c == ' '); // If 'd' we crash with a debug break, otherwise exit normally. diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index af0cc3a6..03aa0499 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -148,9 +148,9 @@ void TestCrashingChild(const base::string16& directory_modification) { // The child tells us (approximately) where it will crash. WinVMAddress break_near_address; - LoggingReadFile(child.stdout_read_handle(), - &break_near_address, - sizeof(break_near_address)); + LoggingReadFileExactly(child.stdout_read_handle(), + &break_near_address, + sizeof(break_near_address)); delegate.set_break_near(break_near_address); // Wait for the child to crash and the exception information to be validated. @@ -250,9 +250,9 @@ void TestDumpWithoutCrashingChild( // The child tells us (approximately) where it will capture a dump. WinVMAddress dump_near_address; - LoggingReadFile(child.stdout_read_handle(), - &dump_near_address, - sizeof(dump_near_address)); + LoggingReadFileExactly(child.stdout_read_handle(), + &dump_near_address, + sizeof(dump_near_address)); delegate.set_dump_near(dump_near_address); // Wait for the child to crash and the exception information to be validated. diff --git a/snapshot/win/extra_memory_ranges_test.cc b/snapshot/win/extra_memory_ranges_test.cc index d6d56596..57d41bb5 100644 --- a/snapshot/win/extra_memory_ranges_test.cc +++ b/snapshot/win/extra_memory_ranges_test.cc @@ -58,7 +58,7 @@ void TestExtraMemoryRanges(TestType type, // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(child.stdout_read_handle(), &c, sizeof(c)); + CheckedReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)); ProcessSnapshotWin snapshot; ASSERT_TRUE(snapshot.Initialize( diff --git a/snapshot/win/pe_image_annotations_reader_test.cc b/snapshot/win/pe_image_annotations_reader_test.cc index 523a4494..f791f80e 100644 --- a/snapshot/win/pe_image_annotations_reader_test.cc +++ b/snapshot/win/pe_image_annotations_reader_test.cc @@ -62,7 +62,7 @@ void TestAnnotationsOnCrash(TestType type, // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. char c; - CheckedReadFile(child.stdout_read_handle(), &c, sizeof(c)); + CheckedReadFileExactly(child.stdout_read_handle(), &c, sizeof(c)); ProcessReaderWin process_reader; ASSERT_TRUE(process_reader.Initialize(child.process_handle(), diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index b9befc9a..af57c622 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -69,7 +69,7 @@ class ProcessReaderChild final : public WinMultiprocess { #endif WinVMAddress address; - CheckedReadFile(ReadPipeHandle(), &address, sizeof(address)); + CheckedReadFileExactly(ReadPipeHandle(), &address, sizeof(address)); char buffer[sizeof(kTestMemory)]; ASSERT_TRUE( @@ -142,7 +142,7 @@ class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { void WinMultiprocessParent() override { char c; - CheckedReadFile(ReadPipeHandle(), &c, sizeof(c)); + CheckedReadFileExactly(ReadPipeHandle(), &c, sizeof(c)); ASSERT_EQ(' ', c); { diff --git a/snapshot/win/process_snapshot_win_test.cc b/snapshot/win/process_snapshot_win_test.cc index a3e31a4e..82b29035 100644 --- a/snapshot/win/process_snapshot_win_test.cc +++ b/snapshot/win/process_snapshot_win_test.cc @@ -48,7 +48,8 @@ void TestImageReaderChild(const base::string16& directory_modification) { child.Start(); char c; - ASSERT_TRUE(LoggingReadFile(child.stdout_read_handle(), &c, sizeof(c))); + ASSERT_TRUE( + LoggingReadFileExactly(child.stdout_read_handle(), &c, sizeof(c))); ASSERT_EQ(' ', c); ScopedProcessSuspend suspend(child.process_handle()); diff --git a/test/multiprocess_exec_test.cc b/test/multiprocess_exec_test.cc index f6231e9c..3e958aa8 100644 --- a/test/multiprocess_exec_test.cc +++ b/test/multiprocess_exec_test.cc @@ -39,7 +39,7 @@ class TestMultiprocessExec final : public MultiprocessExec { char c = 'z'; ASSERT_TRUE(LoggingWriteFile(WritePipeHandle(), &c, 1)); - ASSERT_TRUE(LoggingReadFile(ReadPipeHandle(), &c, 1)); + ASSERT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, 1)); EXPECT_EQ('Z', c); } diff --git a/test/multiprocess_posix_test.cc b/test/multiprocess_posix_test.cc index be927111..9f557fab 100644 --- a/test/multiprocess_posix_test.cc +++ b/test/multiprocess_posix_test.cc @@ -39,11 +39,11 @@ class TestMultiprocess final : public Multiprocess { void MultiprocessParent() override { FileHandle read_handle = ReadPipeHandle(); char c; - CheckedReadFile(read_handle, &c, 1); + CheckedReadFileExactly(read_handle, &c, 1); EXPECT_EQ('M', c); pid_t pid; - CheckedReadFile(read_handle, &pid, sizeof(pid)); + CheckedReadFileExactly(read_handle, &pid, sizeof(pid)); EXPECT_EQ(pid, ChildPID()); c = 'm'; @@ -63,7 +63,7 @@ class TestMultiprocess final : public Multiprocess { pid_t pid = getpid(); CheckedWriteFile(write_handle, &pid, sizeof(pid)); - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('m', c); } diff --git a/test/win/win_child_process.cc b/test/win/win_child_process.cc index 156aa6dc..06ab21b2 100644 --- a/test/win/win_child_process.cc +++ b/test/win/win_child_process.cc @@ -203,7 +203,7 @@ std::unique_ptr WinChildProcess::Launch() { // immediately, and test code expects process initialization to have // completed so it can, for example, read the process memory. char c; - if (!LoggingReadFile(handles_for_parent->read.get(), &c, sizeof(c))) { + if (!LoggingReadFileExactly(handles_for_parent->read.get(), &c, sizeof(c))) { ADD_FAILURE() << "LoggedReadFile"; return std::unique_ptr(); } diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index e7d89bc4..4a2dcc65 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -597,7 +597,7 @@ int DatabaseUtilMain(int argc, char* argv[]) { !LoggingWriteFile(new_report->handle, buf, read_result)) { return EXIT_FAILURE; } - } while (read_result == sizeof(buf)); + } while (read_result > 0); call_error_writing_crash_report.Disarm(); diff --git a/util/file/file_io.cc b/util/file/file_io.cc index 98eb3404..8d3e466c 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -19,38 +19,71 @@ namespace crashpad { -bool LoggingReadFile(FileHandle file, void* buffer, size_t size) { +namespace { + +bool ReadFileExactlyInternal(FileHandle file, + void* buffer, + size_t size, + bool can_log) { FileOperationResult expect = base::checked_cast(size); - FileOperationResult rv = ReadFile(file, buffer, size); - if (rv < 0) { - PLOG(ERROR) << "read"; - return false; + char* buffer_c = static_cast(buffer); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = ReadFile(file, buffer, size); + if (bytes < 0) { + PLOG_IF(ERROR, can_log) << kNativeReadFunctionName; + return false; + } + + DCHECK_LE(static_cast(bytes), size); + + if (bytes == 0) { + break; + } + + buffer_c += bytes; + size -= bytes; + total_bytes += bytes; } - if (rv != expect) { - LOG(ERROR) << "read: expected " << expect << ", observed " << rv; + + if (total_bytes != expect) { + LOG_IF(ERROR, can_log) << kNativeReadFunctionName << ": expected " << expect + << ", observed " << total_bytes; return false; } return true; } +} // namespace + +bool ReadFileExactly(FileHandle file, void* buffer, size_t size) { + return ReadFileExactlyInternal(file, buffer, size, false); +} + +bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size) { + return ReadFileExactlyInternal(file, buffer, size, true); +} + bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size) { FileOperationResult expect = base::checked_cast(size); FileOperationResult rv = WriteFile(file, buffer, size); if (rv < 0) { - PLOG(ERROR) << "write"; + PLOG(ERROR) << kNativeWriteFunctionName; return false; } if (rv != expect) { - LOG(ERROR) << "write: expected " << expect << ", observed " << rv; + LOG(ERROR) << kNativeWriteFunctionName << ": expected " << expect + << ", observed " << rv; return false; } return true; } -void CheckedReadFile(FileHandle file, void* buffer, size_t size) { - CHECK(LoggingReadFile(file, buffer, size)); +void CheckedReadFileExactly(FileHandle file, void* buffer, size_t size) { + CHECK(LoggingReadFileExactly(file, buffer, size)); } void CheckedWriteFile(FileHandle file, const void* buffer, size_t size) { @@ -61,9 +94,9 @@ void CheckedReadFileAtEOF(FileHandle file) { char c; FileOperationResult rv = ReadFile(file, &c, 1); if (rv < 0) { - PCHECK(rv == 0) << "read"; + PCHECK(rv == 0) << kNativeReadFunctionName; } else { - CHECK_EQ(rv, 0) << "read"; + CHECK_EQ(rv, 0) << kNativeReadFunctionName; } } diff --git a/util/file/file_io.h b/util/file/file_io.h index ff20488d..a7415b8c 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -114,20 +114,33 @@ enum class FileLocking : bool { kExclusive, }; -//! \brief Reads from a file, retrying when interrupted on POSIX or following a -//! short read. +//! \brief The name of the native read function used by ReadFile(). //! -//! This function reads into \a buffer, stopping only when \a size bytes have -//! been read or when end-of-file has been reached. On Windows, reading from -//! sockets is not currently supported. +//! This value may be useful for logging. +//! +//! \sa kNativeWriteFunctionName +extern const char kNativeReadFunctionName[]; + +//! \brief The name of the native write function used by WriteFile(). +//! +//! This value may be useful for logging. +//! +//! \sa kNativeReadFunctionName +extern const char kNativeWriteFunctionName[]; + +//! \brief Reads from a file, retrying when interrupted on POSIX. +//! +//! This function reads into \a buffer. Fewer than \a size bytes may be read. +//! On Windows, reading from sockets is not currently supported. //! //! \return The number of bytes read and placed into \a buffer, or `-1` on //! error, with `errno` or `GetLastError()` set appropriately. On error, a //! portion of \a file may have been read into \a buffer. //! //! \sa WriteFile -//! \sa LoggingReadFile -//! \sa CheckedReadFile +//! \sa ReadFileExactly +//! \sa LoggingReadFileExactly +//! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); @@ -146,27 +159,50 @@ FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); //! \sa CheckedWriteFile FileOperationResult WriteFile(FileHandle file, const void* buffer, size_t size); -//! \brief Wraps ReadFile(), ensuring that exactly \a size bytes are read. +//! \brief Wraps ReadFile(), retrying following a short read, ensuring that +//! exactly \a size bytes are read. //! -//! \return `true` on success. If \a size is out of the range of possible -//! ReadFile() return values, if the underlying ReadFile() fails, or if -//! other than \a size bytes were read, this function logs a message and +//! If \a size is out of the range of possible ReadFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying ReadFile() fails, or if fewer +//! than \a size bytes were read, this function logs a message and //! returns `false`. //! //! \sa LoggingWriteFile //! \sa ReadFile -//! \sa CheckedReadFile +//! \sa LoggingReadFileExactly +//! \sa CheckedReadFileExactly //! \sa CheckedReadFileAtEOF -bool LoggingReadFile(FileHandle file, void* buffer, size_t size); +bool ReadFileExactly(FileHandle file, void* buffer, size_t size); + +//! \brief Wraps ReadFile(), retrying following a short read, ensuring that +//! exactly \a size bytes are read. +//! +//! If \a size is out of the range of possible ReadFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying ReadFile() fails, or if fewer +//! than \a size bytes were read, this function logs a message and +//! returns `false`. +//! +//! \sa LoggingWriteFile +//! \sa ReadFile +//! \sa ReadFileExactly +//! \sa CheckedReadFileExactly +//! \sa CheckedReadFileAtEOF +bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! -//! \return `true` on success. If \a size is out of the range of possible -//! WriteFile() return values, if the underlying WriteFile() fails, or if -//! other than \a size bytes were written, this function logs a message and +//! If \a size is out of the range of possible WriteFile() return values, this +//! function causes execution to terminate without returning. +//! +//! \return `true` on success. If the underlying WriteFile() fails, or if fewer +//! than \a size bytes were written, this function logs a message and //! returns `false`. //! -//! \sa LoggingReadFile +//! \sa LoggingReadFileExactly //! \sa WriteFile //! \sa CheckedWriteFile bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size); @@ -174,22 +210,22 @@ bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size); //! \brief Wraps ReadFile(), ensuring that exactly \a size bytes are read. //! //! If \a size is out of the range of possible ReadFile() return values, if the -//! underlying ReadFile() fails, or if other than \a size bytes were read, this +//! underlying ReadFile() fails, or if fewer than \a size bytes were read, this //! function causes execution to terminate without returning. //! //! \sa CheckedWriteFile //! \sa ReadFile -//! \sa LoggingReadFile +//! \sa LoggingReadFileExactly //! \sa CheckedReadFileAtEOF -void CheckedReadFile(FileHandle file, void* buffer, size_t size); +void CheckedReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! //! If \a size is out of the range of possible WriteFile() return values, if the -//! underlying WriteFile() fails, or if other than \a size bytes were written, +//! underlying WriteFile() fails, or if fewer than \a size bytes were written, //! this function causes execution to terminate without returning. //! -//! \sa CheckedReadFile +//! \sa CheckedReadFileExactly //! \sa WriteFile //! \sa LoggingWriteFile void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); @@ -200,7 +236,7 @@ void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); //! If the underlying ReadFile() fails, or if a byte actually is read, this //! function causes execution to terminate without returning. //! -//! \sa CheckedReadFile +//! \sa CheckedReadFileExactly //! \sa ReadFile void CheckedReadFileAtEOF(FileHandle file); diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 1534db15..00d14cd2 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -24,54 +24,6 @@ #include "base/numerics/safe_conversions.h" #include "base/posix/eintr_wrapper.h" -namespace { - -struct ReadTraits { - using VoidBufferType = void*; - using CharBufferType = char*; - static crashpad::FileOperationResult Operate(int fd, - CharBufferType buffer, - size_t size) { - return read(fd, buffer, size); - } -}; - -struct WriteTraits { - using VoidBufferType = const void*; - using CharBufferType = const char*; - static crashpad::FileOperationResult Operate(int fd, - CharBufferType buffer, - size_t size) { - return write(fd, buffer, size); - } -}; - -template -crashpad::FileOperationResult -ReadOrWrite(int fd, typename Traits::VoidBufferType buffer, size_t size) { - typename Traits::CharBufferType buffer_c = - reinterpret_cast(buffer); - - crashpad::FileOperationResult total_bytes = 0; - while (size > 0) { - crashpad::FileOperationResult bytes = - HANDLE_EINTR(Traits::Operate(fd, buffer_c, size)); - if (bytes < 0) { - return bytes; - } else if (bytes == 0) { - break; - } - - buffer_c += bytes; - size -= bytes; - total_bytes += bytes; - } - - return total_bytes; -} - -} // namespace - namespace crashpad { namespace { @@ -108,14 +60,44 @@ FileHandle OpenFileForOutput(int rdwr_or_wronly, } // namespace +const char kNativeReadFunctionName[] = "read"; +const char kNativeWriteFunctionName[] = "write"; + +// TODO(mark): Handle > ssize_t-sized reads and writes if necessary. The +// standard leaves this implementation-defined. Some systems return EINVAL in +// this case. ReadFile() and WriteFile() could enforce this behavior. + FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { - return ReadOrWrite(file, buffer, size); + FileOperationResult bytes = HANDLE_EINTR(read(file, buffer, size)); + if (bytes < 0) { + return -1; + } + + DCHECK_LE(static_cast(bytes), size); + return bytes; } FileOperationResult WriteFile(FileHandle file, const void* buffer, size_t size) { - return ReadOrWrite(file, buffer, size); + const char* buffer_c = static_cast(buffer); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = HANDLE_EINTR(write(file, buffer_c, size)); + if (bytes < 0) { + return -1; + } + + DCHECK_NE(bytes, 0); + DCHECK_LE(static_cast(bytes), size); + + buffer_c += bytes; + size -= bytes; + total_bytes += bytes; + } + + return total_bytes; } FileHandle OpenFileForRead(const base::FilePath& path) { diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 5144869e..110a5c6a 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -70,37 +70,37 @@ FileHandle OpenFileForOutput(DWORD access, } // namespace -// TODO(scottmg): Handle > DWORD sized writes if necessary. +const char kNativeReadFunctionName[] = "ReadFile"; +const char kNativeWriteFunctionName[] = "WriteFile"; + +// TODO(scottmg): Handle > DWORD-sized reads and writes if necessary. FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { DCHECK(!IsSocketHandle(file)); - DWORD size_dword = base::checked_cast(size); - DWORD total_read = 0; - char* buffer_c = reinterpret_cast(buffer); - while (size_dword > 0) { + + while (true) { + DWORD size_dword = base::checked_cast(size); DWORD bytes_read; - BOOL success = ::ReadFile(file, buffer_c, size_dword, &bytes_read, nullptr); + BOOL success = ::ReadFile(file, buffer, size_dword, &bytes_read, nullptr); if (!success) { if (GetLastError() == ERROR_BROKEN_PIPE) { // When reading a pipe and the write handle has been closed, ReadFile // fails with ERROR_BROKEN_PIPE, but only once all pending data has been - // read. - break; - } else if (GetLastError() != ERROR_MORE_DATA) { - return -1; + // read. Treat this as EOF. + return 0; } - } else if (bytes_read == 0 && GetFileType(file) != FILE_TYPE_PIPE) { + return -1; + } + + CHECK_NE(bytes_read, static_cast(-1)); + DCHECK_LE(bytes_read, size_dword); + if (bytes_read != 0 || GetFileType(file) != FILE_TYPE_PIPE) { // Zero bytes read for a file indicates reaching EOF. Zero bytes read from // a pipe indicates only that there was a zero byte WriteFile issued on // the other end, so continue reading. - break; + return bytes_read; } - - buffer_c += bytes_read; - size_dword -= bytes_read; - total_read += bytes_read; } - return total_read; } FileOperationResult WriteFile(FileHandle file, @@ -113,6 +113,7 @@ FileOperationResult WriteFile(FileHandle file, BOOL rv = ::WriteFile(file, buffer, size_dword, &bytes_written, nullptr); if (!rv) return -1; + CHECK_NE(bytes_written, static_cast(-1)); CHECK_EQ(bytes_written, size_dword); return bytes_written; } diff --git a/util/file/file_reader.cc b/util/file/file_reader.cc index 4f5ae773..ac625b87 100644 --- a/util/file/file_reader.cc +++ b/util/file/file_reader.cc @@ -22,12 +22,30 @@ namespace crashpad { bool FileReaderInterface::ReadExactly(void* data, size_t size) { FileOperationResult expect = base::checked_cast(size); - FileOperationResult rv = Read(data, size); - if (rv < 0) { - // Read() will have logged its own error. - return false; - } else if (rv != expect) { - LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " << rv; + char* data_c = static_cast(data); + + FileOperationResult total_bytes = 0; + while (size > 0) { + FileOperationResult bytes = Read(data, size); + if (bytes < 0) { + // Read() will have logged its own error. + return false; + } + + DCHECK_LE(static_cast(bytes), size); + + if (bytes == 0) { + break; + } + + data_c += bytes; + size -= bytes; + total_bytes += bytes; + } + + if (total_bytes != expect) { + LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " + << total_bytes; return false; } @@ -44,13 +62,10 @@ WeakFileHandleFileReader::~WeakFileHandleFileReader() { FileOperationResult WeakFileHandleFileReader::Read(void* data, size_t size) { DCHECK_NE(file_handle_, kInvalidFileHandle); - // Don’t use LoggingReadFile(), which insists on a full read and only returns - // a bool. This method permits short reads and returns the number of bytes - // read. base::checked_cast(size); FileOperationResult rv = ReadFile(file_handle_, data, size); if (rv < 0) { - PLOG(ERROR) << "read"; + PLOG(ERROR) << kNativeReadFunctionName; return -1; } diff --git a/util/file/file_reader.h b/util/file/file_reader.h index 39225e54..04724170 100644 --- a/util/file/file_reader.h +++ b/util/file/file_reader.h @@ -42,7 +42,7 @@ class FileReaderInterface : public virtual FileSeekerInterface { //! \brief Wraps Read(), ensuring that the read succeeded and exactly \a size //! bytes were read. //! - //! Semantically, this behaves as LoggingReadFile(). + //! Semantically, this behaves as LoggingReadFileExactly(). //! //! \return `true` if the operation succeeded, `false` if it failed, with an //! error message logged. Short reads are treated as failures. diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index 51aad905..e3624d70 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -403,20 +403,20 @@ bool ChildPortHandshake::RunClientInternal_ReadPipe(int client_read_fd, child_port_token_t* token, std::string* service_name) { // Read the token from the pipe. - if (!LoggingReadFile(client_read_fd, token, sizeof(*token))) { + if (!LoggingReadFileExactly(client_read_fd, token, sizeof(*token))) { return false; } // Read the service name from the pipe. uint32_t service_name_length; - if (!LoggingReadFile( - client_read_fd, &service_name_length, sizeof(service_name_length))) { + if (!LoggingReadFileExactly( + client_read_fd, &service_name_length, sizeof(service_name_length))) { return false; } service_name->resize(service_name_length); if (!service_name->empty() && - !LoggingReadFile( + !LoggingReadFileExactly( client_read_fd, &(*service_name)[0], service_name_length)) { return false; } diff --git a/util/mach/exception_ports_test.cc b/util/mach/exception_ports_test.cc index 37c7d118..f3d40029 100644 --- a/util/mach/exception_ports_test.cc +++ b/util/mach/exception_ports_test.cc @@ -244,7 +244,7 @@ class TestExceptionPorts : public MachMultiprocess, CheckedWriteFile(test_exception_ports_->WritePipeHandle(), &c, 1); // Wait for the parent process to say that its end is set up. - CheckedReadFile(test_exception_ports_->ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(test_exception_ports_->ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); // Regardless of where ExceptionPorts::SetExceptionPort() ran, @@ -352,7 +352,7 @@ class TestExceptionPorts : public MachMultiprocess, // Wait for the child process to be ready. It needs to have all of its // threads set up before proceeding if in kSetOutOfProcess mode. char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); mach_port_t local_port = LocalPort(); diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index a2d2eb1f..2c4c8e1d 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -344,7 +344,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.parent_wait_for_child_pipe) { // Wait until the child is done sending what it’s going to send. char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); } @@ -397,7 +397,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe_early) { // Wait until the parent is done setting things up on its end. char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); EXPECT_EQ('\0', c); } @@ -431,7 +431,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, if (options_.child_wait_for_parent_pipe_late) { char c; - CheckedReadFile(ReadPipeHandle(), &c, 1); + CheckedReadFileExactly(ReadPipeHandle(), &c, 1); ASSERT_EQ('\0', c); } } diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index 8dfa68a4..cb5c4f1f 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -81,7 +81,7 @@ class HTTPTransportTestFixture : public MultiprocessExec { // The child will write the HTTP server port number as a packed unsigned // short to stdout. uint16_t port; - ASSERT_TRUE(LoggingReadFile(ReadPipeHandle(), &port, sizeof(port))); + ASSERT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &port, sizeof(port))); // Then the parent will tell the web server what response code to send // for the HTTP request. diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 70955c82..fdc159c0 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -396,7 +396,8 @@ bool ExceptionHandlerServer::ServiceClientConnection( const internal::PipeServiceContext& service_context) { ClientToServerMessage message; - if (!LoggingReadFile(service_context.pipe(), &message, sizeof(message))) + if (!LoggingReadFileExactly( + service_context.pipe(), &message, sizeof(message))) return false; switch (message.type) { diff --git a/util/win/exception_handler_server_test.cc b/util/win/exception_handler_server_test.cc index e94b91d5..e9f865be 100644 --- a/util/win/exception_handler_server_test.cc +++ b/util/win/exception_handler_server_test.cc @@ -143,10 +143,11 @@ TEST_F(ExceptionHandlerServerTest, StopWhileConnected) { std::wstring ReadWString(FileHandle handle) { size_t length = 0; - EXPECT_TRUE(LoggingReadFile(handle, &length, sizeof(length))); + EXPECT_TRUE(LoggingReadFileExactly(handle, &length, sizeof(length))); std::wstring str(length, L'\0'); if (length > 0) { - EXPECT_TRUE(LoggingReadFile(handle, &str[0], length * sizeof(str[0]))); + EXPECT_TRUE( + LoggingReadFileExactly(handle, &str[0], length * sizeof(str[0]))); } return str; } diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index e377a6ee..dfa6564d 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -156,7 +156,7 @@ void TestOtherProcess(const base::string16& directory_modification) { // The child sends us a code address we can look up in the memory map. WinVMAddress code_address; - CheckedReadFile( + CheckedReadFileExactly( child.stdout_read_handle(), &code_address, sizeof(code_address)); ASSERT_TRUE(process_info.Initialize(child.process_handle())); diff --git a/util/win/scoped_process_suspend_test.cc b/util/win/scoped_process_suspend_test.cc index c968f710..adddfe27 100644 --- a/util/win/scoped_process_suspend_test.cc +++ b/util/win/scoped_process_suspend_test.cc @@ -80,7 +80,7 @@ class ScopedProcessSuspendTest final : public WinChildProcess { int Run() override { char c; // Wait for notification from parent. - EXPECT_TRUE(LoggingReadFile(ReadPipeHandle(), &c, sizeof(c))); + EXPECT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, sizeof(c))); EXPECT_EQ(' ', c); return EXIT_SUCCESS; } From 4f90f1514692a153cdd582fdf8cdfe787f3ce40f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 16 Mar 2017 16:15:54 -0400 Subject: [PATCH 13/19] Remove WeakStdioFileReader and WeakStdioFileWriter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tools/crashpad_database_util.cc | 21 +++++------ tools/crashpad_http_upload.cc | 3 +- util/file/file_io.h | 50 ++++++++++++++------------ util/file/file_io_posix.cc | 11 ++++++ util/file/file_io_test.cc | 22 ++++++++++++ util/file/file_io_win.cc | 22 ++++++++++++ util/file/file_reader.cc | 39 --------------------- util/file/file_reader.h | 35 ------------------- util/file/file_writer.cc | 62 --------------------------------- util/file/file_writer.h | 35 ------------------- 10 files changed, 96 insertions(+), 204 deletions(-) diff --git a/tools/crashpad_database_util.cc b/tools/crashpad_database_util.cc index 4a2dcc65..b2c4cb49 100644 --- a/tools/crashpad_database_util.cc +++ b/tools/crashpad_database_util.cc @@ -560,13 +560,21 @@ int DatabaseUtilMain(int argc, char* argv[]) { return EXIT_FAILURE; } + bool used_stdin = false; for (const base::FilePath new_report_path : options.new_report_paths) { std::unique_ptr file_reader; - bool is_stdin = false; if (new_report_path.value() == FILE_PATH_LITERAL("-")) { - is_stdin = true; - file_reader.reset(new WeakStdioFileReader(stdin)); + if (used_stdin) { + fprintf(stderr, + "%" PRFilePath + ": Only one --new-report may be read from standard input\n", + me.value().c_str()); + return EXIT_FAILURE; + } + used_stdin = true; + file_reader.reset(new WeakFileHandleFileReader( + StdioFileHandle(StdioStream::kStandardInput))); } else { std::unique_ptr file_path_reader(new FileReader()); if (!file_path_reader->Open(new_report_path)) { @@ -607,13 +615,6 @@ int DatabaseUtilMain(int argc, char* argv[]) { return EXIT_FAILURE; } - file_reader.reset(); - if (is_stdin) { - if (fclose(stdin) == EOF) { - STDIO_PLOG(ERROR) << "fclose"; - } - } - const char* prefix = (show_operations > 1) ? "New report ID: " : ""; printf("%s%s\n", prefix, uuid.ToString().c_str()); } diff --git a/tools/crashpad_http_upload.cc b/tools/crashpad_http_upload.cc index 55ab9550..d4ec3f9c 100644 --- a/tools/crashpad_http_upload.cc +++ b/tools/crashpad_http_upload.cc @@ -163,7 +163,8 @@ int HTTPUploadMain(int argc, char* argv[]) { return EXIT_FAILURE; } } else { - file_writer.reset(new WeakStdioFileWriter(stdout)); + file_writer.reset(new WeakFileHandleFileWriter( + StdioFileHandle(StdioStream::kStandardOutput))); } http_multipart_builder.SetGzipEnabled(options.upload_gzip); diff --git a/util/file/file_io.h b/util/file/file_io.h index a7415b8c..a720cdda 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -26,27 +26,6 @@ #include "util/win/scoped_handle.h" #endif -//! \file - -#if defined(OS_POSIX) || DOXYGEN - -//! \brief A `PLOG()` macro usable for standard input/output error conditions. -//! -//! The `PLOG()` macro uses `errno` on POSIX and is appropriate to report -//! errors from standard input/output functions. On Windows, `PLOG()` uses -//! `GetLastError()`, and cannot be used to report errors from standard -//! input/output functions. This macro uses `PLOG()` when appropriate for -//! standard I/O functions, and `LOG()` otherwise. -#define STDIO_PLOG(x) PLOG(x) - -#else - -#define STDIO_PLOG(x) LOG(x) -#define fseeko(file, offset, whence) _fseeki64(file, offset, whence) -#define ftello(file) _ftelli64(file) - -#endif - namespace base { class FilePath; } // namespace base @@ -114,6 +93,18 @@ enum class FileLocking : bool { kExclusive, }; +//! \brief Determines the FileHandle that StdioFileHandle() returns. +enum class StdioStream { + //! \brief Standard input, or `stdin`. + kStandardInput, + + //! \brief Standard output, or `stdout`. + kStandardOutput, + + //! \brief Standard error, or `stderr`. + kStandardError, +}; + //! \brief The name of the native read function used by ReadFile(). //! //! This value may be useful for logging. @@ -381,12 +372,27 @@ void CheckedCloseFile(FileHandle file); //! \brief Determines the size of a file. //! //! \param[in] file The handle to the file for which the size should be -//! retrived. +//! retrieved. //! //! \return The size of the file. If an error occurs when attempting to //! determine its size, returns `-1` with an error logged. FileOffset LoggingFileSizeByHandle(FileHandle file); +//! \brief Returns a FileHandle corresponding to the requested standard I/O +//! stream. +//! +//! The returned FileHandle should not be closed on POSIX, where it is +//! important to maintain valid file descriptors occupying the slots reserved +//! for these streams. If a need to close such a stream arises on POSIX, +//! `dup2()` should instead be used to replace the existing file descriptor with +//! one opened to `/dev/null`. See CloseStdinAndStdout(). +//! +//! \param[in] stdio_stream The requested standard I/O stream. +//! +//! \return A corresponding FileHandle on success. kInvalidFileHandle on error, +//! with a message logged. +FileHandle StdioFileHandle(StdioStream stdio_stream); + } // namespace crashpad #endif // CRASHPAD_UTIL_FILE_FILE_IO_H_ diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 00d14cd2..79543e3f 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -181,4 +181,15 @@ FileOffset LoggingFileSizeByHandle(FileHandle file) { return st.st_size; } +FileHandle StdioFileHandle(StdioStream stdio_stream) { + switch (stdio_stream) { + case StdioStream::kStandardInput: + return STDIN_FILENO; + case StdioStream::kStandardOutput: + return STDOUT_FILENO; + case StdioStream::kStandardError: + return STDERR_FILENO; + } +} + } // namespace crashpad diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index b8444d41..989d54d7 100644 --- a/util/file/file_io_test.cc +++ b/util/file/file_io_test.cc @@ -14,6 +14,8 @@ #include "util/file/file_io.h" +#include + #include "base/atomicops.h" #include "base/files/file_path.h" #include "base/macros.h" @@ -324,6 +326,26 @@ TEST(FileIO, FileSizeByHandle) { EXPECT_EQ(9, LoggingFileSizeByHandle(file_handle.get())); } +FileHandle FileHandleForFILE(FILE* file) { + int fd = fileno(file); +#if defined(OS_POSIX) + return fd; +#elif defined(OS_WIN) + return reinterpret_cast(_get_osfhandle(fd)); +#else +#error Port +#endif +} + +TEST(FileIO, StdioFileHandle) { + EXPECT_EQ(FileHandleForFILE(stdin), + StdioFileHandle(StdioStream::kStandardInput)); + EXPECT_EQ(FileHandleForFILE(stdout), + StdioFileHandle(StdioStream::kStandardOutput)); + EXPECT_EQ(FileHandleForFILE(stderr), + StdioFileHandle(StdioStream::kStandardError)); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 110a5c6a..189dc44d 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -246,4 +246,26 @@ FileOffset LoggingFileSizeByHandle(FileHandle file) { return file_size.QuadPart; } +FileHandle StdioFileHandle(StdioStream stdio_stream) { + DWORD standard_handle; + switch (stdio_stream) { + case StdioStream::kStandardInput: + standard_handle = STD_INPUT_HANDLE; + break; + case StdioStream::kStandardOutput: + standard_handle = STD_OUTPUT_HANDLE; + break; + case StdioStream::kStandardError: + standard_handle = STD_ERROR_HANDLE; + break; + default: + NOTREACHED(); + return INVALID_HANDLE_VALUE; + } + + HANDLE handle = GetStdHandle(standard_handle); + PLOG_IF(ERROR, handle == INVALID_HANDLE_VALUE) << "GetStdHandle"; + return handle; +} + } // namespace crashpad diff --git a/util/file/file_reader.cc b/util/file/file_reader.cc index ac625b87..b7fcffb5 100644 --- a/util/file/file_reader.cc +++ b/util/file/file_reader.cc @@ -113,43 +113,4 @@ FileOffset FileReader::Seek(FileOffset offset, int whence) { return weak_file_handle_file_reader_.Seek(offset, whence); } -WeakStdioFileReader::WeakStdioFileReader(FILE* file) - : file_(file) { -} - -WeakStdioFileReader::~WeakStdioFileReader() { -} - -FileOperationResult WeakStdioFileReader::Read(void* data, size_t size) { - DCHECK(file_); - - size_t rv = fread(data, 1, size, file_); - if (rv != size && ferror(file_)) { - STDIO_PLOG(ERROR) << "fread"; - return -1; - } - if (rv > size) { - LOG(ERROR) << "fread: expected " << size << ", observed " << rv; - return -1; - } - - return rv; -} - -FileOffset WeakStdioFileReader::Seek(FileOffset offset, int whence) { - DCHECK(file_); - if (fseeko(file_, offset, whence) == -1) { - STDIO_PLOG(ERROR) << "fseeko"; - return -1; - } - - FileOffset new_offset = ftello(file_); - if (new_offset == -1) { - STDIO_PLOG(ERROR) << "ftello"; - return -1; - } - - return new_offset; -} - } // namespace crashpad diff --git a/util/file/file_reader.h b/util/file/file_reader.h index 04724170..d44be1c1 100644 --- a/util/file/file_reader.h +++ b/util/file/file_reader.h @@ -15,7 +15,6 @@ #ifndef CRASHPAD_UTIL_FILE_FILE_READER_H_ #define CRASHPAD_UTIL_FILE_FILE_READER_H_ -#include #include #include "base/files/file_path.h" @@ -142,40 +141,6 @@ class FileReader : public FileReaderInterface { DISALLOW_COPY_AND_ASSIGN(FileReader); }; -//! \brief A file reader backed by a standard input/output `FILE*`. -//! -//! This class accepts an already-open `FILE*`. It is not responsible for -//! opening or closing this `FILE*`. Users of this class must ensure that the -//! `FILE*` is closed appropriately elsewhere. Objects of this class may be used -//! to read from `FILE*` objects not associated with filesystem-based files, -//! although special attention should be paid to the Seek() method, which may -//! not function on `FILE*` objects that do not refer to disk-based files. -//! -//! This class is expected to be used when other code is responsible for -//! opening `FILE*` objects and already provides `FILE*` objects. A good use -//! would be a WeakStdioFileReader for `stdin`. -class WeakStdioFileReader : public FileReaderInterface { - public: - explicit WeakStdioFileReader(FILE* file); - ~WeakStdioFileReader() override; - - // FileReaderInterface: - FileOperationResult Read(void* data, size_t size) override; - - // FileSeekerInterface: - - //! \copydoc FileReaderInterface::Seek() - //! - //! \note This method is only guaranteed to function on `FILE*` objects - //! referring to disk-based files. - FileOffset Seek(FileOffset offset, int whence) override; - - private: - FILE* file_; // weak - - DISALLOW_COPY_AND_ASSIGN(WeakStdioFileReader); -}; - } // namespace crashpad #endif // CRASHPAD_UTIL_FILE_FILE_READER_H_ diff --git a/util/file/file_writer.cc b/util/file/file_writer.cc index d22a1a3b..a8d92566 100644 --- a/util/file/file_writer.cc +++ b/util/file/file_writer.cc @@ -192,66 +192,4 @@ FileOffset FileWriter::Seek(FileOffset offset, int whence) { return weak_file_handle_file_writer_.Seek(offset, whence); } -WeakStdioFileWriter::WeakStdioFileWriter(FILE* file) - : file_(file) { -} - -WeakStdioFileWriter::~WeakStdioFileWriter() { -} - -bool WeakStdioFileWriter::Write(const void* data, size_t size) { - DCHECK(file_); - - size_t rv = fwrite(data, 1, size, file_); - if (rv != size) { - if (ferror(file_)) { - STDIO_PLOG(ERROR) << "fwrite"; - } else { - LOG(ERROR) << "fwrite: expected " << size << ", observed " << rv; - } - return false; - } - - return true; -} - -bool WeakStdioFileWriter::WriteIoVec(std::vector* iovecs) { - DCHECK(file_); - - if (iovecs->empty()) { - LOG(ERROR) << "WriteIoVec(): no iovecs"; - return false; - } - - for (const WritableIoVec& iov : *iovecs) { - if (!Write(iov.iov_base, iov.iov_len)) { - return false; - } - } - -#ifndef NDEBUG - // The interface says that |iovecs| is not sacred, so scramble it to make sure - // that nobody depends on it. - memset(&(*iovecs)[0], 0xa5, sizeof((*iovecs)[0]) * iovecs->size()); -#endif - - return true; -} - -FileOffset WeakStdioFileWriter::Seek(FileOffset offset, int whence) { - DCHECK(file_); - if (fseeko(file_, offset, whence) == -1) { - STDIO_PLOG(ERROR) << "fseeko"; - return -1; - } - - FileOffset new_offset = ftello(file_); - if (new_offset == -1) { - STDIO_PLOG(ERROR) << "ftello"; - return -1; - } - - return new_offset; -} - } // namespace crashpad diff --git a/util/file/file_writer.h b/util/file/file_writer.h index 007e5f3a..ed261ec5 100644 --- a/util/file/file_writer.h +++ b/util/file/file_writer.h @@ -167,41 +167,6 @@ class FileWriter : public FileWriterInterface { DISALLOW_COPY_AND_ASSIGN(FileWriter); }; -//! \brief A file writer backed by a standard input/output `FILE*`. -//! -//! This class accepts an already-open `FILE*`. It is not responsible for -//! opening or closing this `FILE*`. Users of this class must ensure that the -//! `FILE*` is closed appropriately elsewhere. Objects of this class may be used -//! to write to `FILE*` objects not associated with filesystem-based files, -//! although special attention should be paid to the Seek() method, which may -//! not function on `FILE*` objects that do not refer to disk-based files. -//! -//! This class is expected to be used when other code is responsible for -//! opening `FILE*` objects and already provides `FILE*` objects. A good use -//! would be a WeakStdioFileWriter for `stdout`. -class WeakStdioFileWriter : public FileWriterInterface { - public: - explicit WeakStdioFileWriter(FILE* file); - ~WeakStdioFileWriter() override; - - // FileWriterInterface: - bool Write(const void* data, size_t size) override; - bool WriteIoVec(std::vector* iovecs) override; - - // FileSeekerInterface: - - //! \copydoc FileWriterInterface::Seek() - //! - //! \note This method is only guaranteed to function on `FILE*` objects - //! referring to disk-based files. - FileOffset Seek(FileOffset offset, int whence) override; - - private: - FILE* file_; // weak - - DISALLOW_COPY_AND_ASSIGN(WeakStdioFileWriter); -}; - } // namespace crashpad #endif // CRASHPAD_UTIL_FILE_FILE_WRITER_H_ From 88bc09fb86151a7e49717bd039d1594147159e41 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Sat, 18 Mar 2017 09:46:46 -0400 Subject: [PATCH 14/19] posix: Fix StdioFileHandle() for GCC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- util/file/file_io_posix.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 79543e3f..1b36dcea 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -190,6 +190,9 @@ FileHandle StdioFileHandle(StdioStream stdio_stream) { case StdioStream::kStandardError: return STDERR_FILENO; } + + NOTREACHED(); + return kInvalidFileHandle; } } // namespace crashpad From 14138936b5c4bdef716765d04fd926de73d9d0f7 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Sat, 18 Mar 2017 00:18:20 -0400 Subject: [PATCH 15/19] =?UTF-8?q?test:=20Compare=20ProcessInfo::Arguments(?= =?UTF-8?q?)=20to=20main()=E2=80=99s=20argc/argv=20on=20POSIX?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Joshua Peraza --- client/client_test.gyp | 2 +- minidump/minidump_test.gyp | 2 +- snapshot/snapshot_test.gyp | 2 +- test/gmock_main.cc | 23 ++++++++++ test/gtest_main.cc | 22 ++++++++++ test/main_arguments.cc | 38 +++++++++++++++++ test/main_arguments.h | 49 +++++++++++++++++++++ test/main_arguments_test.cc | 34 +++++++++++++++ test/test.gyp | 30 +++++++++++++ test/test_test.gyp | 3 +- util/posix/process_info_test.cc | 75 ++++++++++----------------------- util/util_test.gyp | 2 +- 12 files changed, 225 insertions(+), 57 deletions(-) create mode 100644 test/gmock_main.cc create mode 100644 test/gtest_main.cc create mode 100644 test/main_arguments.cc create mode 100644 test/main_arguments.h create mode 100644 test/main_arguments_test.cc diff --git a/client/client_test.gyp b/client/client_test.gyp index 66ec59ea..dec8f603 100644 --- a/client/client_test.gyp +++ b/client/client_test.gyp @@ -23,9 +23,9 @@ 'dependencies': [ 'client.gyp:crashpad_client', '../handler/handler.gyp:crashpad_handler', + '../test/test.gyp:crashpad_gmock_main', '../test/test.gyp:crashpad_test', '../third_party/gtest/gmock.gyp:gmock', - '../third_party/gtest/gmock.gyp:gmock_main', '../third_party/gtest/gtest.gyp:gtest', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', diff --git a/minidump/minidump_test.gyp b/minidump/minidump_test.gyp index cdae4caa..acf4d635 100644 --- a/minidump/minidump_test.gyp +++ b/minidump/minidump_test.gyp @@ -23,9 +23,9 @@ 'dependencies': [ 'minidump.gyp:crashpad_minidump', '../snapshot/snapshot_test.gyp:crashpad_snapshot_test_lib', + '../test/test.gyp:crashpad_gtest_main', '../test/test.gyp:crashpad_test', '../third_party/gtest/gtest.gyp:gtest', - '../third_party/gtest/gtest.gyp:gtest_main', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', ], diff --git a/snapshot/snapshot_test.gyp b/snapshot/snapshot_test.gyp index 4ca9f4d5..b31b0601 100644 --- a/snapshot/snapshot_test.gyp +++ b/snapshot/snapshot_test.gyp @@ -57,9 +57,9 @@ 'snapshot.gyp:crashpad_snapshot_api', '../client/client.gyp:crashpad_client', '../compat/compat.gyp:crashpad_compat', + '../test/test.gyp:crashpad_gtest_main', '../test/test.gyp:crashpad_test', '../third_party/gtest/gtest.gyp:gtest', - '../third_party/gtest/gtest.gyp:gtest_main', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', ], diff --git a/test/gmock_main.cc b/test/gmock_main.cc new file mode 100644 index 00000000..05492817 --- /dev/null +++ b/test/gmock_main.cc @@ -0,0 +1,23 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "test/main_arguments.h" + +int main(int argc, char* argv[]) { + crashpad::test::InitializeMainArguments(argc, argv); + testing::InitGoogleMock(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/gtest_main.cc b/test/gtest_main.cc new file mode 100644 index 00000000..5608d2c2 --- /dev/null +++ b/test/gtest_main.cc @@ -0,0 +1,22 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "gtest/gtest.h" +#include "test/main_arguments.h" + +int main(int argc, char* argv[]) { + crashpad::test::InitializeMainArguments(argc, argv); + testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/test/main_arguments.cc b/test/main_arguments.cc new file mode 100644 index 00000000..c28d2fa3 --- /dev/null +++ b/test/main_arguments.cc @@ -0,0 +1,38 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "test/main_arguments.h" + +#include "base/logging.h" + +namespace crashpad { +namespace test { + +const std::vector* g_arguments; + +void InitializeMainArguments(int argc, char* argv[]) { + CHECK(!g_arguments); + CHECK(argc); + CHECK(argv); + + g_arguments = new const std::vector(argv, argv + argc); +} + +const std::vector& GetMainArguments() { + CHECK(g_arguments); + return *g_arguments; +} + +} // namespace test +} // namespace crashpad diff --git a/test/main_arguments.h b/test/main_arguments.h new file mode 100644 index 00000000..d6c4d5c8 --- /dev/null +++ b/test/main_arguments.h @@ -0,0 +1,49 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_TEST_MAIN_ARGUMENTS_H_ +#define CRASHPAD_TEST_MAIN_ARGUMENTS_H_ + +#include +#include + +namespace crashpad { +namespace test { + +//! \brief Saves the arguments to `main()` for later use. +//! +//! Call this function from a test program’s `main()` function so that tests +//! that require access to these variables can retrieve them from +//! GetMainArguments(). +//! +//! The contents of \a argv, limited to \a argc elements, will be copied, so +//! that subsequent modifications to these variables by `main()` will not affect +//! the state returned by GetMainArguments(). +//! +//! This function must be called exactly once during the lifetime of a test +//! program. +void InitializeMainArguments(int argc, char* argv[]); + +//! \brief Retrieves pointers to the arguments to `main()`. +//! +//! Tests that need to access the original values of a test program’s `main()` +//! function’s parameters at process creation can use this function to retrieve +//! them, provided that `main()` called InitializeMainArguments() before making +//! any changes to its arguments. +const std::vector& GetMainArguments(); + +} // namespace test +} // namespace crashpad + +#endif // CRASHPAD_TEST_MAIN_ARGUMENTS_H_ diff --git a/test/main_arguments_test.cc b/test/main_arguments_test.cc new file mode 100644 index 00000000..85d15984 --- /dev/null +++ b/test/main_arguments_test.cc @@ -0,0 +1,34 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "test/main_arguments.h" + +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(MainArguments, GetMainArguments) { + // Make sure that InitializeMainArguments() has been called and that + // GetMainArguments() provides reasonable values. + const std::vector& arguments = GetMainArguments(); + + ASSERT_FALSE(arguments.empty()); + EXPECT_FALSE(arguments[0].empty()); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/test/test.gyp b/test/test.gyp index d84ff640..eb0a28b9 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -42,6 +42,8 @@ 'mac/mach_errors.h', 'mac/mach_multiprocess.cc', 'mac/mach_multiprocess.h', + 'main_arguments.cc', + 'main_arguments.h', 'multiprocess.h', 'multiprocess_exec.h', 'multiprocess_exec_posix.cc', @@ -63,6 +65,11 @@ 'win/win_multiprocess.cc', 'win/win_multiprocess.h', ], + 'direct_dependent_settings': { + 'include_dirs': [ + '..', + ], + }, 'conditions': [ ['OS=="mac"', { 'link_settings': { @@ -87,5 +94,28 @@ }], ], }, + { + 'target_name': 'crashpad_gtest_main', + 'type': 'static_library', + 'dependencies': [ + 'crashpad_test', + '../third_party/gtest/gtest.gyp:gtest', + ], + 'sources': [ + 'gtest_main.cc', + ], + }, + { + 'target_name': 'crashpad_gmock_main', + 'type': 'static_library', + 'dependencies': [ + 'crashpad_test', + '../third_party/gtest/gmock.gyp:gmock', + '../third_party/gtest/gtest.gyp:gtest', + ], + 'sources': [ + 'gmock_main.cc', + ], + }, ], } diff --git a/test/test_test.gyp b/test/test_test.gyp index 2c58c170..053d9c82 100644 --- a/test/test_test.gyp +++ b/test/test_test.gyp @@ -22,10 +22,10 @@ 'type': 'executable', 'dependencies': [ 'crashpad_test_test_multiprocess_exec_test_child', + 'test.gyp:crashpad_gmock_main', 'test.gyp:crashpad_test', '../compat/compat.gyp:crashpad_compat', '../third_party/gtest/gmock.gyp:gmock', - '../third_party/gtest/gmock.gyp:gmock_main', '../third_party/gtest/gtest.gyp:gtest', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', @@ -36,6 +36,7 @@ 'sources': [ 'hex_string_test.cc', 'mac/mach_multiprocess_test.cc', + 'main_arguments_test.cc', 'multiprocess_exec_test.cc', 'multiprocess_posix_test.cc', 'paths_test.cc', diff --git a/util/posix/process_info_test.cc b/util/posix/process_info_test.cc index 8d63b722..6a1a132b 100644 --- a/util/posix/process_info_test.cc +++ b/util/posix/process_info_test.cc @@ -15,28 +15,21 @@ #include "util/posix/process_info.h" #include -#include #include #include +#include #include #include #include -#include "base/files/scoped_file.h" +#include "base/strings/stringprintf.h" #include "build/build_config.h" #include "gtest/gtest.h" #include "test/errors.h" +#include "test/main_arguments.h" #include "util/misc/implicit_cast.h" -#if defined(OS_MACOSX) -#include -#endif - -#if defined(OS_LINUX) || defined(OS_ANDROID) -#include -#endif - namespace crashpad { namespace test { namespace { @@ -100,51 +93,29 @@ void TestProcessSelfOrClone(const ProcessInfo& process_info) { std::vector argv; ASSERT_TRUE(process_info.Arguments(&argv)); - // gtest argv processing scrambles argv, but it leaves argc and argv[0] - // intact, so test those. + const std::vector& expect_argv = GetMainArguments(); -#if defined(OS_MACOSX) - int expect_argc = *_NSGetArgc(); - char** expect_argv = *_NSGetArgv(); -#elif defined(OS_LINUX) || defined(OS_ANDROID) - std::vector expect_arg_vector; - { - base::ScopedFILE cmdline(fopen("/proc/self/cmdline", "re")); - ASSERT_NE(nullptr, cmdline.get()) << ErrnoMessage("fopen"); + // expect_argv always contains the initial view of the arguments at the time + // the program was invoked. argv may contain this view, or it may contain the + // current view of arguments after gtest argv processing. argv may be a subset + // of expect_argv. + // + // gtest argv processing always leaves argv[0] intact, so this can be checked + // directly. + ASSERT_FALSE(expect_argv.empty()); + ASSERT_FALSE(argv.empty()); + EXPECT_EQ(expect_argv[0], argv[0]); - int expect_arg_char; - std::string expect_arg_string; - while ((expect_arg_char = fgetc(cmdline.get())) != EOF) { - if (expect_arg_char != '\0') { - expect_arg_string.append(1, expect_arg_char); - } else { - expect_arg_vector.push_back(expect_arg_string); - expect_arg_string.clear(); - } - } - ASSERT_EQ(0, ferror(cmdline.get())) << ErrnoMessage("fgetc"); - ASSERT_TRUE(expect_arg_string.empty()); + EXPECT_LE(argv.size(), expect_argv.size()); + + // Everything else in argv should have a match in expect_argv too, but things + // may have moved around. + for (size_t arg_index = 1; arg_index < argv.size(); ++arg_index) { + const std::string& arg = argv[arg_index]; + SCOPED_TRACE( + base::StringPrintf("arg_index %zu, arg %s", arg_index, arg.c_str())); + EXPECT_NE(expect_argv.end(), std::find(argv.begin(), argv.end(), arg)); } - - std::vector expect_argv_storage; - for (const std::string& expect_arg_string : expect_arg_vector) { - expect_argv_storage.push_back(expect_arg_string.c_str()); - } - - int expect_argc = expect_argv_storage.size(); - const char* const* expect_argv = - !expect_argv_storage.empty() ? &expect_argv_storage[0] : nullptr; -#else -#error Obtain expect_argc and expect_argv correctly on your system. -#endif - - int argc = implicit_cast(argv.size()); - EXPECT_EQ(expect_argc, argc); - - ASSERT_GE(expect_argc, 1); - ASSERT_GE(argc, 1); - - EXPECT_EQ(std::string(expect_argv[0]), argv[0]); } void TestSelfProcess(const ProcessInfo& process_info) { diff --git a/util/util_test.gyp b/util/util_test.gyp index bf579810..e1d2c02d 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -24,9 +24,9 @@ 'util.gyp:crashpad_util', '../client/client.gyp:crashpad_client', '../compat/compat.gyp:crashpad_compat', + '../test/test.gyp:crashpad_gmock_main', '../test/test.gyp:crashpad_test', '../third_party/gtest/gmock.gyp:gmock', - '../third_party/gtest/gmock.gyp:gmock_main', '../third_party/gtest/gtest.gyp:gtest', '../third_party/mini_chromium/mini_chromium.gyp:base', '../third_party/zlib/zlib.gyp:zlib', From bc5b7b06db68b53953d7a5da90717efdd6b11947 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Sun, 19 Mar 2017 15:30:19 -0400 Subject: [PATCH 16/19] Fix racy WorkerThread test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- util/thread/worker_thread_test.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/util/thread/worker_thread_test.cc b/util/thread/worker_thread_test.cc index 0cbee187..044734c8 100644 --- a/util/thread/worker_thread_test.cc +++ b/util/thread/worker_thread_test.cc @@ -30,8 +30,11 @@ class WorkDelegate : public WorkerThread::Delegate { ~WorkDelegate() {} void DoWork(const WorkerThread* thread) override { - if (++work_count_ == waiting_for_count_) - semaphore_.Signal(); + if (work_count_ < waiting_for_count_) { + if (++work_count_ == waiting_for_count_) { + semaphore_.Signal(); + } + } } void SetDesiredWorkCount(int times) { @@ -59,6 +62,7 @@ TEST(WorkerThread, DoWork) { WorkerThread thread(0.05, &delegate); uint64_t start = ClockMonotonicNanoseconds(); + delegate.SetDesiredWorkCount(2); thread.Start(0); EXPECT_TRUE(thread.is_running()); @@ -103,12 +107,12 @@ TEST(WorkerThread, DoWorkNow) { WorkDelegate delegate; WorkerThread thread(100, &delegate); + uint64_t start = ClockMonotonicNanoseconds(); + delegate.SetDesiredWorkCount(1); thread.Start(0); EXPECT_TRUE(thread.is_running()); - uint64_t start = ClockMonotonicNanoseconds(); - delegate.WaitForWorkCount(); EXPECT_EQ(1, delegate.work_count()); From 542306626df8356fee5b4dafdf35798041362a37 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 20 Mar 2017 08:47:58 -0400 Subject: [PATCH 17/19] minidump: Make the MemoryListStream the caboose once again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- minidump/minidump_file_writer.cc | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/minidump/minidump_file_writer.cc b/minidump/minidump_file_writer.cc index 819e807f..ebff2bab 100644 --- a/minidump/minidump_file_writer.cc +++ b/minidump/minidump_file_writer.cc @@ -146,29 +146,33 @@ void MinidumpFileWriter::InitializeFromSnapshot( memory_list->AddFromSnapshot(exception_snapshot->ExtraMemory()); } - // The memory list stream should be added last. This keeps the “extra memory” - // at the end so that if the minidump file is truncated, other, more critical - // data is more likely to be preserved. Note that non-“extra” memory regions - // will not have to ride at the end of the file. Thread stack memory, for - // example, exists as a children of threads, and appears alongside them in the - // file. - // - // It would be nice if this followed the user streams, but that would be - // hazardous. See below. - add_stream_result = AddStream(std::move(memory_list)); - DCHECK(add_stream_result); - // These user streams must be added last. Otherwise, a user stream with the // same type as a well-known stream could preempt the well-known stream. As it // stands now, earlier-discovered user streams can still preempt - // later-discovered ones. + // later-discovered ones. The well-known memory list stream is added after + // these user streams, but only with a check here to avoid adding a user + // stream that would preempt the memory list stream. for (const auto& module : process_snapshot->Modules()) { for (const UserMinidumpStream* stream : module->CustomMinidumpStreams()) { + if (stream->stream_type() == kMinidumpStreamTypeMemoryList) { + LOG(WARNING) << "discarding duplicate stream of type " + << stream->stream_type(); + continue; + } auto user_stream = base::WrapUnique(new MinidumpUserStreamWriter()); user_stream->InitializeFromSnapshot(stream); AddStream(std::move(user_stream)); } } + + // The memory list stream should be added last. This keeps the “extra memory” + // at the end so that if the minidump file is truncated, other, more critical + // data is more likely to be preserved. Note that non-“extra” memory regions + // will not have to ride at the end of the file. Thread stack memory, for + // example, exists as a children of threads, and appears alongside them in the + // file, despite also being mentioned by the memory list stream. + add_stream_result = AddStream(std::move(memory_list)); + DCHECK(add_stream_result); } void MinidumpFileWriter::SetTimestamp(time_t timestamp) { From 39f13a77a44969f0e51208ecbc9246fcc93be583 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 21 Mar 2017 20:52:50 -0400 Subject: [PATCH 18/19] Make run_tests.py work with paths instead of configuration names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Scott Graham --- build/run_tests.py | 24 +++++------------------- doc/developing.md | 8 +++++--- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/build/run_tests.py b/build/run_tests.py index f6306852..00b40f6b 100755 --- a/build/run_tests.py +++ b/build/run_tests.py @@ -15,7 +15,6 @@ # limitations under the License. import os -import platform import subprocess import sys @@ -25,25 +24,12 @@ import sys # location in the recipe. def main(args): if len(args) != 1: - print >> sys.stderr, \ - 'usage: run_tests.py {Debug|Release|Debug_x64|Release_x64}' + print >> sys.stderr, 'usage: run_tests.py ' return 1 crashpad_dir = \ os.path.join(os.path.dirname(os.path.abspath(__file__)), os.pardir) - - # In a standalone Crashpad build, the out directory is in the Crashpad root. - out_dir = os.path.join(crashpad_dir, 'out') - if not os.path.exists(out_dir): - # In an in-Chromium build, the out directory is in the Chromium root, and - # the Crashpad root is in third_party/crashpad/crashpad relative to the - # Chromium root. - chromium_dir = os.path.join(crashpad_dir, os.pardir, os.pardir, os.pardir) - out_dir = os.path.join(chromium_dir, 'out') - if not os.path.exists(out_dir): - raise Exception('could not determine out_dir', crashpad_dir) - - binary_dir = os.path.join(out_dir, args[0]) + binary_dir = args[0] tests = [ 'crashpad_client_test', @@ -59,12 +45,12 @@ def main(args): subprocess.check_call(os.path.join(binary_dir, test)) if sys.platform == 'win32': - name = 'snapshot/win/end_to_end_test.py' + script = 'snapshot/win/end_to_end_test.py' print '-' * 80 - print name + print script print '-' * 80 subprocess.check_call( - [sys.executable, os.path.join(crashpad_dir, name), binary_dir]) + [sys.executable, os.path.join(crashpad_dir, script), binary_dir]) return 0 diff --git a/doc/developing.md b/doc/developing.md index e091b894..9beec52c 100644 --- a/doc/developing.md +++ b/doc/developing.md @@ -91,7 +91,9 @@ crashpad`, `gclient sync`, or `gclient runhooks`. The Ninja build files and build output are in the `out` directory. Both debug- and release-mode configurations are available. The examples below show the debug configuration. To build and test the release configuration, substitute `Release` -for `Debug`. +for `Debug`. On Windows, four configurations are available: `Debug` and +`Release` produce 32-bit x86 executables, and `Debug_x64` and `Release_x64` +produce x86_64 executables. ``` $ cd ~/crashpad/crashpad @@ -193,11 +195,11 @@ $ out/Debug/crashpad_util_test ``` A script is provided to run all of Crashpad’s tests. It accepts a single -argument that tells it which configuration to test. +argument, a path to the directory containing the test executables. ``` $ cd ~/crashpad/crashpad -$ python build/run_tests.py Debug +$ python build/run_tests.py out/Debug ``` ### Android From 3983b80ca2fc684a45903f4eb8e8089569ab0286 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 21 Mar 2017 15:08:05 -0400 Subject: [PATCH 19/19] util/file: Handle oversized reads and writes gracefully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- util/file/file_io.cc | 122 ++++++++---- util/file/file_io.h | 119 +++++++++--- util/file/file_io_posix.cc | 77 ++++---- util/file/file_io_test.cc | 340 ++++++++++++++++++++++++++++++++++ util/file/file_io_win.cc | 58 ++++-- util/file/file_reader.cc | 55 +++--- util/file/file_reader_test.cc | 205 ++++++++++++++++++++ util/util_test.gyp | 1 + 8 files changed, 839 insertions(+), 138 deletions(-) create mode 100644 util/file/file_reader_test.cc diff --git a/util/file/file_io.cc b/util/file/file_io.cc index 8d3e466c..6cae837a 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -15,67 +15,123 @@ #include "util/file/file_io.h" #include "base/logging.h" +#include "base/macros.h" #include "base/numerics/safe_conversions.h" namespace crashpad { namespace { -bool ReadFileExactlyInternal(FileHandle file, - void* buffer, - size_t size, - bool can_log) { - FileOperationResult expect = base::checked_cast(size); - char* buffer_c = static_cast(buffer); +class FileIOReadExactly final : public internal::ReadExactlyInternal { + public: + explicit FileIOReadExactly(FileHandle file) + : ReadExactlyInternal(), file_(file) {} + ~FileIOReadExactly() {} - FileOperationResult total_bytes = 0; - while (size > 0) { - FileOperationResult bytes = ReadFile(file, buffer, size); - if (bytes < 0) { - PLOG_IF(ERROR, can_log) << kNativeReadFunctionName; + private: + // ReadExactlyInternal: + FileOperationResult Read(void* buffer, size_t size, bool can_log) override { + FileOperationResult rv = ReadFile(file_, buffer, size); + if (rv < 0) { + PLOG_IF(ERROR, can_log) << internal::kNativeReadFunctionName; + return -1; + } + return rv; + } + + FileHandle file_; + + DISALLOW_COPY_AND_ASSIGN(FileIOReadExactly); +}; + +class FileIOWriteAll final : public internal::WriteAllInternal { + public: + explicit FileIOWriteAll(FileHandle file) : WriteAllInternal(), file_(file) {} + ~FileIOWriteAll() {} + + private: + // WriteAllInternal: + FileOperationResult Write(const void* buffer, size_t size) override { + return internal::NativeWriteFile(file_, buffer, size); + } + + FileHandle file_; + + DISALLOW_COPY_AND_ASSIGN(FileIOWriteAll); +}; + +} // namespace + +namespace internal { + +bool ReadExactlyInternal::ReadExactly(void* buffer, size_t size, bool can_log) { + char* buffer_c = static_cast(buffer); + size_t total_bytes = 0; + size_t remaining = size; + while (remaining > 0) { + FileOperationResult bytes_read = Read(buffer_c, remaining, can_log); + if (bytes_read < 0) { return false; } - DCHECK_LE(static_cast(bytes), size); + DCHECK_LE(static_cast(bytes_read), remaining); - if (bytes == 0) { + if (bytes_read == 0) { break; } - buffer_c += bytes; - size -= bytes; - total_bytes += bytes; + buffer_c += bytes_read; + remaining -= bytes_read; + total_bytes += bytes_read; } - if (total_bytes != expect) { - LOG_IF(ERROR, can_log) << kNativeReadFunctionName << ": expected " << expect - << ", observed " << total_bytes; + if (total_bytes != size) { + LOG_IF(ERROR, can_log) << "ReadExactly: expected " << size << ", observed " + << total_bytes; return false; } return true; } -} // namespace +bool WriteAllInternal::WriteAll(const void* buffer, size_t size) { + const char* buffer_c = static_cast(buffer); + + while (size > 0) { + FileOperationResult bytes_written = Write(buffer_c, size); + if (bytes_written < 0) { + return false; + } + + DCHECK_NE(bytes_written, 0); + + buffer_c += bytes_written; + size -= bytes_written; + } + + return true; +} + +} // namespace internal bool ReadFileExactly(FileHandle file, void* buffer, size_t size) { - return ReadFileExactlyInternal(file, buffer, size, false); + FileIOReadExactly read_exactly(file); + return read_exactly.ReadExactly(buffer, size, false); } bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size) { - return ReadFileExactlyInternal(file, buffer, size, true); + FileIOReadExactly read_exactly(file); + return read_exactly.ReadExactly(buffer, size, true); +} + +bool WriteFile(FileHandle file, const void* buffer, size_t size) { + FileIOWriteAll write_all(file); + return write_all.WriteAll(buffer, size); } bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size) { - FileOperationResult expect = base::checked_cast(size); - FileOperationResult rv = WriteFile(file, buffer, size); - if (rv < 0) { - PLOG(ERROR) << kNativeWriteFunctionName; - return false; - } - if (rv != expect) { - LOG(ERROR) << kNativeWriteFunctionName << ": expected " << expect - << ", observed " << rv; + if (!WriteFile(file, buffer, size)) { + PLOG(ERROR) << internal::kNativeWriteFunctionName; return false; } @@ -94,9 +150,9 @@ void CheckedReadFileAtEOF(FileHandle file) { char c; FileOperationResult rv = ReadFile(file, &c, 1); if (rv < 0) { - PCHECK(rv == 0) << kNativeReadFunctionName; + PCHECK(rv == 0) << internal::kNativeReadFunctionName; } else { - CHECK_EQ(rv, 0) << kNativeReadFunctionName; + CHECK_EQ(rv, 0) << internal::kNativeReadFunctionName; } } diff --git a/util/file/file_io.h b/util/file/file_io.h index a720cdda..87c40702 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -105,6 +105,8 @@ enum class StdioStream { kStandardError, }; +namespace internal { + //! \brief The name of the native read function used by ReadFile(). //! //! This value may be useful for logging. @@ -119,7 +121,91 @@ extern const char kNativeReadFunctionName[]; //! \sa kNativeReadFunctionName extern const char kNativeWriteFunctionName[]; -//! \brief Reads from a file, retrying when interrupted on POSIX. +//! \brief The internal implementation of ReadFileExactly() and its wrappers. +//! +//! The logic is exposed so that it may be reused by FileReaderInterface, and +//! so that it may be tested without requiring large files to be read. It is not +//! intended to be used more generally. Use ReadFileExactly(), +//! LoggingReadFileExactly(), CheckedReadFileExactly(), or +//! FileReaderInterface::ReadExactly() instead. +class ReadExactlyInternal { + public: + //! \brief Calls Read(), retrying following a short read, ensuring that + //! exactly \a size bytes are read. + //! + //! \return `true` on success. `false` if the underlying Read() fails or if + //! fewer than \a size bytes were read. When returning `false`, if \a + //! can_log is `true`, logs a message. + bool ReadExactly(void* buffer, size_t size, bool can_log); + + protected: + ReadExactlyInternal() {} + ~ReadExactlyInternal() {} + + private: + //! \brief Wraps a read operation, such as ReadFile(). + //! + //! \return The number of bytes read and placed into \a buffer, or `-1` on + //! error. When returning `-1`, if \a can_log is `true`, logs a message. + virtual FileOperationResult Read(void* buffer, size_t size, bool can_log) = 0; + + DISALLOW_COPY_AND_ASSIGN(ReadExactlyInternal); +}; + +//! \brief The internal implementation of WriteFile() and its wrappers. +//! +//! The logic is exposed so that it may be tested without requiring large files +//! to be written. It is not intended to be used more generally. Use +//! WriteFile(), LoggingWriteFile(), CheckedWriteFile(), or +//! FileWriterInterface::Write() instead. +class WriteAllInternal { + public: + //! \brief Calls Write(), retrying following a short write, ensuring that + //! exactly \a size bytes are written. + //! + //! \return `true` on success. `false` if the underlying Write() fails or if + //! fewer than \a size bytes were written. + bool WriteAll(const void* buffer, size_t size); + + protected: + WriteAllInternal() {} + ~WriteAllInternal() {} + + private: + //! \brief Wraps a write operation, such as NativeWriteFile(). + //! + //! \return The number of bytes written from \a buffer, or `-1` on error. + virtual FileOperationResult Write(const void* buffer, size_t size) = 0; + + DISALLOW_COPY_AND_ASSIGN(WriteAllInternal); +}; + +//! \brief Writes to a file, retrying when interrupted on POSIX. +//! +//! Fewer than \a size bytes may be written to \a file. This can happen if the +//! underlying write operation returns before writing the entire buffer, or if +//! the buffer is too large to write in a single operation, possibly due to a +//! limitation of a data type used to express the number of bytes written. +//! +//! This function adapts native write operations for uniform use by WriteFile(). +//! This function should only be called by WriteFile(). Other code should call +//! WriteFile() or another function that wraps WriteFile(). +//! +//! \param[in] file The file to write to. +//! \param[in] buffer A buffer containing data to be written. +//! \param[in] size The number of bytes from \a buffer to write. +//! +//! \return The number of bytes actually written from \a buffer to \a file on +//! success. `-1` on error, with `errno` or `GetLastError()` set +//! appropriately. +FileOperationResult NativeWriteFile(FileHandle file, + const void* buffer, + size_t size); + +} // namespace internal + +//! \brief Reads from a file, retrying when interrupted before reading any data +//! on POSIX. //! //! This function reads into \a buffer. Fewer than \a size bytes may be read. //! On Windows, reading from sockets is not currently supported. @@ -135,27 +221,24 @@ extern const char kNativeWriteFunctionName[]; //! \sa CheckedReadFileAtEOF FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size); -//! \brief Writes to a file, retrying when interrupted or following a short -//! write on POSIX. +//! \brief Writes to a file, retrying when interrupted on POSIX or following a +//! short write. //! //! This function writes to \a file, stopping only when \a size bytes have been //! written. //! -//! \return The number of bytes written from \a buffer, or `-1` on error, with -//! `errno` or `GetLastError()` set appropriately. On error, a portion of -//! \a buffer may have been written to \a file. +//! \return `true` on success. `false` on error, with `errno` or +//! `GetLastError()` set appropriately. On error, a portion of \a buffer may +//! have been written to \a file. //! //! \sa ReadFile //! \sa LoggingWriteFile //! \sa CheckedWriteFile -FileOperationResult WriteFile(FileHandle file, const void* buffer, size_t size); +bool WriteFile(FileHandle file, const void* buffer, size_t size); //! \brief Wraps ReadFile(), retrying following a short read, ensuring that //! exactly \a size bytes are read. //! -//! If \a size is out of the range of possible ReadFile() return values, this -//! function causes execution to terminate without returning. -//! //! \return `true` on success. If the underlying ReadFile() fails, or if fewer //! than \a size bytes were read, this function logs a message and //! returns `false`. @@ -170,9 +253,6 @@ bool ReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps ReadFile(), retrying following a short read, ensuring that //! exactly \a size bytes are read. //! -//! If \a size is out of the range of possible ReadFile() return values, this -//! function causes execution to terminate without returning. -//! //! \return `true` on success. If the underlying ReadFile() fails, or if fewer //! than \a size bytes were read, this function logs a message and //! returns `false`. @@ -186,9 +266,6 @@ bool LoggingReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! -//! If \a size is out of the range of possible WriteFile() return values, this -//! function causes execution to terminate without returning. -//! //! \return `true` on success. If the underlying WriteFile() fails, or if fewer //! than \a size bytes were written, this function logs a message and //! returns `false`. @@ -200,9 +277,8 @@ bool LoggingWriteFile(FileHandle file, const void* buffer, size_t size); //! \brief Wraps ReadFile(), ensuring that exactly \a size bytes are read. //! -//! If \a size is out of the range of possible ReadFile() return values, if the -//! underlying ReadFile() fails, or if fewer than \a size bytes were read, this -//! function causes execution to terminate without returning. +//! If the underlying ReadFile() fails, or if fewer than \a size bytes were +//! read, this function causes execution to terminate without returning. //! //! \sa CheckedWriteFile //! \sa ReadFile @@ -212,9 +288,8 @@ void CheckedReadFileExactly(FileHandle file, void* buffer, size_t size); //! \brief Wraps WriteFile(), ensuring that exactly \a size bytes are written. //! -//! If \a size is out of the range of possible WriteFile() return values, if the -//! underlying WriteFile() fails, or if fewer than \a size bytes were written, -//! this function causes execution to terminate without returning. +//! if the underlying WriteFile() fails, or if fewer than \a size bytes were +//! written, this function causes execution to terminate without returning. //! //! \sa CheckedReadFileExactly //! \sa WriteFile diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 1b36dcea..49441244 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -19,15 +19,49 @@ #include #include +#include +#include + #include "base/files/file_path.h" #include "base/logging.h" -#include "base/numerics/safe_conversions.h" #include "base/posix/eintr_wrapper.h" namespace crashpad { namespace { +struct ReadTraits { + using BufferType = void*; + static FileOperationResult Operate(int fd, BufferType buffer, size_t size) { + return read(fd, buffer, size); + } +}; + +struct WriteTraits { + using BufferType = const void*; + static FileOperationResult Operate(int fd, BufferType buffer, size_t size) { + return write(fd, buffer, size); + } +}; + +template +FileOperationResult ReadOrWrite(int fd, + typename Traits::BufferType buffer, + size_t size) { + constexpr size_t kMaxReadWriteSize = + static_cast(std::numeric_limits::max()); + const size_t requested_bytes = std::min(size, kMaxReadWriteSize); + + FileOperationResult transacted_bytes = + HANDLE_EINTR(Traits::Operate(fd, buffer, requested_bytes)); + if (transacted_bytes < 0) { + return -1; + } + + DCHECK_LE(static_cast(transacted_bytes), requested_bytes); + return transacted_bytes; +} + FileHandle OpenFileForOutput(int rdwr_or_wronly, const base::FilePath& path, FileWriteMode mode, @@ -60,44 +94,21 @@ FileHandle OpenFileForOutput(int rdwr_or_wronly, } // namespace +namespace internal { + const char kNativeReadFunctionName[] = "read"; const char kNativeWriteFunctionName[] = "write"; -// TODO(mark): Handle > ssize_t-sized reads and writes if necessary. The -// standard leaves this implementation-defined. Some systems return EINVAL in -// this case. ReadFile() and WriteFile() could enforce this behavior. - -FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { - FileOperationResult bytes = HANDLE_EINTR(read(file, buffer, size)); - if (bytes < 0) { - return -1; - } - - DCHECK_LE(static_cast(bytes), size); - return bytes; +FileOperationResult NativeWriteFile(FileHandle file, + const void* buffer, + size_t size) { + return ReadOrWrite(file, buffer, size); } -FileOperationResult WriteFile(FileHandle file, - const void* buffer, - size_t size) { - const char* buffer_c = static_cast(buffer); +} // namespace internal - FileOperationResult total_bytes = 0; - while (size > 0) { - FileOperationResult bytes = HANDLE_EINTR(write(file, buffer_c, size)); - if (bytes < 0) { - return -1; - } - - DCHECK_NE(bytes, 0); - DCHECK_LE(static_cast(bytes), size); - - buffer_c += bytes; - size -= bytes; - total_bytes += bytes; - } - - return total_bytes; +FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { + return ReadOrWrite(file, buffer, size); } FileHandle OpenFileForRead(const base::FilePath& path) { diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index 989d54d7..c4e60585 100644 --- a/util/file/file_io_test.cc +++ b/util/file/file_io_test.cc @@ -16,9 +16,13 @@ #include +#include +#include + #include "base/atomicops.h" #include "base/files/file_path.h" #include "base/macros.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "test/errors.h" #include "test/file.h" @@ -30,6 +34,342 @@ namespace crashpad { namespace test { namespace { +using testing::_; +using testing::InSequence; +using testing::Return; + +class MockReadExactly : public internal::ReadExactlyInternal { + public: + MockReadExactly() : ReadExactlyInternal() {} + ~MockReadExactly() {} + + // Since it’s more convenient for the test to use uintptr_t than void*, + // ReadExactlyInt() and ReadInt() adapt the types. + + bool ReadExactlyInt(uintptr_t data, size_t size, bool can_log) { + return ReadExactly(reinterpret_cast(data), size, can_log); + } + + MOCK_METHOD3(ReadInt, FileOperationResult(uintptr_t, size_t, bool)); + + // ReadExactlyInternal: + FileOperationResult Read(void* data, size_t size, bool can_log) { + return ReadInt(reinterpret_cast(data), size, can_log); + } + + private: + DISALLOW_COPY_AND_ASSIGN(MockReadExactly); +}; + +TEST(FileIO, ReadExactly_Zero) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(_, _, false)).Times(0); + EXPECT_TRUE(read_exactly.ReadExactlyInt(100, 0, false)); +} + +TEST(FileIO, ReadExactly_SingleSmallSuccess) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(1000, 1, false)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(1000, 1, false)); +} + +TEST(FileIO, ReadExactly_SingleSmallSuccessCanLog) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(1000, 1, true)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(1000, 1, true)); +} + +TEST(FileIO, ReadExactly_SingleSmallFailure) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(1000, 1, false)).WillOnce(Return(-1)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(1000, 1, false)); +} + +TEST(FileIO, ReadExactly_SingleSmallFailureCanLog) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(1000, 1, true)).WillOnce(Return(-1)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(1000, 1, true)); +} + +TEST(FileIO, ReadExactly_DoubleSmallSuccess) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0x1000, 2, false)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(0x1001, 1, false)).WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(0x1000, 2, false)); +} + +TEST(FileIO, ReadExactly_DoubleSmallShort) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0x20000, 2, false)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(0x20001, 1, false)).WillOnce(Return(0)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(0x20000, 2, false)); +} + +TEST(FileIO, ReadExactly_DoubleSmallShortCanLog) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0x20000, 2, true)).WillOnce(Return(1)); + EXPECT_CALL(read_exactly, ReadInt(0x20001, 1, true)).WillOnce(Return(0)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(0x20000, 2, true)); +} + +TEST(FileIO, ReadExactly_Medium) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0x80000000, 0x20000000, false)) + .WillOnce(Return(0x10000000)); + EXPECT_CALL(read_exactly, ReadInt(0x90000000, 0x10000000, false)) + .WillOnce(Return(0x8000000)); + EXPECT_CALL(read_exactly, ReadInt(0x98000000, 0x8000000, false)) + .WillOnce(Return(0x4000000)); + EXPECT_CALL(read_exactly, ReadInt(0x9c000000, 0x4000000, false)) + .WillOnce(Return(0x2000000)); + EXPECT_CALL(read_exactly, ReadInt(0x9e000000, 0x2000000, false)) + .WillOnce(Return(0x1000000)); + EXPECT_CALL(read_exactly, ReadInt(0x9f000000, 0x1000000, false)) + .WillOnce(Return(0x800000)); + EXPECT_CALL(read_exactly, ReadInt(0x9f800000, 0x800000, false)) + .WillOnce(Return(0x400000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fc00000, 0x400000, false)) + .WillOnce(Return(0x200000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fe00000, 0x200000, false)) + .WillOnce(Return(0x100000)); + EXPECT_CALL(read_exactly, ReadInt(0x9ff00000, 0x100000, false)) + .WillOnce(Return(0x80000)); + EXPECT_CALL(read_exactly, ReadInt(0x9ff80000, 0x80000, false)) + .WillOnce(Return(0x40000)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffc0000, 0x40000, false)) + .WillOnce(Return(0x20000)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffe0000, 0x20000, false)) + .WillOnce(Return(0x10000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fff0000, 0x10000, false)) + .WillOnce(Return(0x8000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fff8000, 0x8000, false)) + .WillOnce(Return(0x4000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffc000, 0x4000, false)) + .WillOnce(Return(0x2000)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffe000, 0x2000, false)) + .WillOnce(Return(0x1000)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffff000, 0x1000, false)) + .WillOnce(Return(0x800)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffff800, 0x800, false)) + .WillOnce(Return(0x400)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffc00, 0x400, false)) + .WillOnce(Return(0x200)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffe00, 0x200, false)) + .WillOnce(Return(0x100)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffff00, 0x100, false)) + .WillOnce(Return(0x80)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffff80, 0x80, false)) + .WillOnce(Return(0x40)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffffc0, 0x40, false)) + .WillOnce(Return(0x20)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffffe0, 0x20, false)) + .WillOnce(Return(0x10)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffff0, 0x10, false)) + .WillOnce(Return(0x8)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffff8, 0x8, false)) + .WillOnce(Return(0x4)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffffc, 0x4, false)) + .WillOnce(Return(0x2)); + EXPECT_CALL(read_exactly, ReadInt(0x9ffffffe, 0x2, false)) + .WillOnce(Return(0x1)); + EXPECT_CALL(read_exactly, ReadInt(0x9fffffff, 0x1, false)) + .WillOnce(Return(0x1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(0x80000000, 0x20000000, false)); +} + +TEST(FileIO, ReadExactly_LargeSuccess) { + MockReadExactly read_exactly; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = std::numeric_limits::max(); + EXPECT_CALL(read_exactly, ReadInt(0, max, false)).WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(increment, max - increment, false)) + .WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(2 * increment, 1, false)) + .WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(0, max, false)); +} + +TEST(FileIO, ReadExactly_LargeShort) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0, 0xffffffff, false)) + .WillOnce(Return(0x7fffffff)); + EXPECT_CALL(read_exactly, ReadInt(0x7fffffff, 0x80000000, false)) + .WillOnce(Return(0x10000000)); + EXPECT_CALL(read_exactly, ReadInt(0x8fffffff, 0x70000000, false)) + .WillOnce(Return(0)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(0, 0xffffffff, false)); +} + +TEST(FileIO, ReadExactly_LargeFailure) { + MockReadExactly read_exactly; + InSequence in_sequence; + EXPECT_CALL(read_exactly, ReadInt(0, 0xffffffff, false)) + .WillOnce(Return(0x7fffffff)); + EXPECT_CALL(read_exactly, ReadInt(0x7fffffff, 0x80000000, false)) + .WillOnce(Return(-1)); + EXPECT_FALSE(read_exactly.ReadExactlyInt(0, 0xffffffff, false)); +} + +TEST(FileIO, ReadExactly_TripleMax) { + MockReadExactly read_exactly; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = + std::numeric_limits::type>::max(); + EXPECT_CALL(read_exactly, ReadInt(0, max, false)).WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(increment, max - increment, false)) + .WillOnce(Return(increment)); + EXPECT_CALL(read_exactly, ReadInt(2 * increment, 1, false)) + .WillOnce(Return(1)); + EXPECT_TRUE(read_exactly.ReadExactlyInt(0, max, false)); +} + +class MockWriteAll : public internal::WriteAllInternal { + public: + MockWriteAll() : WriteAllInternal() {} + ~MockWriteAll() {} + + // Since it’s more convenient for the test to use uintptr_t than const void*, + // WriteAllInt() and WriteInt() adapt the types. + + bool WriteAllInt(uintptr_t data, size_t size) { + return WriteAll(reinterpret_cast(data), size); + } + + MOCK_METHOD2(WriteInt, FileOperationResult(uintptr_t, size_t)); + + // WriteAllInternal: + FileOperationResult Write(const void* data, size_t size) { + return WriteInt(reinterpret_cast(data), size); + } + + private: + DISALLOW_COPY_AND_ASSIGN(MockWriteAll); +}; + +TEST(FileIO, WriteAll_Zero) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(_, _)).Times(0); + EXPECT_TRUE(write_all.WriteAllInt(100, 0)); +} + +TEST(FileIO, WriteAll_SingleSmallSuccess) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(1000, 1)).WillOnce(Return(1)); + EXPECT_TRUE(write_all.WriteAllInt(1000, 1)); +} + +TEST(FileIO, WriteAll_SingleSmallFailure) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(1000, 1)).WillOnce(Return(-1)); + EXPECT_FALSE(write_all.WriteAllInt(1000, 1)); +} + +TEST(FileIO, WriteAll_DoubleSmall) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(0x1000, 2)).WillOnce(Return(1)); + EXPECT_CALL(write_all, WriteInt(0x1001, 1)).WillOnce(Return(1)); + EXPECT_TRUE(write_all.WriteAllInt(0x1000, 2)); +} + +TEST(FileIO, WriteAll_Medium) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(0x80000000, 0x20000000)) + .WillOnce(Return(0x10000000)); + EXPECT_CALL(write_all, WriteInt(0x90000000, 0x10000000)) + .WillOnce(Return(0x8000000)); + EXPECT_CALL(write_all, WriteInt(0x98000000, 0x8000000)) + .WillOnce(Return(0x4000000)); + EXPECT_CALL(write_all, WriteInt(0x9c000000, 0x4000000)) + .WillOnce(Return(0x2000000)); + EXPECT_CALL(write_all, WriteInt(0x9e000000, 0x2000000)) + .WillOnce(Return(0x1000000)); + EXPECT_CALL(write_all, WriteInt(0x9f000000, 0x1000000)) + .WillOnce(Return(0x800000)); + EXPECT_CALL(write_all, WriteInt(0x9f800000, 0x800000)) + .WillOnce(Return(0x400000)); + EXPECT_CALL(write_all, WriteInt(0x9fc00000, 0x400000)) + .WillOnce(Return(0x200000)); + EXPECT_CALL(write_all, WriteInt(0x9fe00000, 0x200000)) + .WillOnce(Return(0x100000)); + EXPECT_CALL(write_all, WriteInt(0x9ff00000, 0x100000)) + .WillOnce(Return(0x80000)); + EXPECT_CALL(write_all, WriteInt(0x9ff80000, 0x80000)) + .WillOnce(Return(0x40000)); + EXPECT_CALL(write_all, WriteInt(0x9ffc0000, 0x40000)) + .WillOnce(Return(0x20000)); + EXPECT_CALL(write_all, WriteInt(0x9ffe0000, 0x20000)) + .WillOnce(Return(0x10000)); + EXPECT_CALL(write_all, WriteInt(0x9fff0000, 0x10000)) + .WillOnce(Return(0x8000)); + EXPECT_CALL(write_all, WriteInt(0x9fff8000, 0x8000)).WillOnce(Return(0x4000)); + EXPECT_CALL(write_all, WriteInt(0x9fffc000, 0x4000)).WillOnce(Return(0x2000)); + EXPECT_CALL(write_all, WriteInt(0x9fffe000, 0x2000)).WillOnce(Return(0x1000)); + EXPECT_CALL(write_all, WriteInt(0x9ffff000, 0x1000)).WillOnce(Return(0x800)); + EXPECT_CALL(write_all, WriteInt(0x9ffff800, 0x800)).WillOnce(Return(0x400)); + EXPECT_CALL(write_all, WriteInt(0x9ffffc00, 0x400)).WillOnce(Return(0x200)); + EXPECT_CALL(write_all, WriteInt(0x9ffffe00, 0x200)).WillOnce(Return(0x100)); + EXPECT_CALL(write_all, WriteInt(0x9fffff00, 0x100)).WillOnce(Return(0x80)); + EXPECT_CALL(write_all, WriteInt(0x9fffff80, 0x80)).WillOnce(Return(0x40)); + EXPECT_CALL(write_all, WriteInt(0x9fffffc0, 0x40)).WillOnce(Return(0x20)); + EXPECT_CALL(write_all, WriteInt(0x9fffffe0, 0x20)).WillOnce(Return(0x10)); + EXPECT_CALL(write_all, WriteInt(0x9ffffff0, 0x10)).WillOnce(Return(0x8)); + EXPECT_CALL(write_all, WriteInt(0x9ffffff8, 0x8)).WillOnce(Return(0x4)); + EXPECT_CALL(write_all, WriteInt(0x9ffffffc, 0x4)).WillOnce(Return(0x2)); + EXPECT_CALL(write_all, WriteInt(0x9ffffffe, 0x2)).WillOnce(Return(0x1)); + EXPECT_CALL(write_all, WriteInt(0x9fffffff, 0x1)).WillOnce(Return(0x1)); + EXPECT_TRUE(write_all.WriteAllInt(0x80000000, 0x20000000)); +} + +TEST(FileIO, WriteAll_LargeSuccess) { + MockWriteAll write_all; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = std::numeric_limits::max(); + EXPECT_CALL(write_all, WriteInt(0, max)).WillOnce(Return(increment)); + EXPECT_CALL(write_all, WriteInt(increment, max - increment)) + .WillOnce(Return(increment)); + EXPECT_CALL(write_all, WriteInt(2 * increment, 1)).WillOnce(Return(1)); + EXPECT_TRUE(write_all.WriteAllInt(0, max)); +} + +TEST(FileIO, WriteAll_LargeFailure) { + MockWriteAll write_all; + InSequence in_sequence; + EXPECT_CALL(write_all, WriteInt(0, 0xffffffff)).WillOnce(Return(0x7fffffff)); + EXPECT_CALL(write_all, WriteInt(0x7fffffff, 0x80000000)).WillOnce(Return(-1)); + EXPECT_FALSE(write_all.WriteAllInt(0, 0xffffffff)); +} + +TEST(FileIO, WriteAll_TripleMax) { + MockWriteAll write_all; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = + std::numeric_limits::type>::max(); + EXPECT_CALL(write_all, WriteInt(0, max)).WillOnce(Return(increment)); + EXPECT_CALL(write_all, WriteInt(increment, max - increment)) + .WillOnce(Return(increment)); + EXPECT_CALL(write_all, WriteInt(2 * increment, 1)).WillOnce(Return(1)); + EXPECT_TRUE(write_all.WriteAllInt(0, max)); +} + void TestOpenFileForWrite(FileHandle (*opener)(const base::FilePath&, FileWriteMode, FilePermissions)) { diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index 189dc44d..467fc3c8 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -14,9 +14,11 @@ #include "util/file/file_io.h" +#include +#include + #include "base/files/file_path.h" #include "base/logging.h" -#include "base/numerics/safe_conversions.h" #include "base/strings/utf_string_conversions.h" namespace { @@ -37,6 +39,16 @@ namespace crashpad { namespace { +// kMaxReadWriteSize needs to be limited to the range of DWORD for the calls to +// ::ReadFile() and ::WriteFile(), and also limited to the range of +// FileOperationResult to be able to adequately express the number of bytes read +// and written in the return values from ReadFile() and NativeWriteFile(). In a +// 64-bit build, the former will control, and the limit will be (2^32)-1. In a +// 32-bit build, the latter will control, and the limit will be (2^31)-1. +constexpr size_t kMaxReadWriteSize = std::min( + static_cast(std::numeric_limits::max()), + static_cast(std::numeric_limits::max())); + FileHandle OpenFileForOutput(DWORD access, const base::FilePath& path, FileWriteMode mode, @@ -70,18 +82,39 @@ FileHandle OpenFileForOutput(DWORD access, } // namespace +namespace internal { + const char kNativeReadFunctionName[] = "ReadFile"; const char kNativeWriteFunctionName[] = "WriteFile"; -// TODO(scottmg): Handle > DWORD-sized reads and writes if necessary. +FileOperationResult NativeWriteFile(FileHandle file, + const void* buffer, + size_t size) { + // TODO(scottmg): This might need to handle the limit for pipes across a + // network in the future. + + const DWORD write_size = + static_cast(std::min(size, kMaxReadWriteSize)); + + DWORD bytes_written; + if (!::WriteFile(file, buffer, write_size, &bytes_written, nullptr)) + return -1; + + CHECK_NE(bytes_written, static_cast(-1)); + DCHECK_LE(static_cast(bytes_written), write_size); + return bytes_written; +} + +} // namespace internal FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { DCHECK(!IsSocketHandle(file)); + const DWORD read_size = static_cast(std::min(size, kMaxReadWriteSize)); + while (true) { - DWORD size_dword = base::checked_cast(size); DWORD bytes_read; - BOOL success = ::ReadFile(file, buffer, size_dword, &bytes_read, nullptr); + BOOL success = ::ReadFile(file, buffer, read_size, &bytes_read, nullptr); if (!success) { if (GetLastError() == ERROR_BROKEN_PIPE) { // When reading a pipe and the write handle has been closed, ReadFile @@ -93,7 +126,7 @@ FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { } CHECK_NE(bytes_read, static_cast(-1)); - DCHECK_LE(bytes_read, size_dword); + DCHECK_LE(bytes_read, read_size); if (bytes_read != 0 || GetFileType(file) != FILE_TYPE_PIPE) { // Zero bytes read for a file indicates reaching EOF. Zero bytes read from // a pipe indicates only that there was a zero byte WriteFile issued on @@ -103,21 +136,6 @@ FileOperationResult ReadFile(FileHandle file, void* buffer, size_t size) { } } -FileOperationResult WriteFile(FileHandle file, - const void* buffer, - size_t size) { - // TODO(scottmg): This might need to handle the limit for pipes across a - // network in the future. - DWORD size_dword = base::checked_cast(size); - DWORD bytes_written; - BOOL rv = ::WriteFile(file, buffer, size_dword, &bytes_written, nullptr); - if (!rv) - return -1; - CHECK_NE(bytes_written, static_cast(-1)); - CHECK_EQ(bytes_written, size_dword); - return bytes_written; -} - FileHandle OpenFileForRead(const base::FilePath& path) { return CreateFile(path.value().c_str(), GENERIC_READ, diff --git a/util/file/file_reader.cc b/util/file/file_reader.cc index b7fcffb5..6f272f01 100644 --- a/util/file/file_reader.cc +++ b/util/file/file_reader.cc @@ -20,36 +20,31 @@ namespace crashpad { +namespace { + +class FileReaderReadExactly final : public internal::ReadExactlyInternal { + public: + explicit FileReaderReadExactly(FileReaderInterface* file_reader) + : ReadExactlyInternal(), file_reader_(file_reader) {} + ~FileReaderReadExactly() {} + + private: + // ReadExactlyInternal: + FileOperationResult Read(void* buffer, size_t size, bool can_log) override { + DCHECK(can_log); + return file_reader_->Read(buffer, size); + } + + FileReaderInterface* file_reader_; // weak + + DISALLOW_COPY_AND_ASSIGN(FileReaderReadExactly); +}; + +} // namespace + bool FileReaderInterface::ReadExactly(void* data, size_t size) { - FileOperationResult expect = base::checked_cast(size); - char* data_c = static_cast(data); - - FileOperationResult total_bytes = 0; - while (size > 0) { - FileOperationResult bytes = Read(data, size); - if (bytes < 0) { - // Read() will have logged its own error. - return false; - } - - DCHECK_LE(static_cast(bytes), size); - - if (bytes == 0) { - break; - } - - data_c += bytes; - size -= bytes; - total_bytes += bytes; - } - - if (total_bytes != expect) { - LOG(ERROR) << "ReadExactly(): expected " << expect << ", observed " - << total_bytes; - return false; - } - - return true; + FileReaderReadExactly read_exactly(this); + return read_exactly.ReadExactly(data, size, true); } WeakFileHandleFileReader::WeakFileHandleFileReader(FileHandle file_handle) @@ -65,7 +60,7 @@ FileOperationResult WeakFileHandleFileReader::Read(void* data, size_t size) { base::checked_cast(size); FileOperationResult rv = ReadFile(file_handle_, data, size); if (rv < 0) { - PLOG(ERROR) << kNativeReadFunctionName; + PLOG(ERROR) << internal::kNativeReadFunctionName; return -1; } diff --git a/util/file/file_reader_test.cc b/util/file/file_reader_test.cc new file mode 100644 index 00000000..7c168bb0 --- /dev/null +++ b/util/file/file_reader_test.cc @@ -0,0 +1,205 @@ +// Copyright 2017 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/file/file_reader.h" + +#include + +#include +#include + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +using testing::_; +using testing::InSequence; +using testing::Return; + +class MockFileReader : public FileReaderInterface { + public: + MockFileReader() : FileReaderInterface() {} + ~MockFileReader() override {} + + // Since it’s more convenient for the test to use uintptr_t than void*, + // ReadExactlyInt() and ReadInt() adapt the types. + + bool ReadExactlyInt(uintptr_t data, size_t size) { + return ReadExactly(reinterpret_cast(data), size); + } + + MOCK_METHOD2(ReadInt, FileOperationResult(uintptr_t, size_t)); + + // FileReaderInterface: + FileOperationResult Read(void* data, size_t size) { + return ReadInt(reinterpret_cast(data), size); + } + + // FileSeekerInterface: + MOCK_METHOD2(Seek, FileOffset(FileOffset, int)); + + private: + DISALLOW_COPY_AND_ASSIGN(MockFileReader); +}; + +TEST(FileReader, ReadExactly_Zero) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(_, _)).Times(0); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(100, 0)); +} + +TEST(FileReader, ReadExactly_SingleSmallSuccess) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(1000, 1)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(1000, 1)); +} + +TEST(FileReader, ReadExactly_SingleSmallFailure) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(1000, 1)).WillOnce(Return(-1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_FALSE(file_reader.ReadExactlyInt(1000, 1)); +} + +TEST(FileReader, ReadExactly_DoubleSmallSuccess) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(0x1000, 2)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, ReadInt(0x1001, 1)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(0x1000, 2)); +} + +TEST(FileReader, ReadExactly_DoubleSmallShort) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(0x20000, 2)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, ReadInt(0x20001, 1)).WillOnce(Return(0)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_FALSE(file_reader.ReadExactlyInt(0x20000, 2)); +} + +TEST(FileReader, ReadExactly_Medium) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(0x80000000, 0x20000000)) + .WillOnce(Return(0x10000000)); + EXPECT_CALL(file_reader, ReadInt(0x90000000, 0x10000000)) + .WillOnce(Return(0x8000000)); + EXPECT_CALL(file_reader, ReadInt(0x98000000, 0x8000000)) + .WillOnce(Return(0x4000000)); + EXPECT_CALL(file_reader, ReadInt(0x9c000000, 0x4000000)) + .WillOnce(Return(0x2000000)); + EXPECT_CALL(file_reader, ReadInt(0x9e000000, 0x2000000)) + .WillOnce(Return(0x1000000)); + EXPECT_CALL(file_reader, ReadInt(0x9f000000, 0x1000000)) + .WillOnce(Return(0x800000)); + EXPECT_CALL(file_reader, ReadInt(0x9f800000, 0x800000)) + .WillOnce(Return(0x400000)); + EXPECT_CALL(file_reader, ReadInt(0x9fc00000, 0x400000)) + .WillOnce(Return(0x200000)); + EXPECT_CALL(file_reader, ReadInt(0x9fe00000, 0x200000)) + .WillOnce(Return(0x100000)); + EXPECT_CALL(file_reader, ReadInt(0x9ff00000, 0x100000)) + .WillOnce(Return(0x80000)); + EXPECT_CALL(file_reader, ReadInt(0x9ff80000, 0x80000)) + .WillOnce(Return(0x40000)); + EXPECT_CALL(file_reader, ReadInt(0x9ffc0000, 0x40000)) + .WillOnce(Return(0x20000)); + EXPECT_CALL(file_reader, ReadInt(0x9ffe0000, 0x20000)) + .WillOnce(Return(0x10000)); + EXPECT_CALL(file_reader, ReadInt(0x9fff0000, 0x10000)) + .WillOnce(Return(0x8000)); + EXPECT_CALL(file_reader, ReadInt(0x9fff8000, 0x8000)) + .WillOnce(Return(0x4000)); + EXPECT_CALL(file_reader, ReadInt(0x9fffc000, 0x4000)) + .WillOnce(Return(0x2000)); + EXPECT_CALL(file_reader, ReadInt(0x9fffe000, 0x2000)) + .WillOnce(Return(0x1000)); + EXPECT_CALL(file_reader, ReadInt(0x9ffff000, 0x1000)).WillOnce(Return(0x800)); + EXPECT_CALL(file_reader, ReadInt(0x9ffff800, 0x800)).WillOnce(Return(0x400)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffc00, 0x400)).WillOnce(Return(0x200)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffe00, 0x200)).WillOnce(Return(0x100)); + EXPECT_CALL(file_reader, ReadInt(0x9fffff00, 0x100)).WillOnce(Return(0x80)); + EXPECT_CALL(file_reader, ReadInt(0x9fffff80, 0x80)).WillOnce(Return(0x40)); + EXPECT_CALL(file_reader, ReadInt(0x9fffffc0, 0x40)).WillOnce(Return(0x20)); + EXPECT_CALL(file_reader, ReadInt(0x9fffffe0, 0x20)).WillOnce(Return(0x10)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffff0, 0x10)).WillOnce(Return(0x8)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffff8, 0x8)).WillOnce(Return(0x4)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffffc, 0x4)).WillOnce(Return(0x2)); + EXPECT_CALL(file_reader, ReadInt(0x9ffffffe, 0x2)).WillOnce(Return(0x1)); + EXPECT_CALL(file_reader, ReadInt(0x9fffffff, 0x1)).WillOnce(Return(0x1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(0x80000000, 0x20000000)); +} + +TEST(FileReader, ReadExactly_LargeSuccess) { + MockFileReader file_reader; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = std::numeric_limits::max(); + EXPECT_CALL(file_reader, ReadInt(0, max)).WillOnce(Return(increment)); + EXPECT_CALL(file_reader, ReadInt(increment, max - increment)) + .WillOnce(Return(increment)); + EXPECT_CALL(file_reader, ReadInt(2 * increment, 1)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(0, max)); +} + +TEST(FileReader, ReadExactly_LargeShort) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(0, 0xffffffff)).WillOnce(Return(0x7fffffff)); + EXPECT_CALL(file_reader, ReadInt(0x7fffffff, 0x80000000)) + .WillOnce(Return(0x10000000)); + EXPECT_CALL(file_reader, ReadInt(0x8fffffff, 0x70000000)).WillOnce(Return(0)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0xffffffff)); +} + +TEST(FileReader, ReadExactly_LargeFailure) { + MockFileReader file_reader; + InSequence in_sequence; + EXPECT_CALL(file_reader, ReadInt(0, 0xffffffff)).WillOnce(Return(0x7fffffff)); + EXPECT_CALL(file_reader, ReadInt(0x7fffffff, 0x80000000)) + .WillOnce(Return(-1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_FALSE(file_reader.ReadExactlyInt(0, 0xffffffff)); +} + +TEST(FileReader, ReadExactly_TripleMax) { + MockFileReader file_reader; + InSequence in_sequence; + constexpr size_t max = std::numeric_limits::max(); + constexpr size_t increment = + std::numeric_limits::type>::max(); + EXPECT_CALL(file_reader, ReadInt(0, max)).WillOnce(Return(increment)); + EXPECT_CALL(file_reader, ReadInt(increment, max - increment)) + .WillOnce(Return(increment)); + EXPECT_CALL(file_reader, ReadInt(2 * increment, 1)).WillOnce(Return(1)); + EXPECT_CALL(file_reader, Seek(_, _)).Times(0); + EXPECT_TRUE(file_reader.ReadExactlyInt(0, max)); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util_test.gyp b/util/util_test.gyp index e1d2c02d..495c5e61 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -37,6 +37,7 @@ 'sources': [ 'file/delimited_file_reader_test.cc', 'file/file_io_test.cc', + 'file/file_reader_test.cc', 'file/string_file_test.cc', 'mac/launchd_test.mm', 'mac/mac_util_test.mm',