From 2815dbdf8e053e90509c17e7dc7f7dd362d7e2be Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Wed, 19 Apr 2017 13:45:41 -0700 Subject: [PATCH 01/32] linux: Add CheckedLinuxAddressRange and make CheckedAddressRanges copyable Bug: crashpad:30 Change-Id: Ied2b8659315c09c77054c0a5a82ac37284f27334 Reviewed-on: https://chromium-review.googlesource.com/481036 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/linux/checked_linux_address_range.h | 37 ++++++++++++++++++++++++ util/numeric/checked_address_range.cc | 11 +++++-- util/numeric/checked_address_range.h | 2 -- util/util.gyp | 1 + util/util_test.gyp | 6 ---- 5 files changed, 47 insertions(+), 10 deletions(-) create mode 100644 util/linux/checked_linux_address_range.h diff --git a/util/linux/checked_linux_address_range.h b/util/linux/checked_linux_address_range.h new file mode 100644 index 00000000..6bf86ec5 --- /dev/null +++ b/util/linux/checked_linux_address_range.h @@ -0,0 +1,37 @@ +// 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_LINUX_CHECKED_LINUX_ADDRESS_RANGE_H_ +#define CRASHPAD_UTIL_LINUX_CHECKED_LINUX_ADDRESS_RANGE_H_ + +#include "util/linux/address_types.h" +#include "util/numeric/checked_address_range.h" + +namespace crashpad { + +//! \brief Ensures that a range, composed of a base and a size, does not +//! overflow the pointer type of the process it describes a range in. +//! +//! This class checks bases of type #LinuxVMAddress and sizes of type +//! #LinuxVMSize against a process whose pointer type is either 32 or 64 +//! bits wide. +//! +//! Aside from varying the overall range on the basis of a process’ pointer type +//! width, this class functions very similarly to CheckedRange. +using CheckedLinuxAddressRange = + internal::CheckedAddressRangeGeneric; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_LINUX_CHECKED_LINUX_ADDRESS_RANGE_H_ diff --git a/util/numeric/checked_address_range.cc b/util/numeric/checked_address_range.cc index 1d50696f..17f77618 100644 --- a/util/numeric/checked_address_range.cc +++ b/util/numeric/checked_address_range.cc @@ -14,12 +14,15 @@ #include "util/numeric/checked_address_range.h" +#include "base/format_macros.h" #include "base/strings/stringprintf.h" #if defined(OS_MACOSX) #include #elif defined(OS_WIN) #include "util/win/address_types.h" +#elif defined(OS_LINUX) || defined(OS_ANDROID) +#include "util/linux/address_types.h" #endif // OS_MACOSX namespace crashpad { @@ -113,8 +116,10 @@ bool CheckedAddressRangeGeneric::ContainsRange( template std::string CheckedAddressRangeGeneric::AsString() const { - return base::StringPrintf( - "0x%llx + 0x%llx (%s)", Base(), Size(), Is64Bit() ? "64" : "32"); + return base::StringPrintf("0x%" PRIx64 " + 0x%" PRIx64 " (%s)", + uint64_t{Base()}, + uint64_t{Size()}, + Is64Bit() ? "64" : "32"); } // Explicit instantiations for the cases we use. @@ -122,6 +127,8 @@ std::string CheckedAddressRangeGeneric::AsString() const { template class CheckedAddressRangeGeneric; #elif defined(OS_WIN) template class CheckedAddressRangeGeneric; +#elif defined(OS_LINUX) || defined(OS_ANDROID) +template class CheckedAddressRangeGeneric; #endif // OS_MACOSX } // namespace internal diff --git a/util/numeric/checked_address_range.h b/util/numeric/checked_address_range.h index 302e27d6..40c82390 100644 --- a/util/numeric/checked_address_range.h +++ b/util/numeric/checked_address_range.h @@ -143,8 +143,6 @@ class CheckedAddressRangeGeneric { // uniformly, but these types are too wide for the underlying pointer and size // types in 32-bit processes. bool range_ok_; - - DISALLOW_COPY_AND_ASSIGN(CheckedAddressRangeGeneric); }; } // namespace internal diff --git a/util/util.gyp b/util/util.gyp index 74613f93..576803db 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -45,6 +45,7 @@ 'file/string_file.cc', 'file/string_file.h', 'linux/address_types.h', + 'linux/checked_address_range.h', 'linux/process_memory.cc', 'linux/process_memory.h', 'linux/scoped_ptrace_attach.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 20e0c954..7e1661df 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -135,12 +135,6 @@ ['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$'], - ] - }], ], 'target_conditions': [ ['OS=="android"', { From fd8e2de0c502d507f7dbc01ab832b7657e0f96b2 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 19 Apr 2017 20:10:33 -0400 Subject: [PATCH 02/32] win: MSVS 2017 (15)/C++ 14.1/C 19.10 compatibility Includes mini_chromium ef0ded8717340c9fe48e8e0f34f3e0e74d10a392. 1d2a024fdb1d android: Use _FILE_OFFSET_BITS after all (undo dc3d480305b2) ef0ded871734 win: MSVS 2017 (15)/C++ 14.1/C 19.10 compatibility Change-Id: I5c814669a0ef8577872bddff9112ce28ec628ba3 Reviewed-on: https://chromium-review.googlesource.com/482639 Commit-Queue: Mark Mentovai Reviewed-by: Scott Graham --- DEPS | 2 +- client/prune_crash_reports_test.cc | 6 ++++-- snapshot/win/crashpad_snapshot_test_crashing_child.cc | 1 + .../win/crashpad_snapshot_test_dump_without_crashing.cc | 1 + 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/DEPS b/DEPS index 5c35e6d6..74e2522f 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - 'dc3d480305b27a5a1fb57f51a997529e00fed00b', + 'ef0ded8717340c9fe48e8e0f34f3e0e74d10a392', 'crashpad/third_party/zlib/zlib': Var('chromium_git') + '/chromium/src/third_party/zlib@' + '13dc246a58e4b72104d35f9b1809af95221ebda7', diff --git a/client/prune_crash_reports_test.cc b/client/prune_crash_reports_test.cc index 14eb2e0b..130ea72a 100644 --- a/client/prune_crash_reports_test.cc +++ b/client/prune_crash_reports_test.cc @@ -14,12 +14,14 @@ #include "client/prune_crash_reports.h" +#include #include #include #include #include +#include "base/numerics/safe_conversions.h" #include "base/rand_util.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -209,8 +211,8 @@ TEST(PruneCrashReports, PruneOrder) { reports.push_back(temp); } // The randomness from std::rand() is not, so use a better rand() instead. - const auto random_generator = [](int rand_max) { - return base::RandInt(0, rand_max - 1); + const auto random_generator = [](ptrdiff_t rand_max) { + return base::RandInt(0, base::checked_cast(rand_max) - 1); }; std::random_shuffle(reports.begin(), reports.end(), random_generator); std::vector pending_reports( diff --git a/snapshot/win/crashpad_snapshot_test_crashing_child.cc b/snapshot/win/crashpad_snapshot_test_crashing_child.cc index 2cfb9906..b0caa065 100644 --- a/snapshot/win/crashpad_snapshot_test_crashing_child.cc +++ b/snapshot/win/crashpad_snapshot_test_crashing_child.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include "base/files/file_path.h" diff --git a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc index b6c9814a..a9fbdcf5 100644 --- a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc +++ b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include "base/logging.h" From 4036e2c9d92c1cf626e8072c710ddcc83ad429b9 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 25 Apr 2017 08:12:49 -0700 Subject: [PATCH 03/32] linux: Add MemoryMap to collect information about mapped memory regions Bug: crashpad:30 Change-Id: Id11d549829bd1a956d31991d4b829a43ce5696aa Reviewed-on: https://chromium-review.googlesource.com/477597 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/linux/memory_map.cc | 207 +++++++++++++++++++++ util/linux/memory_map.h | 78 ++++++++ util/linux/memory_map_test.cc | 329 ++++++++++++++++++++++++++++++++++ util/util.gyp | 2 + util/util_test.gyp | 1 + 5 files changed, 617 insertions(+) create mode 100644 util/linux/memory_map.cc create mode 100644 util/linux/memory_map.h create mode 100644 util/linux/memory_map_test.cc diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc new file mode 100644 index 00000000..cbf0c4e4 --- /dev/null +++ b/util/linux/memory_map.cc @@ -0,0 +1,207 @@ +// 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/linux/memory_map.h" + +#include +#include +#include + +#include "base/files/file_path.h" +#include "base/logging.h" +#include "build/build_config.h" +#include "util/file/delimited_file_reader.h" +#include "util/file/file_reader.h" +#include "util/stdlib/string_number_conversion.h" + +namespace crashpad { + +namespace { + +// This function is used in this file specfically for signed or unsigned longs. +// longs are typically either int or int64 sized, but pointers to longs are not +// automatically coerced to pointers to ints when they are the same size. +// Simply adding a StringToNumber for longs doesn't work since sometimes long +// and int64_t are actually the same type, resulting in a redefinition error. +template +bool LocalStringToNumber(const base::StringPiece& string, Type* number) { + static_assert(sizeof(Type) == sizeof(int) || sizeof(Type) == sizeof(int64_t), + "Unexpected Type size"); + + if (sizeof(Type) == sizeof(int)) { + return std::numeric_limits::is_signed + ? StringToNumber(string, reinterpret_cast(number)) + : StringToNumber(string, + reinterpret_cast(number)); + } else { + return std::numeric_limits::is_signed + ? StringToNumber(string, reinterpret_cast(number)) + : StringToNumber(string, reinterpret_cast(number)); + } +} + +template +bool HexStringToNumber(const std::string& string, Type* number) { + return LocalStringToNumber("0x" + string, number); +} + +} // namespace + +MemoryMap::Mapping::Mapping() + : name(), + range(false, 0, 0), + offset(0), + device(0), + inode(0), + readable(false), + writable(false), + executable(false), + shareable(false) {} + +MemoryMap::MemoryMap() : mappings_(), initialized_() {} + +MemoryMap::~MemoryMap() {} + +bool MemoryMap::Initialize(pid_t pid) { + INITIALIZATION_STATE_SET_INITIALIZING(initialized_); + + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/maps", pid); + FileReader maps_file; + if (!maps_file.Open(base::FilePath(path))) { + return false; + } + DelimitedFileReader maps_file_reader(&maps_file); + + DelimitedFileReader::Result result; + std::string field; + while ((result = maps_file_reader.GetDelim('-', &field)) == + DelimitedFileReader::Result::kSuccess) { + field.pop_back(); + + LinuxVMAddress start_address; + if (!HexStringToNumber(field, &start_address)) { + LOG(ERROR) << "format error"; + return false; + } + + LinuxVMAddress end_address; + if (maps_file_reader.GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &end_address))) { + LOG(ERROR) << "format error"; + return false; + } + if (end_address <= start_address) { + LOG(ERROR) << "format error"; + return false; + } + + // TODO(jperaza): set bitness properly +#if defined(ARCH_CPU_64_BITS) + const bool is_64_bit = true; +#else + const bool is_64_bit = false; +#endif + + Mapping mapping; + mapping.range.SetRange( + is_64_bit, start_address, end_address - start_address); + + if (maps_file_reader.GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 4)) { + LOG(ERROR) << "format error"; + return false; + } +#define SET_FIELD(actual_c, outval, true_chars, false_chars) \ + do { \ + if (strchr(true_chars, actual_c)) { \ + *outval = true; \ + } else if (strchr(false_chars, actual_c)) { \ + *outval = false; \ + } else { \ + LOG(ERROR) << "format error"; \ + return false; \ + } \ + } while (false) + SET_FIELD(field[0], &mapping.readable, "r", "-"); + SET_FIELD(field[1], &mapping.writable, "w", "-"); + SET_FIELD(field[2], &mapping.executable, "x", "-"); + SET_FIELD(field[3], &mapping.shareable, "sS", "p"); +#undef SET_FIELD + + if (maps_file_reader.GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &mapping.offset))) { + LOG(ERROR) << "format error"; + return false; + } + + int major, minor; + if (maps_file_reader.GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 5) || + !HexStringToNumber(field.substr(0, 2), &major) || + !HexStringToNumber(field.substr(3, 2), &minor)) { + LOG(ERROR) << "format error"; + return false; + } + mapping.device = MKDEV(major, minor); + + if (maps_file_reader.GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !LocalStringToNumber(field, &mapping.inode))) { + LOG(ERROR) << "format error"; + return false; + } + + if (maps_file_reader.GetDelim('\n', &field) != + DelimitedFileReader::Result::kSuccess) { + LOG(ERROR) << "format error"; + return false; + } + if (field.back() != '\n') { + LOG(ERROR) << "format error"; + return false; + } + field.pop_back(); + + mappings_.push_back(mapping); + + size_t path_start = field.find_first_not_of(' '); + if (path_start != std::string::npos) { + mappings_.back().name = field.substr(path_start); + } + } + if (result != DelimitedFileReader::Result::kEndOfFile) { + return false; + } + + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; +} + +const MemoryMap::Mapping* MemoryMap::FindMapping(LinuxVMAddress address) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + for (const auto& mapping : mappings_) { + if (mapping.range.Base() <= address && mapping.range.End() > address) { + return &mapping; + } + } + return nullptr; +} + +} // namespace crashpad diff --git a/util/linux/memory_map.h b/util/linux/memory_map.h new file mode 100644 index 00000000..2d7a01a1 --- /dev/null +++ b/util/linux/memory_map.h @@ -0,0 +1,78 @@ +// 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_LINUX_MEMORY_MAP_H_ +#define CRASHPAD_UTIL_LINUX_MEMORY_MAP_H_ + +#include + +#include +#include + +#include "util/linux/address_types.h" +#include "util/linux/checked_linux_address_range.h" +#include "util/misc/initialization_state_dcheck.h" + +namespace crashpad { + +//! \brief Accesses information about mapped memory in another process. +//! +//! The target process must be stopped to guarantee correct mappings. If the +//! target process is not stopped, mappings may be invalid after the return from +//! Initialize(), and even mappings existing at the time Initialize() was called +//! may not be found. +class MemoryMap { + public: + //! \brief Information about a mapped region of memory. + struct Mapping { + Mapping(); + + std::string name; + CheckedLinuxAddressRange range; + off_t offset; + dev_t device; + ino_t inode; + bool readable; + bool writable; + bool executable; + bool shareable; + }; + + MemoryMap(); + ~MemoryMap(); + + //! \brief Initializes this object with information about the mapped memory + //! regions in the process whose ID is \a pid. + //! + //! This method must be called successfully prior to calling any other method + //! in this class. This method may only be called once. + //! + //! \param[in] pid The process ID to obtain information for. + //! + //! \return `true` on success, `false` on failure with a message logged. + bool Initialize(pid_t pid); + + //! \return The Mapping containing \a address. The caller does not take + //! ownership of this object. It is scoped to the lifetime of the + //! MemoryMap object that it was obtained from. + const Mapping* FindMapping(LinuxVMAddress address) const; + + private: + std::vector mappings_; + InitializationStateDcheck initialized_; +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_LINUX_MEMORY_MAP_H_ diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc new file mode 100644 index 00000000..1ae9b7d4 --- /dev/null +++ b/util/linux/memory_map_test.cc @@ -0,0 +1,329 @@ +// 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/linux/memory_map.h" + +#include +#include +#include +#include +#include + +#include "base/files/file_path.h" +#include "gtest/gtest.h" +#include "test/errors.h" +#include "test/file.h" +#include "test/multiprocess.h" +#include "test/scoped_temp_dir.h" +#include "util/file/file_io.h" +#include "util/misc/clock.h" +#include "util/posix/scoped_mmap.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(MemoryMap, SelfBasic) { + ScopedMmap mmapping; + ASSERT_TRUE(mmapping.ResetMmap(nullptr, + getpagesize(), + PROT_EXEC | PROT_READ, + MAP_SHARED | MAP_ANON, + -1, + 0)); + MemoryMap map; + ASSERT_TRUE(map.Initialize(getpid())); + + auto stack_address = reinterpret_cast(&map); + const MemoryMap::Mapping* mapping = map.FindMapping(stack_address); + ASSERT_TRUE(mapping); + EXPECT_GE(stack_address, mapping->range.Base()); + EXPECT_LT(stack_address, mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_TRUE(mapping->writable); + + auto code_address = reinterpret_cast(getpid); + mapping = map.FindMapping(code_address); + ASSERT_TRUE(mapping); + EXPECT_GE(code_address, mapping->range.Base()); + EXPECT_LT(code_address, mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_FALSE(mapping->writable); + EXPECT_TRUE(mapping->executable); + + auto mapping_address = mmapping.addr_as(); + mapping = map.FindMapping(mapping_address); + ASSERT_TRUE(mapping); + mapping = map.FindMapping(mapping_address + mmapping.len() - 1); + ASSERT_TRUE(mapping); + EXPECT_EQ(mapping_address, mapping->range.Base()); + EXPECT_EQ(mapping_address + mmapping.len(), mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_FALSE(mapping->writable); + EXPECT_TRUE(mapping->executable); + EXPECT_TRUE(mapping->shareable); +} + +class MapChildTest : public Multiprocess { + public: + MapChildTest() : Multiprocess(), page_size_(getpagesize()) {} + ~MapChildTest() {} + + private: + void MultiprocessParent() override { + LinuxVMAddress code_address; + CheckedReadFileExactly( + ReadPipeHandle(), &code_address, sizeof(code_address)); + + LinuxVMAddress stack_address; + CheckedReadFileExactly( + ReadPipeHandle(), &stack_address, sizeof(stack_address)); + + LinuxVMAddress mapped_address; + CheckedReadFileExactly( + ReadPipeHandle(), &mapped_address, sizeof(mapped_address)); + + LinuxVMAddress mapped_file_address; + CheckedReadFileExactly( + ReadPipeHandle(), &mapped_file_address, sizeof(mapped_file_address)); + LinuxVMSize path_length; + CheckedReadFileExactly(ReadPipeHandle(), &path_length, sizeof(path_length)); + std::string mapped_file_name(path_length, std::string::value_type()); + CheckedReadFileExactly(ReadPipeHandle(), &mapped_file_name[0], path_length); + + MemoryMap map; + ASSERT_TRUE(map.Initialize(ChildPID())); + + const MemoryMap::Mapping* mapping = map.FindMapping(code_address); + ASSERT_TRUE(mapping); + EXPECT_GE(code_address, mapping->range.Base()); + EXPECT_LT(code_address, mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_TRUE(mapping->executable); + EXPECT_FALSE(mapping->writable); + + mapping = map.FindMapping(stack_address); + ASSERT_TRUE(mapping); + EXPECT_GE(stack_address, mapping->range.Base()); + EXPECT_LT(stack_address, mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_TRUE(mapping->writable); + + mapping = map.FindMapping(mapped_address); + ASSERT_TRUE(mapping); + EXPECT_EQ(mapped_address, mapping->range.Base()); + EXPECT_EQ(mapped_address + page_size_, mapping->range.End()); + EXPECT_FALSE(mapping->readable); + EXPECT_FALSE(mapping->writable); + EXPECT_FALSE(mapping->executable); + EXPECT_TRUE(mapping->shareable); + + mapping = map.FindMapping(mapped_file_address); + ASSERT_TRUE(mapping); + EXPECT_EQ(mapped_file_address, mapping->range.Base()); + EXPECT_EQ(mapping->offset, static_cast(page_size_)); + EXPECT_TRUE(mapping->readable); + EXPECT_TRUE(mapping->writable); + EXPECT_FALSE(mapping->executable); + EXPECT_FALSE(mapping->shareable); + EXPECT_EQ(mapping->name, mapped_file_name); + struct stat file_stat; + ASSERT_EQ(stat(mapped_file_name.c_str(), &file_stat), 0) + << ErrnoMessage("stat"); + EXPECT_EQ(mapping->device, file_stat.st_dev); + EXPECT_EQ(mapping->inode, file_stat.st_ino); + } + + void MultiprocessChild() override { + auto code_address = reinterpret_cast(getpid); + CheckedWriteFile(WritePipeHandle(), &code_address, sizeof(code_address)); + + auto stack_address = reinterpret_cast(&code_address); + CheckedWriteFile(WritePipeHandle(), &stack_address, sizeof(stack_address)); + + ScopedMmap mapping; + ASSERT_TRUE(mapping.ResetMmap( + nullptr, page_size_, PROT_NONE, MAP_SHARED | MAP_ANON, -1, 0)); + auto mapped_address = mapping.addr_as(); + CheckedWriteFile( + WritePipeHandle(), &mapped_address, sizeof(mapped_address)); + + ScopedTempDir temp_dir; + base::FilePath path = + temp_dir.path().Append(FILE_PATH_LITERAL("test_file")); + ASSERT_FALSE(FileExists(path)); + std::string path_string = path.value(); + ScopedFileHandle handle(LoggingOpenFileForReadAndWrite( + path, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); + ASSERT_TRUE(handle.is_valid()); + std::unique_ptr file_contents(new char[page_size_ * 2]); + CheckedWriteFile(handle.get(), file_contents.get(), page_size_ * 2); + + ScopedMmap file_mapping; + ASSERT_TRUE(file_mapping.ResetMmap(nullptr, + page_size_, + PROT_READ | PROT_WRITE, + MAP_PRIVATE, + handle.get(), + page_size_)); + + auto mapped_file_address = file_mapping.addr_as(); + CheckedWriteFile( + WritePipeHandle(), &mapped_file_address, sizeof(mapped_file_address)); + LinuxVMSize path_length = path_string.size(); + CheckedWriteFile(WritePipeHandle(), &path_length, sizeof(path_length)); + CheckedWriteFile(WritePipeHandle(), path_string.c_str(), path_length); + + CheckedReadFileAtEOF(ReadPipeHandle()); + } + + const size_t page_size_; + + DISALLOW_COPY_AND_ASSIGN(MapChildTest); +}; + +TEST(MemoryMap, MapChild) { + MapChildTest test; + test.Run(); +} + +// Some systems optimize mappings by allocating new mappings inside existing +// mappings with matching permissions. Work around this by allocating one large +// mapping and then switching up the permissions of individual pages to force +// populating more entries in the maps file. +void InitializeMappings(ScopedMmap* mappings, + size_t num_mappings, + size_t mapping_size) { + ASSERT_TRUE(mappings->ResetMmap(nullptr, + mapping_size * num_mappings, + PROT_READ, + MAP_PRIVATE | MAP_ANON, + -1, + 0)); + + auto region = mappings->addr_as(); + for (size_t index = 0; index < num_mappings; index += 2) { + ASSERT_EQ(mprotect(reinterpret_cast(region + index * mapping_size), + mapping_size, + PROT_READ | PROT_WRITE), + 0) + << ErrnoMessage("mprotect"); + } +} + +void ExpectMappings(const MemoryMap& map, + LinuxVMAddress region_addr, + size_t num_mappings, + size_t mapping_size) { + for (size_t index = 0; index < num_mappings; ++index) { + auto mapping_address = region_addr + index * mapping_size; + const MemoryMap::Mapping* mapping = map.FindMapping(mapping_address); + ASSERT_TRUE(mapping); + EXPECT_EQ(mapping_address, mapping->range.Base()); + EXPECT_EQ(mapping_address + mapping_size, mapping->range.End()); + EXPECT_TRUE(mapping->readable); + EXPECT_FALSE(mapping->shareable); + EXPECT_FALSE(mapping->executable); + if (index % 2 == 0) { + EXPECT_TRUE(mapping->writable); + } else { + EXPECT_FALSE(mapping->writable); + } + } +} + +TEST(MemoryMap, SelfLargeMapFile) { + constexpr size_t kNumMappings = 4096; + const size_t page_size = getpagesize(); + ScopedMmap mappings; + + ASSERT_NO_FATAL_FAILURE( + InitializeMappings(&mappings, kNumMappings, page_size)); + + MemoryMap map; + ASSERT_TRUE(map.Initialize(getpid())); + + ExpectMappings( + map, mappings.addr_as(), kNumMappings, page_size); +} + +class MapRunningChildTest : public Multiprocess { + public: + MapRunningChildTest() : Multiprocess(), page_size_(getpagesize()) {} + ~MapRunningChildTest() {} + + private: + void MultiprocessParent() override { + // Let the child get started + LinuxVMAddress region_addr; + CheckedReadFileExactly(ReadPipeHandle(), ®ion_addr, sizeof(region_addr)); + + // Let the child get back to its work + SleepNanoseconds(1000); + + for (int iter = 0; iter < 8; ++iter) { + MemoryMap map; + ASSERT_TRUE(map.Initialize(ChildPID())); + + // We should at least find the original mappings. The extra mappings may + // or + // not be found depending on scheduling. + ExpectMappings(map, region_addr, kNumMappings, page_size_); + } + } + + void MultiprocessChild() override { + ASSERT_EQ(fcntl(ReadPipeHandle(), F_SETFL, O_NONBLOCK), 0) + << ErrnoMessage("fcntl"); + + ScopedMmap mappings; + ASSERT_NO_FATAL_FAILURE( + InitializeMappings(&mappings, kNumMappings, page_size_)); + + // Let the parent start mapping us + auto region_addr = mappings.addr_as(); + CheckedWriteFile(WritePipeHandle(), ®ion_addr, sizeof(region_addr)); + + // But don't stop there! + constexpr size_t kNumExtraMappings = 1024; + ScopedMmap extra_mappings; + + while (true) { + ASSERT_NO_FATAL_FAILURE( + InitializeMappings(&extra_mappings, kNumExtraMappings, page_size_)); + + // Quit when the parent is done + char c; + FileOperationResult res = ReadFile(ReadPipeHandle(), &c, sizeof(c)); + if (res == 0) { + break; + } + ASSERT_EQ(errno, EAGAIN); + } + } + + static constexpr size_t kNumMappings = 4096; + const size_t page_size_; + + DISALLOW_COPY_AND_ASSIGN(MapRunningChildTest); +}; + +TEST(MemoryMap, MapRunningChild) { + MapRunningChildTest test; + test.Run(); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index 576803db..5a7947a1 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -46,6 +46,8 @@ 'file/string_file.h', 'linux/address_types.h', 'linux/checked_address_range.h', + 'linux/memory_map.cc', + 'linux/memory_map.h', 'linux/process_memory.cc', 'linux/process_memory.h', 'linux/scoped_ptrace_attach.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 7e1661df..6ee35258 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -39,6 +39,7 @@ 'file/file_io_test.cc', 'file/file_reader_test.cc', 'file/string_file_test.cc', + 'linux/memory_map_test.cc', 'linux/process_memory_test.cc', 'linux/scoped_ptrace_attach_test.cc', 'mac/launchd_test.mm', From f31459b266a4cd8c1cec536b9f3ee058216c07f6 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 25 Apr 2017 12:41:49 -0400 Subject: [PATCH 04/32] Update GYP to ffd524cefaad for MSVS 2017 (15)/C++ 14.1/C 19.10 support aae1e3efb507 CQ config: add gerrit CQAbility verifier. 95da7665b1a3 [win-test] loosen win-driver-target-type test eb296f67da07 [win] Add support for MS VS2017 (via Registry) 19495aa28282 Update test/no-cpp/gyptest-no-cpp. a94b02ec68fb Disable a bunch of tests on Mac ae76d9198630 Clean up gyptest.py b62d04ff85e6 win,ninja: ninja generator better on windows 8dc77241251e Disable flaky test/copies/gyptest-all under msvs e8850240a433 Fix MSVC++ 32-on-32 builds after b62d04ff85e6 ffd524cefaad win ninja/make: Always use a native compiler executable with MSVS 2017 developing.md is updated to call out supported toolchain versions, and to explain the CDB requirement for end_to_end_tests.py. Change-Id: Iace68009aa22acec7303ea02a2ded755645ea96c Reviewed-on: https://chromium-review.googlesource.com/486539 Reviewed-by: Scott Graham --- DEPS | 2 +- doc/developing.md | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/DEPS b/DEPS index 74e2522f..0c7f66be 100644 --- a/DEPS +++ b/DEPS @@ -25,7 +25,7 @@ deps = { 'd62d6c6556d96dda924382547c54a4b3afedb22c', 'crashpad/third_party/gyp/gyp': Var('chromium_git') + '/external/gyp@' + - 'a7055b3989c1074adca03b4b4829e7f0e57f6efd', + 'ffd524cefaad622e72995e852ffb0b18e83f8054', # TODO(scottmg): Consider pinning these. For now, we don't have any particular # reason to do so. diff --git a/doc/developing.md b/doc/developing.md index 429fff7f..b5ee935e 100644 --- a/doc/developing.md +++ b/doc/developing.md @@ -36,9 +36,14 @@ provides more detail. To develop Crashpad, the following tools are necessary, and must be present in the `$PATH` environment variable: - * Appropriate development tools. For macOS, this is - [Xcode](https://developer.apple.com/xcode/) and for Windows, it’s [Visual - Studio](https://www.visualstudio.com/). + * Appropriate development tools. + * On macOS, install [Xcode](https://developer.apple.com/xcode/). The latest + version is generally recommended. + * On Windows, install [Visual Studio](https://www.visualstudio.com/) with + C++ support and the Windows SDK. MSVS 2015 and MSVS 2017 are both + supported. Some tests also require the CDB debugger, installed with + [Debugging Tools for + Windows](https://msdn.microsoft.com/library/windows/hardware/ff551063.aspx). * Chromium’s [depot_tools](https://dev.chromium.org/developers/how-tos/depottools). * [Git](https://git-scm.com/). This is provided by Xcode on macOS and by @@ -191,6 +196,18 @@ $ cd ~/crashpad/crashpad $ python build/run_tests.py out/Debug ``` +### Windows + +On Windows, `end_to_end_test.py` requires the CDB debugger, installed with +[Debugging Tools for +Windows](https://msdn.microsoft.com/library/windows/hardware/ff551063.aspx). +This can be installed either as part of the [Windows Driver +Kit](https://go.microsoft.com/fwlink/p?LinkID=239721) or the [Windows +SDK](https://go.microsoft.com/fwlink/p?LinkID=271979). If the Windows SDK has +already been installed (possibly with Visual Studio) but Debugging Tools for +Windows is not present, it can be installed from Add or remove programs→Windows +Software Development Kit. + ### Android To test on Android, use [ADB (Android Debug From 44e32fe1233dc949503a1d0f9d02004b7b5412f2 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 25 Apr 2017 13:37:23 -0400 Subject: [PATCH 05/32] Tweak InitializationState tests that rely on undefined behavior These tests: - InitializationState.InitializationState - InitializationStateDcheckDeathTest.Destroyed_NotUninitialized - InitializationStateDcheckDeathTest.Destroyed_NotValid rely on certain behavior from destroyed objects. This is undefined behavior and we know it, but the whole point of the of InitializationState and InitializationStateDcheck destructors is to try to help catch other parts of the program making use of undefined behavior. To make it impossible for the memory that formerly hosted these objects to be repurposed during tests after the objects are destroyed, these tests that attempt to work with destroyed objects are changed to use placement new, so that the lifetimes of the objects can be decoupled from the lifetimes of the buffers. Test: crashpad_util_test InitializationState* Change-Id: Ie972a54116c8b90a21a502d3ba13623583dfac06 Reviewed-on: https://chromium-review.googlesource.com/486383 Reviewed-by: Joshua Peraza --- util/misc/initialization_state_dcheck_test.cc | 57 ++++++++++++------- util/misc/initialization_state_test.cc | 56 ++++++++++-------- 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/util/misc/initialization_state_dcheck_test.cc b/util/misc/initialization_state_dcheck_test.cc index b293ab72..2c407675 100644 --- a/util/misc/initialization_state_dcheck_test.cc +++ b/util/misc/initialization_state_dcheck_test.cc @@ -14,7 +14,12 @@ #include "util/misc/initialization_state_dcheck.h" +#include + +#include + #include "base/logging.h" +#include "base/memory/free_deleter.h" #include "gtest/gtest.h" #include "test/gtest_death_check.h" @@ -98,33 +103,45 @@ TEST(InitializationStateDcheckDeathTest, Destroyed_NotUninitialized) { // This tests that an attempt to reinitialize a destroyed object fails. See // the InitializationState.InitializationState test for an explanation of this // use-after-free test. - InitializationStateDcheck* initialization_state_dcheck_pointer; - { - InitializationStateDcheck initialization_state_dcheck; - initialization_state_dcheck_pointer = &initialization_state_dcheck; - INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); - } - ASSERT_DEATH_CHECK(INITIALIZATION_STATE_SET_INITIALIZING( - *initialization_state_dcheck_pointer), - "kStateUninitialized"); + std::unique_ptr + initialization_state_dcheck_buffer(static_cast( + malloc(sizeof(InitializationStateDcheck)))); + + InitializationStateDcheck* initialization_state_dcheck = + new (initialization_state_dcheck_buffer.get()) + InitializationStateDcheck(); + + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck); + INITIALIZATION_STATE_SET_VALID(*initialization_state_dcheck); + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck); + + initialization_state_dcheck->~InitializationStateDcheck(); + + ASSERT_DEATH_CHECK( + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck), + "kStateUninitialized"); } TEST(InitializationStateDcheckDeathTest, Destroyed_NotValid) { // This tests that an attempt to use a destroyed object fails. See the // InitializationState.InitializationState test for an explanation of this // use-after-free test. - InitializationStateDcheck* initialization_state_dcheck_pointer; - { - InitializationStateDcheck initialization_state_dcheck; - initialization_state_dcheck_pointer = &initialization_state_dcheck; - INITIALIZATION_STATE_SET_INITIALIZING(initialization_state_dcheck); - INITIALIZATION_STATE_SET_VALID(initialization_state_dcheck); - INITIALIZATION_STATE_DCHECK_VALID(initialization_state_dcheck); - } + std::unique_ptr + initialization_state_dcheck_buffer(static_cast( + malloc(sizeof(InitializationStateDcheck)))); + + InitializationStateDcheck* initialization_state_dcheck = + new (initialization_state_dcheck_buffer.get()) + InitializationStateDcheck(); + + INITIALIZATION_STATE_SET_INITIALIZING(*initialization_state_dcheck); + INITIALIZATION_STATE_SET_VALID(*initialization_state_dcheck); + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck); + + initialization_state_dcheck->~InitializationStateDcheck(); + ASSERT_DEATH_CHECK( - INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck_pointer), + INITIALIZATION_STATE_DCHECK_VALID(*initialization_state_dcheck), "kStateValid"); } diff --git a/util/misc/initialization_state_test.cc b/util/misc/initialization_state_test.cc index d427a140..9f462826 100644 --- a/util/misc/initialization_state_test.cc +++ b/util/misc/initialization_state_test.cc @@ -14,6 +14,11 @@ #include "util/misc/initialization_state.h" +#include + +#include + +#include "base/memory/free_deleter.h" #include "gtest/gtest.h" namespace crashpad { @@ -21,36 +26,41 @@ namespace test { namespace { TEST(InitializationState, InitializationState) { - InitializationState* initialization_state_pointer; - { - InitializationState initialization_state; - initialization_state_pointer = &initialization_state; + // Use placement new so that the buffer used to host the object remains live + // even after the object is destroyed. + std::unique_ptr + initialization_state_buffer( + static_cast(malloc(sizeof(InitializationState)))); - EXPECT_TRUE(initialization_state.is_uninitialized()); - EXPECT_FALSE(initialization_state.is_valid()); + InitializationState* initialization_state = + new (initialization_state_buffer.get()) InitializationState(); - initialization_state.set_invalid(); + EXPECT_TRUE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); - EXPECT_FALSE(initialization_state.is_uninitialized()); - EXPECT_FALSE(initialization_state.is_valid()); + initialization_state->set_invalid(); - initialization_state.set_valid(); + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); - EXPECT_FALSE(initialization_state.is_uninitialized()); - EXPECT_TRUE(initialization_state.is_valid()); - } + initialization_state->set_valid(); - // initialization_state_pointer points to something that no longer exists. - // This portion of the test is intended to check that after an - // InitializationState object goes out of scope, it will not be considered - // valid on a use-after-free, assuming that nothing else was written to its - // former home in memory. + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_TRUE(initialization_state->is_valid()); + + initialization_state->~InitializationState(); + + // initialization_state points to something that no longer exists. This + // portion of the test is intended to check that after an InitializationState + // object is destroyed, it will not be considered valid on a use-after-free, + // assuming that nothing else was written to its former home in memory. // - // This portion of the test is technically not valid C++, but it exists to - // test that the behavior is as desired when other code uses the language - // improperly. - EXPECT_FALSE(initialization_state_pointer->is_uninitialized()); - EXPECT_FALSE(initialization_state_pointer->is_valid()); + // Because initialization_state was constructed via placement new into a + // buffer that’s still valid and its destructor was called directly, this + // approximates use-after-free without risking that the memory formerly used + // for the InitializationState object has been repurposed. + EXPECT_FALSE(initialization_state->is_uninitialized()); + EXPECT_FALSE(initialization_state->is_valid()); } } // namespace From ed8e637817b1e2302618509f440528d8af8a485c Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 25 Apr 2017 14:21:58 -0400 Subject: [PATCH 06/32] linux: Fill a test file with zeroes instead of garbage in MemoryMapTest Bug: crashapd:30 Test: MemoryMap.MapChild Change-Id: I40cd1c3a1f37e7a9d0c344c50b79b15ae3842182 Reviewed-on: https://chromium-review.googlesource.com/486602 Reviewed-by: Joshua Peraza --- util/linux/memory_map_test.cc | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index 1ae9b7d4..e78cb83a 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -167,8 +167,8 @@ class MapChildTest : public Multiprocess { ScopedFileHandle handle(LoggingOpenFileForReadAndWrite( path, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); ASSERT_TRUE(handle.is_valid()); - std::unique_ptr file_contents(new char[page_size_ * 2]); - CheckedWriteFile(handle.get(), file_contents.get(), page_size_ * 2); + std::string file_contents(page_size_ * 2, std::string::value_type()); + CheckedWriteFile(handle.get(), file_contents.c_str(), file_contents.size()); ScopedMmap file_mapping; ASSERT_TRUE(file_mapping.ResetMmap(nullptr, @@ -277,8 +277,7 @@ class MapRunningChildTest : public Multiprocess { ASSERT_TRUE(map.Initialize(ChildPID())); // We should at least find the original mappings. The extra mappings may - // or - // not be found depending on scheduling. + // or not be found depending on scheduling. ExpectMappings(map, region_addr, kNumMappings, page_size_); } } From 984749479f569e7fb43d44aadc39dcc5d6090be0 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 27 Apr 2017 15:08:17 -0400 Subject: [PATCH 07/32] Introduce FromPointerCast<>(), with defined sign/zero-extension behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some of the new Linux/Android tests were failing in 32-bit code where pointers were being casted via reinterpret_cast<>() to LinuxVMAddress, an unsigned 64-bit type. The behavior of such casts is implementation-defined, and in this case, sign-extension was being used to convert the 32-bit pointers to 64 bits, resulting in very large (unsigned) LinuxVMAddress values that could not possibly refer to proper addresses in a 32-bit process’ address space. The offending reinterpret_cast<>() conversions have been replaced with the new FromPointerCast<>(), which is careful to do sign-extension when converting to a signed type, and zero-extension when converting to an unsigned type like LinuxVMAddress. Bug: crashpad:30 Test: crashpad_util_test FromPointerCast*:MemoryMap.*:ProcessMemory.* Change-Id: I6f1408dc63369a8740ecd6015d657e4407a7c271 Reviewed-on: https://chromium-review.googlesource.com/488264 Reviewed-by: Joshua Peraza --- util/linux/memory_map_test.cc | 20 ++- util/linux/process_memory_test.cc | 7 +- util/misc/from_pointer_cast.h | 91 +++++++++++++ util/misc/from_pointer_cast_test.cc | 190 ++++++++++++++++++++++++++++ util/posix/scoped_mmap.h | 4 +- util/util.gyp | 1 + util/util_test.gyp | 1 + 7 files changed, 303 insertions(+), 11 deletions(-) create mode 100644 util/misc/from_pointer_cast.h create mode 100644 util/misc/from_pointer_cast_test.cc diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index e78cb83a..9258f83e 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -21,6 +21,7 @@ #include #include "base/files/file_path.h" +#include "base/strings/stringprintf.h" #include "gtest/gtest.h" #include "test/errors.h" #include "test/file.h" @@ -28,6 +29,7 @@ #include "test/scoped_temp_dir.h" #include "util/file/file_io.h" #include "util/misc/clock.h" +#include "util/misc/from_pointer_cast.h" #include "util/posix/scoped_mmap.h" namespace crashpad { @@ -45,7 +47,7 @@ TEST(MemoryMap, SelfBasic) { MemoryMap map; ASSERT_TRUE(map.Initialize(getpid())); - auto stack_address = reinterpret_cast(&map); + auto stack_address = FromPointerCast(&map); const MemoryMap::Mapping* mapping = map.FindMapping(stack_address); ASSERT_TRUE(mapping); EXPECT_GE(stack_address, mapping->range.Base()); @@ -53,7 +55,7 @@ TEST(MemoryMap, SelfBasic) { EXPECT_TRUE(mapping->readable); EXPECT_TRUE(mapping->writable); - auto code_address = reinterpret_cast(getpid); + auto code_address = FromPointerCast(getpid); mapping = map.FindMapping(code_address); ASSERT_TRUE(mapping); EXPECT_GE(code_address, mapping->range.Base()); @@ -146,10 +148,10 @@ class MapChildTest : public Multiprocess { } void MultiprocessChild() override { - auto code_address = reinterpret_cast(getpid); + auto code_address = FromPointerCast(getpid); CheckedWriteFile(WritePipeHandle(), &code_address, sizeof(code_address)); - auto stack_address = reinterpret_cast(&code_address); + auto stack_address = FromPointerCast(&code_address); CheckedWriteFile(WritePipeHandle(), &stack_address, sizeof(stack_address)); ScopedMmap mapping; @@ -227,6 +229,8 @@ void ExpectMappings(const MemoryMap& map, size_t num_mappings, size_t mapping_size) { for (size_t index = 0; index < num_mappings; ++index) { + SCOPED_TRACE(base::StringPrintf("index %zu", index)); + auto mapping_address = region_addr + index * mapping_size; const MemoryMap::Mapping* mapping = map.FindMapping(mapping_address); ASSERT_TRUE(mapping); @@ -269,10 +273,12 @@ class MapRunningChildTest : public Multiprocess { LinuxVMAddress region_addr; CheckedReadFileExactly(ReadPipeHandle(), ®ion_addr, sizeof(region_addr)); - // Let the child get back to its work - SleepNanoseconds(1000); - for (int iter = 0; iter < 8; ++iter) { + SCOPED_TRACE(base::StringPrintf("iter %d", iter)); + + // Let the child get back to its work + SleepNanoseconds(1000); + MemoryMap map; ASSERT_TRUE(map.Initialize(ChildPID())); diff --git a/util/linux/process_memory_test.cc b/util/linux/process_memory_test.cc index e00134a7..21377553 100644 --- a/util/linux/process_memory_test.cc +++ b/util/linux/process_memory_test.cc @@ -24,6 +24,7 @@ #include "test/errors.h" #include "test/multiprocess.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/posix/scoped_mmap.h" namespace crashpad { @@ -66,7 +67,7 @@ class ReadTest : public TargetProcessTest { ProcessMemory memory; ASSERT_TRUE(memory.Initialize(pid)); - LinuxVMAddress address = reinterpret_cast(region_.get()); + LinuxVMAddress address = FromPointerCast(region_.get()); std::unique_ptr result(new char[region_size_]); // Ensure that the entire region can be read. @@ -123,7 +124,7 @@ TEST(ProcessMemory, ReadForked) { bool ReadCString(const ProcessMemory& memory, const char* pointer, std::string* result) { - return memory.ReadCString(reinterpret_cast(pointer), result); + return memory.ReadCString(FromPointerCast(pointer), result); } bool ReadCStringSizeLimited(const ProcessMemory& memory, @@ -131,7 +132,7 @@ bool ReadCStringSizeLimited(const ProcessMemory& memory, size_t size, std::string* result) { return memory.ReadCStringSizeLimited( - reinterpret_cast(pointer), size, result); + FromPointerCast(pointer), size, result); } const char kConstCharEmpty[] = ""; diff --git a/util/misc/from_pointer_cast.h b/util/misc/from_pointer_cast.h new file mode 100644 index 00000000..3f42447d --- /dev/null +++ b/util/misc/from_pointer_cast.h @@ -0,0 +1,91 @@ +// 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_MISC_FROM_POINTER_CAST_H_ +#define CRASHPAD_UTIL_MISC_FROM_POINTER_CAST_H_ + +#include + +#include +#include + +namespace crashpad { + +#if DOXYGEN + +//! \brief Casts from a pointer type to an integer. +//! +//! Compared to `reinterpret_cast<>()`, FromPointerCast<>() defines whether a +//! pointer type is sign-extended or zero-extended. Casts to signed integral +//! types are sign-extended. Casts to unsigned integral types are zero-extended. +template +FromPointerCast(const From from) { + return reinterpret_cast(from); +} + +#else // DOXYGEN + +// Cast std::nullptr_t to any pointer type. +// +// In C++14, the nullptr_t check could use std::is_null_pointer::type +// instead of the is_same::type, nullptr_t>::type construct. +template +typename std::enable_if< + std::is_same::type, std::nullptr_t>::value && + std::is_pointer::value, + To>::type +FromPointerCast(const From& from) { + return static_cast(from); +} + +// Cast std::nullptr_t to any integral type. +// +// In C++14, the nullptr_t check could use std::is_null_pointer::type +// instead of the is_same::type, nullptr_t>::type construct. +template +typename std::enable_if< + std::is_same::type, std::nullptr_t>::value && + std::is_integral::value, + To>::type +FromPointerCast(const From& from) { + return reinterpret_cast(from); +} + +// Cast a pointer to any other pointer type. +template +typename std::enable_if::value && + std::is_pointer::value, + To>::type +FromPointerCast(const From from) { + return reinterpret_cast(from); +} + +// Cast a pointer to an integral type. Sign-extend when casting to a signed +// type, zero-extend when casting to an unsigned type. +template +typename std::enable_if::value && + std::is_integral::value, + To>::type +FromPointerCast(const From from) { + return static_cast( + reinterpret_cast::value, + intptr_t, + uintptr_t>::type>(from)); +} + +#endif // DOXYGEN + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_MISC_FROM_POINTER_CAST_H_ diff --git a/util/misc/from_pointer_cast_test.cc b/util/misc/from_pointer_cast_test.cc new file mode 100644 index 00000000..0c90c275 --- /dev/null +++ b/util/misc/from_pointer_cast_test.cc @@ -0,0 +1,190 @@ +// 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/misc/from_pointer_cast.h" + +#include +#include + +#include + +#include "build/build_config.h" +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +struct SomeType {}; + +template +class FromPointerCastTest : public testing::Test {}; + +using FromPointerCastTestTypes = testing::Types; + +TYPED_TEST_CASE(FromPointerCastTest, FromPointerCastTestTypes); + +TYPED_TEST(FromPointerCastTest, ToSigned) { + EXPECT_EQ(FromPointerCast(nullptr), 0); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), -1); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::max())), + static_cast(std::numeric_limits::max())); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::min())), + std::numeric_limits::min()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::max())), + std::numeric_limits::max()); +} + +TYPED_TEST(FromPointerCastTest, ToUnsigned) { + EXPECT_EQ(FromPointerCast(nullptr), 0u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), + static_cast(-1)); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::max())), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::min())), + static_cast(std::numeric_limits::min())); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + std::numeric_limits::max())), + static_cast(std::numeric_limits::max())); +} + +// MatchCV::Type adapts YourType to match the +// const/void qualification of CVQualifiedType. +template +struct MatchCV { + private: + using NonCVBase = typename std::remove_cv::type; + + public: + using Type = typename std::conditional< + std::is_const::value, + typename std::conditional::value, + const volatile NonCVBase, + const NonCVBase>::type, + typename std::conditional::value, + volatile NonCVBase, + NonCVBase>::type>::type; +}; + +#if defined(COMPILER_MSVC) && _MSC_VER < 1910 +// gtest under MSVS 2015 (MSC 19.0) doesn’t handle EXPECT_EQ(a, b) when a or b +// is a pointer to a volatile type, because it can’t figure out how to print +// them. +template +typename std::remove_volatile::type>::type* +MaybeRemoveVolatile(const T& value) { + return const_cast::type>::type*>(value); +} +#else // COMPILER_MSVC && _MSC_VER < 1910 +// This isn’t a problem in MSVS 2017 (MSC 19.1) or with other compilers. +template +T MaybeRemoveVolatile(const T& value) { + return value; +} +#endif // COMPILER_MSVC && _MSC_VER < 1910 + +TYPED_TEST(FromPointerCastTest, ToPointer) { + using CVSomeType = + typename MatchCV::type>::Type; + + EXPECT_EQ(MaybeRemoveVolatile(FromPointerCast(nullptr)), + MaybeRemoveVolatile(static_cast(nullptr))); + EXPECT_EQ(MaybeRemoveVolatile( + FromPointerCast(reinterpret_cast(1))), + MaybeRemoveVolatile(reinterpret_cast(1))); + EXPECT_EQ(MaybeRemoveVolatile( + FromPointerCast(reinterpret_cast(-1))), + MaybeRemoveVolatile(reinterpret_cast(-1))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast( + reinterpret_cast(std::numeric_limits::max()))), + MaybeRemoveVolatile(reinterpret_cast( + std::numeric_limits::max()))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast( + reinterpret_cast(std::numeric_limits::min()))), + MaybeRemoveVolatile( + reinterpret_cast(std::numeric_limits::min()))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast( + reinterpret_cast(std::numeric_limits::max()))), + MaybeRemoveVolatile( + reinterpret_cast(std::numeric_limits::max()))); +} + +TEST(FromPointerCast, FromFunctionPointer) { + // These casts should work with or without the & in &malloc. + + EXPECT_NE(FromPointerCast(malloc), 0); + EXPECT_NE(FromPointerCast(&malloc), 0); + + EXPECT_NE(FromPointerCast(malloc), 0u); + EXPECT_NE(FromPointerCast(&malloc), 0u); + + EXPECT_EQ(FromPointerCast(malloc), reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(&malloc), reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(&malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(MaybeRemoveVolatile(FromPointerCast(malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ(MaybeRemoveVolatile(FromPointerCast(&malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast(malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast(&malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + + EXPECT_EQ(FromPointerCast(malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(&malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(FromPointerCast(&malloc), + reinterpret_cast(malloc)); + EXPECT_EQ(MaybeRemoveVolatile(FromPointerCast(malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ(MaybeRemoveVolatile(FromPointerCast(&malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast(malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); + EXPECT_EQ( + MaybeRemoveVolatile(FromPointerCast(&malloc)), + MaybeRemoveVolatile(reinterpret_cast(malloc))); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/posix/scoped_mmap.h b/util/posix/scoped_mmap.h index 5b216dee..b0ff3dc0 100644 --- a/util/posix/scoped_mmap.h +++ b/util/posix/scoped_mmap.h @@ -20,6 +20,8 @@ #include #include +#include "util/misc/from_pointer_cast.h" + namespace crashpad { //! \brief Maintains a memory-mapped region created by `mmap()`. @@ -85,7 +87,7 @@ class ScopedMmap { //! a type of the caller’s choosing. template T addr_as() const { - return reinterpret_cast(addr_); + return FromPointerCast(addr_); } //! \brief Returns the size of the memory-mapped region. diff --git a/util/util.gyp b/util/util.gyp index 5a7947a1..e3074e9a 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -101,6 +101,7 @@ 'misc/clock_mac.cc', 'misc/clock_posix.cc', 'misc/clock_win.cc', + 'misc/from_pointer_cast.h', 'misc/implicit_cast.h', 'misc/initialization_state.h', 'misc/initialization_state_dcheck.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 6ee35258..db54a9b0 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -63,6 +63,7 @@ 'mach/task_memory_test.cc', 'misc/arraysize_unsafe_test.cc', 'misc/clock_test.cc', + 'misc/from_pointer_cast_test.cc', 'misc/initialization_state_dcheck_test.cc', 'misc/initialization_state_test.cc', 'misc/paths_test.cc', From 15103742e06e415cf8b88d1726dc321108fed4be Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 28 Apr 2017 10:08:35 -0400 Subject: [PATCH 08/32] Use FromPointerCast<>() in many places where it makes sense I opted to leave casts to types that were definitely the same size alone. reinterpret_cast(pointer) and reinterpret_cast(pointer) should always be safe, for example. Casts to other integral types have been replaced with FromPointerCast<>(), which does zero-extension or sign-extension based on the target type. To make it possible to use FromPointerCast<>() with some use sites that were already using checked_cast<>(), FromPointerCast<>() now uses check_cast<>() when converting to a narrower type. Test: crashpad_util_test FromPointerCast*, others Change-Id: I4a71b4aa2d87f545c75524290a702f5f3138d675 Reviewed-on: https://chromium-review.googlesource.com/489701 Reviewed-by: Scott Graham --- client/crashpad_client_win.cc | 29 ++++---- client/crashpad_info.cc | 8 +-- client/simple_address_range_bag.h | 12 ++-- snapshot/api/module_annotations_win.cc | 3 +- snapshot/mac/mach_o_image_reader_test.cc | 13 ++-- snapshot/mac/process_reader_test.cc | 20 +++--- snapshot/mac/process_types_test.cc | 3 +- .../crashpad_snapshot_test_crashing_child.cc | 3 +- ...pad_snapshot_test_dump_without_crashing.cc | 3 +- snapshot/win/pe_image_reader.cc | 5 +- snapshot/win/pe_image_reader_test.cc | 5 +- snapshot/win/process_reader_win_test.cc | 9 ++- snapshot/win/process_snapshot_win.cc | 3 +- util/mach/task_memory_test.cc | 11 +-- util/misc/from_pointer_cast.h | 52 +++++++------- util/misc/from_pointer_cast_test.cc | 71 +++++++++++++++++++ util/win/handle.cc | 3 +- util/win/process_info.cc | 3 +- util/win/process_info_test.cc | 5 +- 19 files changed, 173 insertions(+), 88 deletions(-) diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 1a183c8a..487b39ab 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -30,6 +30,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/synchronization/lock.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/random_string.h" #include "util/win/address_types.h" #include "util/win/capture_context.h" @@ -156,7 +157,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { // signal the crash handler. g_crash_exception_information.thread_id = GetCurrentThreadId(); g_crash_exception_information.exception_pointers = - reinterpret_cast(exception_pointers); + FromPointerCast(exception_pointers); // Now signal the crash server, which will take a dump and then terminate us // when it's complete. @@ -390,9 +391,9 @@ bool StartHandlerProcess( g_non_crash_dump_done, data->ipc_pipe_handle.get(), this_process.get(), - reinterpret_cast(&g_crash_exception_information), - reinterpret_cast(&g_non_crash_exception_information), - reinterpret_cast(&g_critical_section_with_debug_info)); + FromPointerCast(&g_crash_exception_information), + FromPointerCast(&g_non_crash_exception_information), + FromPointerCast(&g_critical_section_with_debug_info)); AppendCommandLineArgument( base::UTF8ToUTF16(std::string("--initial-client-data=") + initial_client_data.StringRepresentation()), @@ -655,14 +656,14 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) { message.registration.version = RegistrationRequest::kMessageVersion; message.registration.client_process_id = GetCurrentProcessId(); message.registration.crash_exception_information = - reinterpret_cast(&g_crash_exception_information); + FromPointerCast(&g_crash_exception_information); message.registration.non_crash_exception_information = - reinterpret_cast(&g_non_crash_exception_information); + FromPointerCast(&g_non_crash_exception_information); CommonInProcessInitialization(); message.registration.critical_section_address = - reinterpret_cast(&g_critical_section_with_debug_info); + FromPointerCast(&g_critical_section_with_debug_info); ServerToClientMessage response = {}; @@ -765,7 +766,7 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) { g_non_crash_exception_information.thread_id = GetCurrentThreadId(); g_non_crash_exception_information.exception_pointers = - reinterpret_cast(&exception_pointers); + FromPointerCast(&exception_pointers); bool set_event_result = !!SetEvent(g_signal_non_crash_dump); PLOG_IF(ERROR, !set_event_result) << "SetEvent"; @@ -830,11 +831,11 @@ bool CrashpadClient::DumpAndCrashTargetProcess(HANDLE process, const size_t kInjectBufferSize = 4 * 1024; WinVMAddress inject_memory = - reinterpret_cast(VirtualAllocEx(process, - nullptr, - kInjectBufferSize, - MEM_RESERVE | MEM_COMMIT, - PAGE_READWRITE)); + FromPointerCast(VirtualAllocEx(process, + nullptr, + kInjectBufferSize, + MEM_RESERVE | MEM_COMMIT, + PAGE_READWRITE)); if (!inject_memory) { PLOG(ERROR) << "VirtualAllocEx"; return false; @@ -844,7 +845,7 @@ bool CrashpadClient::DumpAndCrashTargetProcess(HANDLE process, // loaded at the same address in our process as the target, and just look up // its address here. WinVMAddress raise_exception_address = - reinterpret_cast(&RaiseException); + FromPointerCast(&RaiseException); WinVMAddress code_entry_point = 0; std::vector data_to_write; diff --git a/client/crashpad_info.cc b/client/crashpad_info.cc index e517f7b1..b7ba5490 100644 --- a/client/crashpad_info.cc +++ b/client/crashpad_info.cc @@ -15,6 +15,7 @@ #include "client/crashpad_info.h" #include "util/misc/address_sanitizer.h" +#include "util/misc/from_pointer_cast.h" #include "util/stdlib/cxx.h" #if defined(OS_MACOSX) @@ -131,11 +132,10 @@ void CrashpadInfo::AddUserDataMinidumpStream(uint32_t stream_type, const void* data, size_t size) { auto to_be_added = new internal::UserDataMinidumpStreamListEntry(); - to_be_added->next = base::checked_cast( - reinterpret_cast(user_data_minidump_stream_head_)); + to_be_added->next = + FromPointerCast(user_data_minidump_stream_head_); to_be_added->stream_type = stream_type; - to_be_added->base_address = - base::checked_cast(reinterpret_cast(data)); + to_be_added->base_address = FromPointerCast(data); to_be_added->size = base::checked_cast(size); user_data_minidump_stream_head_ = to_be_added; } diff --git a/client/simple_address_range_bag.h b/client/simple_address_range_bag.h index 336a748d..a09ae6e8 100644 --- a/client/simple_address_range_bag.h +++ b/client/simple_address_range_bag.h @@ -19,6 +19,8 @@ #include "base/logging.h" #include "base/macros.h" +#include "base/numerics/safe_conversions.h" +#include "util/misc/from_pointer_cast.h" #include "util/numeric/checked_range.h" namespace crashpad { @@ -138,9 +140,8 @@ class TSimpleAddressRangeBag { bool Insert(void* base, size_t size) { DCHECK(base != nullptr); DCHECK_NE(0u, size); - return Insert(CheckedRange( - base::checked_cast(reinterpret_cast(base)), - base::checked_cast(size))); + return Insert(CheckedRange(FromPointerCast(base), + base::checked_cast(size))); } //! \brief Removes the given range from the bag. @@ -175,9 +176,8 @@ class TSimpleAddressRangeBag { bool Remove(void* base, size_t size) { DCHECK(base != nullptr); DCHECK_NE(0u, size); - return Remove(CheckedRange( - base::checked_cast(reinterpret_cast(base)), - base::checked_cast(size))); + return Remove(CheckedRange(FromPointerCast(base), + base::checked_cast(size))); } diff --git a/snapshot/api/module_annotations_win.cc b/snapshot/api/module_annotations_win.cc index 8f444fa5..fd468703 100644 --- a/snapshot/api/module_annotations_win.cc +++ b/snapshot/api/module_annotations_win.cc @@ -17,6 +17,7 @@ #include "snapshot/win/pe_image_annotations_reader.h" #include "snapshot/win/pe_image_reader.h" #include "snapshot/win/process_reader_win.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/get_module_information.h" namespace crashpad { @@ -38,7 +39,7 @@ bool ReadModuleAnnotations(HANDLE process, PEImageReader image_reader; if (!image_reader.Initialize( &process_reader, - reinterpret_cast(module_info.lpBaseOfDll), + FromPointerCast(module_info.lpBaseOfDll), module_info.SizeOfImage, "")) return false; diff --git a/snapshot/mac/mach_o_image_reader_test.cc b/snapshot/mac/mach_o_image_reader_test.cc index cccf24fa..6c545bc1 100644 --- a/snapshot/mac/mach_o_image_reader_test.cc +++ b/snapshot/mac/mach_o_image_reader_test.cc @@ -32,6 +32,7 @@ #include "snapshot/mac/process_reader.h" #include "snapshot/mac/process_types.h" #include "test/mac/dyld.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/implicit_cast.h" #include "util/misc/uuid.h" @@ -133,7 +134,7 @@ void ExpectSegmentCommand(const SegmentCommand* expect_segment, const uint8_t* expect_segment_data = getsegmentdata( expect_image, segment_name.c_str(), &expect_segment_size); mach_vm_address_t expect_segment_address = - reinterpret_cast(expect_segment_data); + FromPointerCast(expect_segment_data); EXPECT_EQ(actual_segment->Address(), expect_segment_address); EXPECT_EQ(actual_segment->vmsize(), expect_segment_size); EXPECT_EQ(actual_segment->Size(), actual_segment->vmsize()); @@ -191,7 +192,7 @@ void ExpectSegmentCommand(const SegmentCommand* expect_segment, section_name.c_str(), &expect_section_size); mach_vm_address_t expect_section_address = - reinterpret_cast(expect_section_data); + FromPointerCast(expect_section_data); EXPECT_EQ(actual_section_address, expect_section_address); EXPECT_EQ(actual_section->size, expect_section_size); } else { @@ -501,7 +502,7 @@ TEST(MachOImageReader, Self_MainExecutable) { reinterpret_cast(dlsym(RTLD_MAIN_ONLY, MH_EXECUTE_SYM)); ASSERT_NE(mh_execute_header, nullptr); mach_vm_address_t mh_execute_header_address = - reinterpret_cast(mh_execute_header); + FromPointerCast(mh_execute_header); MachOImageReader image_reader; ASSERT_TRUE(image_reader.Initialize( @@ -547,7 +548,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast(_dyld_get_image_header(index)); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( @@ -588,7 +589,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast( dyld_image_infos->dyldImageLoadAddress); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( @@ -619,7 +620,7 @@ TEST(MachOImageReader, Self_DyldImages) { const MachHeader* mach_header = reinterpret_cast(dyld_image->imageLoadAddress); mach_vm_address_t image_address = - reinterpret_cast(mach_header); + FromPointerCast(mach_header); MachOImageReader image_reader; ASSERT_TRUE( diff --git a/snapshot/mac/process_reader_test.cc b/snapshot/mac/process_reader_test.cc index 26cb26d2..3554326c 100644 --- a/snapshot/mac/process_reader_test.cc +++ b/snapshot/mac/process_reader_test.cc @@ -40,6 +40,7 @@ #include "util/file/file_io.h" #include "util/mac/mac_util.h" #include "util/mach/mach_extensions.h" +#include "util/misc/from_pointer_cast.h" #include "util/stdlib/pointer_container.h" #include "util/synchronization/semaphore.h" @@ -74,7 +75,7 @@ TEST(ProcessReader, SelfBasic) { const char kTestMemory[] = "Some test memory"; char buffer[arraysize(kTestMemory)]; ASSERT_TRUE(process_reader.Memory()->Read( - reinterpret_cast(kTestMemory), + FromPointerCast(kTestMemory), sizeof(kTestMemory), &buffer)); EXPECT_STREQ(kTestMemory, buffer); @@ -115,8 +116,7 @@ class ProcessReaderChild final : public MachMultiprocess { void MachMultiprocessChild() override { FileHandle write_handle = WritePipeHandle(); - mach_vm_address_t address = - reinterpret_cast(kTestMemory); + mach_vm_address_t address = FromPointerCast(kTestMemory); CheckedWriteFile(write_handle, &address, sizeof(address)); // Wait for the parent to signal that it’s OK to exit by closing its end of @@ -282,7 +282,7 @@ class TestThreadPool { ThreadInfo* thread_info = static_cast(argument); thread_info->stack_address = - reinterpret_cast(&thread_info); + FromPointerCast(&thread_info); thread_info->ready_semaphore.Signal(); thread_info->exit_semaphore.Wait(); @@ -391,7 +391,7 @@ TEST(ProcessReader, SelfSeveralThreads) { ThreadMap thread_map; const uint64_t self_thread_id = PthreadToThreadID(pthread_self()); TestThreadPool::ThreadExpectation expectation; - expectation.stack_address = reinterpret_cast(&thread_map); + expectation.stack_address = FromPointerCast(&thread_map); expectation.suspend_count = 0; thread_map[self_thread_id] = expectation; for (size_t thread_index = 0; thread_index < kChildThreads; ++thread_index) { @@ -484,7 +484,7 @@ class ProcessReaderThreadedChild final : public MachMultiprocess { CheckedWriteFile(write_handle, &thread_id, sizeof(thread_id)); TestThreadPool::ThreadExpectation expectation; - expectation.stack_address = reinterpret_cast(&thread_id); + expectation.stack_address = FromPointerCast(&thread_id); expectation.suspend_count = 0; CheckedWriteFile(write_handle, @@ -667,7 +667,7 @@ TEST(ProcessReader, SelfModules) { ASSERT_TRUE(modules[index].reader); EXPECT_EQ( modules[index].reader->Address(), - reinterpret_cast(_dyld_get_image_header(index))); + FromPointerCast(_dyld_get_image_header(index))); if (index == 0) { // dyld didn’t load the main executable, so it couldn’t record its @@ -703,7 +703,7 @@ TEST(ProcessReader, SelfModules) { if (dyld_image_infos->version >= 2) { ASSERT_TRUE(modules[index].reader); EXPECT_EQ(modules[index].reader->Address(), - reinterpret_cast( + FromPointerCast( dyld_image_infos->dyldImageLoadAddress)); } } @@ -801,10 +801,10 @@ class ProcessReaderModulesChild final : public MachMultiprocess { if (index < dyld_image_count) { dyld_image_name = _dyld_get_image_name(index); dyld_image_address = - reinterpret_cast(_dyld_get_image_header(index)); + FromPointerCast(_dyld_get_image_header(index)); } else { dyld_image_name = kDyldPath; - dyld_image_address = reinterpret_cast( + dyld_image_address = FromPointerCast( dyld_image_infos->dyldImageLoadAddress); } diff --git a/snapshot/mac/process_types_test.cc b/snapshot/mac/process_types_test.cc index f9ff7d19..18595f02 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -27,6 +27,7 @@ #include "snapshot/mac/process_types/internal.h" #include "test/mac/dyld.h" #include "util/mac/mac_util.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/implicit_cast.h" namespace crashpad { @@ -81,7 +82,7 @@ TEST(ProcessTypes, DyldImagesSelf) { ASSERT_EQ(kr, KERN_SUCCESS); EXPECT_EQ(dyld_info.all_image_info_addr, - reinterpret_cast(self_image_infos)); + FromPointerCast(self_image_infos)); EXPECT_GT(dyld_info.all_image_info_size, 1u); // This field is only present in the OS X 10.7 SDK (at build time) and kernel diff --git a/snapshot/win/crashpad_snapshot_test_crashing_child.cc b/snapshot/win/crashpad_snapshot_test_crashing_child.cc index b0caa065..146c66a1 100644 --- a/snapshot/win/crashpad_snapshot_test_crashing_child.cc +++ b/snapshot/win/crashpad_snapshot_test_crashing_child.cc @@ -19,12 +19,13 @@ #include "base/logging.h" #include "client/crashpad_client.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/address_types.h" namespace { __declspec(noinline) crashpad::WinVMAddress CurrentAddress() { - return reinterpret_cast(_ReturnAddress()); + return crashpad::FromPointerCast(_ReturnAddress()); } } // namespace diff --git a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc index a9fbdcf5..f55f503b 100644 --- a/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc +++ b/snapshot/win/crashpad_snapshot_test_dump_without_crashing.cc @@ -19,12 +19,13 @@ #include "client/crashpad_client.h" #include "client/simulate_crash.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/address_types.h" namespace { __declspec(noinline) crashpad::WinVMAddress CurrentAddress() { - return reinterpret_cast(_ReturnAddress()); + return crashpad::FromPointerCast(_ReturnAddress()); } } // namespace diff --git a/snapshot/win/pe_image_reader.cc b/snapshot/win/pe_image_reader.cc index 342021a6..824f525b 100644 --- a/snapshot/win/pe_image_reader.cc +++ b/snapshot/win/pe_image_reader.cc @@ -22,6 +22,7 @@ #include "base/logging.h" #include "client/crashpad_info.h" #include "snapshot/win/pe_image_resource_reader.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/pdb_structures.h" #include "util/win/process_structs.h" @@ -202,10 +203,8 @@ bool PEImageReader::VSFixedFileInfo( WinVMAddress address; WinVMSize size; - const uint16_t vs_file_info_type = static_cast( - reinterpret_cast(VS_FILE_INFO)); // RT_VERSION if (!resource_reader.FindResourceByID( - vs_file_info_type, + FromPointerCast(VS_FILE_INFO), // RT_VERSION VS_VERSION_INFO, MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL), &address, diff --git a/snapshot/win/pe_image_reader_test.cc b/snapshot/win/pe_image_reader_test.cc index 2468c140..041d72a2 100644 --- a/snapshot/win/pe_image_reader_test.cc +++ b/snapshot/win/pe_image_reader_test.cc @@ -22,6 +22,7 @@ #include "gtest/gtest.h" #include "snapshot/win/process_reader_win.h" #include "test/errors.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/get_module_information.h" #include "util/win/module_version.h" #include "util/win/process_info.h" @@ -44,7 +45,7 @@ TEST(PEImageReader, DebugDirectory) { << ErrorMessage("GetModuleInformation"); EXPECT_EQ(module_info.lpBaseOfDll, self); ASSERT_TRUE(pe_image_reader.Initialize(&process_reader, - reinterpret_cast(self), + FromPointerCast(self), module_info.SizeOfImage, "self")); UUID uuid; @@ -139,7 +140,7 @@ TEST(PEImageReader, VSFixedFileInfo_OneModule) { ProcessInfo::Module module; module.name = kModuleName; - module.dll_base = reinterpret_cast(module_info.lpBaseOfDll); + module.dll_base = FromPointerCast(module_info.lpBaseOfDll); module.size = module_info.SizeOfImage; TestVSFixedFileInfo(&process_reader, module, true); diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index 5b4b7d0a..87e72a94 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "test/win/win_multiprocess.h" +#include "util/misc/from_pointer_cast.h" #include "util/synchronization/semaphore.h" #include "util/thread/thread.h" #include "util/win/scoped_process_suspend.h" @@ -42,10 +43,8 @@ TEST(ProcessReaderWin, SelfBasic) { const char kTestMemory[] = "Some test memory"; char buffer[arraysize(kTestMemory)]; - ASSERT_TRUE( - process_reader.ReadMemory(reinterpret_cast(kTestMemory), - sizeof(kTestMemory), - &buffer)); + ASSERT_TRUE(process_reader.ReadMemory( + reinterpret_cast(kTestMemory), sizeof(kTestMemory), &buffer)); EXPECT_STREQ(kTestMemory, buffer); } @@ -78,7 +77,7 @@ class ProcessReaderChild final : public WinMultiprocess { } void WinMultiprocessChild() override { - WinVMAddress address = reinterpret_cast(kTestMemory); + WinVMAddress address = FromPointerCast(kTestMemory); CheckedWriteFile(WritePipeHandle(), &address, sizeof(address)); // Wait for the parent to signal that it's OK to exit by closing its end of diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index 11df2b80..f580fd09 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -26,6 +26,7 @@ #include "snapshot/win/exception_snapshot_win.h" #include "snapshot/win/memory_snapshot_win.h" #include "snapshot/win/module_snapshot_win.h" +#include "util/misc/from_pointer_cast.h" #include "util/win/nt_internals.h" #include "util/win/registration_protocol_win.h" #include "util/win/time.h" @@ -295,7 +296,7 @@ void ProcessSnapshotWin::InitializeUnloadedModules() { } const WinVMAddress address_in_target_process = - reinterpret_cast(event_trace_address); + FromPointerCast(event_trace_address); Traits::Pointer pointer_to_array; if (!process_reader_.ReadMemory(address_in_target_process, diff --git a/util/mach/task_memory_test.cc b/util/mach/task_memory_test.cc index 4360d680..514f48f1 100644 --- a/util/mach/task_memory_test.cc +++ b/util/mach/task_memory_test.cc @@ -25,6 +25,7 @@ #include "base/mac/scoped_mach_vm.h" #include "gtest/gtest.h" #include "test/mac/mach_errors.h" +#include "util/misc/from_pointer_cast.h" namespace crashpad { namespace test { @@ -165,7 +166,7 @@ TEST(TaskMemory, ReadSelfUnmapped) { bool ReadCStringSelf(TaskMemory* memory, const char* pointer, std::string* result) { - return memory->ReadCString(reinterpret_cast(pointer), + return memory->ReadCString(FromPointerCast(pointer), result); } @@ -272,7 +273,7 @@ bool ReadCStringSizeLimitedSelf(TaskMemory* memory, size_t size, std::string* result) { return memory->ReadCStringSizeLimited( - reinterpret_cast(pointer), size, result); + FromPointerCast(pointer), size, result); } TEST(TaskMemory, ReadCStringSizeLimited_ConstCharEmpty) { @@ -462,7 +463,7 @@ TEST(TaskMemory, MappedMemoryDeallocates) { static const char kTestBuffer[] = "hello!"; mach_vm_address_t test_address = - reinterpret_cast(&kTestBuffer); + FromPointerCast(&kTestBuffer); ASSERT_TRUE((mapped = memory.ReadMapped(test_address, sizeof(kTestBuffer)))); EXPECT_EQ(memcmp(kTestBuffer, mapped->data(), sizeof(kTestBuffer)), 0); @@ -477,7 +478,7 @@ TEST(TaskMemory, MappedMemoryDeallocates) { // deallocated. const size_t kBigSize = 4 * PAGE_SIZE; std::unique_ptr big_buffer(new char[kBigSize]); - test_address = reinterpret_cast(&big_buffer[0]); + test_address = FromPointerCast(&big_buffer[0]); ASSERT_TRUE((mapped = memory.ReadMapped(test_address, kBigSize))); mapped_address = reinterpret_cast(mapped->data()); @@ -499,7 +500,7 @@ TEST(TaskMemory, MappedMemoryReadCString) { static const char kTestBuffer[] = "0\0" "2\0" "45\0" "789"; const mach_vm_address_t kTestAddress = - reinterpret_cast(&kTestBuffer); + FromPointerCast(&kTestBuffer); ASSERT_TRUE((mapped = memory.ReadMapped(kTestAddress, 10))); std::string string; diff --git a/util/misc/from_pointer_cast.h b/util/misc/from_pointer_cast.h index 3f42447d..f1b37349 100644 --- a/util/misc/from_pointer_cast.h +++ b/util/misc/from_pointer_cast.h @@ -20,6 +20,8 @@ #include #include +#include "base/numerics/safe_conversions.h" + namespace crashpad { #if DOXYGEN @@ -29,37 +31,29 @@ namespace crashpad { //! Compared to `reinterpret_cast<>()`, FromPointerCast<>() defines whether a //! pointer type is sign-extended or zero-extended. Casts to signed integral //! types are sign-extended. Casts to unsigned integral types are zero-extended. +//! +//! Use FromPointerCast<>() instead of `reinterpret_cast<>()` when casting a +//! pointer to an integral type that may not be the same width as a pointer. +//! There is no need to prefer FromPointerCast<>() when casting to an integral +//! type that’s definitely the same width as a pointer, such as `uintptr_t` and +//! `intptr_t`. template -FromPointerCast(const From from) { +FromPointerCast(From from) { return reinterpret_cast(from); } #else // DOXYGEN -// Cast std::nullptr_t to any pointer type. +// Cast std::nullptr_t to any type. // -// In C++14, the nullptr_t check could use std::is_null_pointer::type +// In C++14, the nullptr_t check could use std::is_null_pointer::value // instead of the is_same::type, nullptr_t>::type construct. template typename std::enable_if< - std::is_same::type, std::nullptr_t>::value && - std::is_pointer::value, + std::is_same::type, std::nullptr_t>::value, To>::type -FromPointerCast(const From& from) { - return static_cast(from); -} - -// Cast std::nullptr_t to any integral type. -// -// In C++14, the nullptr_t check could use std::is_null_pointer::type -// instead of the is_same::type, nullptr_t>::type construct. -template -typename std::enable_if< - std::is_same::type, std::nullptr_t>::value && - std::is_integral::value, - To>::type -FromPointerCast(const From& from) { - return reinterpret_cast(from); +FromPointerCast(From) { + return To(); } // Cast a pointer to any other pointer type. @@ -67,7 +61,7 @@ template typename std::enable_if::value && std::is_pointer::value, To>::type -FromPointerCast(const From from) { +FromPointerCast(From from) { return reinterpret_cast(from); } @@ -77,11 +71,21 @@ template typename std::enable_if::value && std::is_integral::value, To>::type -FromPointerCast(const From from) { - return static_cast( +FromPointerCast(From from) { + const auto intermediate = reinterpret_cast::value, intptr_t, - uintptr_t>::type>(from)); + uintptr_t>::type>(from); + + if (sizeof(To) >= sizeof(From)) { + // If the destination integral type is at least as wide as the source + // pointer type, use static_cast<>() and just return it. + return static_cast(intermediate); + } + + // If the destination integral type is narrower than the source pointer type, + // use checked_cast<>(). + return base::checked_cast(intermediate); } #endif // DOXYGEN diff --git a/util/misc/from_pointer_cast_test.cc b/util/misc/from_pointer_cast_test.cc index 0c90c275..b961abba 100644 --- a/util/misc/from_pointer_cast_test.cc +++ b/util/misc/from_pointer_cast_test.cc @@ -21,6 +21,7 @@ #include "build/build_config.h" #include "gtest/gtest.h" +#include "test/gtest_death_check.h" namespace crashpad { namespace test { @@ -185,6 +186,76 @@ TEST(FromPointerCast, FromFunctionPointer) { MaybeRemoveVolatile(reinterpret_cast(malloc))); } +TEST(FromPointerCast, ToNarrowInteger) { + EXPECT_EQ(FromPointerCast(nullptr), 0); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), -1); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::min()))), + std::numeric_limits::min()); + + EXPECT_EQ(FromPointerCast(nullptr), 0u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1u); + EXPECT_EQ( + FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + static_cast(std::numeric_limits::max())); + + // int and unsigned int may not be narrower than a pointer, so also test short + // and unsigned short. + + EXPECT_EQ(FromPointerCast(nullptr), 0); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1); + EXPECT_EQ(FromPointerCast(reinterpret_cast(-1)), -1); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::min()))), + std::numeric_limits::min()); + + EXPECT_EQ(FromPointerCast(nullptr), 0u); + EXPECT_EQ(FromPointerCast(reinterpret_cast(1)), 1u); + EXPECT_EQ( + FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + std::numeric_limits::max()); + EXPECT_EQ(FromPointerCast(reinterpret_cast( + static_cast(std::numeric_limits::max()))), + static_cast(std::numeric_limits::max())); +} + +TEST(FromPointerCastDeathTest, ToNarrowInteger) { + if (sizeof(int) < sizeof(void*)) { + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1ull))), + ""); + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1ull))), + ""); + } + + // int and unsigned int may not be narrower than a pointer, so also test short + // and unsigned short. + + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1u))), + ""); + EXPECT_DEATH(FromPointerCast( + reinterpret_cast(static_cast( + std::numeric_limits::max() + 1u))), + ""); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/util/win/handle.cc b/util/win/handle.cc index c53f5438..1744ecc2 100644 --- a/util/win/handle.cc +++ b/util/win/handle.cc @@ -17,6 +17,7 @@ #include #include "base/numerics/safe_conversions.h" +#include "util/misc/from_pointer_cast.h" namespace crashpad { @@ -26,7 +27,7 @@ namespace crashpad { // back to the same HANDLE value. int HandleToInt(HANDLE handle) { - return base::checked_cast(reinterpret_cast(handle)); + return FromPointerCast(handle); } HANDLE IntToHandle(int handle_int) { diff --git a/util/win/process_info.cc b/util/win/process_info.cc index f0e455db..d3871c2d 100644 --- a/util/win/process_info.cc +++ b/util/win/process_info.cc @@ -28,6 +28,7 @@ #include "base/process/memory.h" #include "base/strings/stringprintf.h" #include "build/build_config.h" +#include "util/misc/from_pointer_cast.h" #include "util/numeric/safe_assignment.h" #include "util/win/get_function.h" #include "util/win/handle.h" @@ -133,7 +134,7 @@ bool RegionIsAccessible(const MEMORY_BASIC_INFORMATION64& memory_info) { MEMORY_BASIC_INFORMATION64 MemoryBasicInformationToMemoryBasicInformation64( const MEMORY_BASIC_INFORMATION& mbi) { MEMORY_BASIC_INFORMATION64 mbi64 = {0}; - mbi64.BaseAddress = reinterpret_cast(mbi.BaseAddress); + mbi64.BaseAddress = FromPointerCast(mbi.BaseAddress); mbi64.AllocationBase = reinterpret_cast(mbi.AllocationBase); mbi64.AllocationProtect = mbi.AllocationProtect; mbi64.RegionSize = mbi.RegionSize; diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 322a987f..50f932c8 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -30,6 +30,7 @@ #include "test/test_paths.h" #include "test/win/child_launcher.h" #include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" #include "util/misc/random_string.h" #include "util/misc/uuid.h" #include "util/win/command_line.h" @@ -125,7 +126,7 @@ TEST(ProcessInfo, Self) { // Find something we know is a code address and confirm expected memory // information settings. VerifyAddressInInCodePage(process_info, - reinterpret_cast(_ReturnAddress())); + FromPointerCast(_ReturnAddress())); } void TestOtherProcess(const base::string16& directory_modification) { @@ -640,7 +641,7 @@ TEST(ProcessInfo, OutOfRangeCheck) { EXPECT_TRUE( info.LoggingRangeIsFullyReadable(CheckedRange( - reinterpret_cast(safe_memory.get()), kAllocationSize))); + FromPointerCast(safe_memory.get()), kAllocationSize))); EXPECT_FALSE(info.LoggingRangeIsFullyReadable( CheckedRange(0, 1024))); } From f03c7b2d8f5299b2689c7c945739bd2c4700c5e7 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 27 Apr 2017 19:43:00 -0400 Subject: [PATCH 09/32] mac: Trigger a real SIGSYS on 32-bit x86 during tests syscall(0) results in SIGSYS on x86_64, but not 32-bit x86. Choose a high number as a nonexistent syscall number. As of 10.12.4, the highest known system call number is 521. Test: crashpad_util_test Signals.Cause* Change-Id: I82dbd210f0c90fe933898ea0d360b431b10d090e Reviewed-on: https://chromium-review.googlesource.com/489826 Reviewed-by: Robert Sesek --- util/posix/signals_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/posix/signals_test.cc b/util/posix/signals_test.cc index 5fce38cc..75ff5580 100644 --- a/util/posix/signals_test.cc +++ b/util/posix/signals_test.cc @@ -180,7 +180,7 @@ void CauseSignal(int sig) { case SIGSYS: { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" - int rv = syscall(0); + int rv = syscall(4095); #pragma clang diagnostic pop if (rv != 0) { PLOG(ERROR) << "syscall"; From 8af3203d811c27c0e7d2f3fd96bbb41692211404 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Mon, 1 May 2017 20:40:01 -0700 Subject: [PATCH 10/32] Add LoggingReadEntireFile for reading a file into a string Change-Id: Ie07ef12131ef1d995aa78749091f3adacde75160 Reviewed-on: https://chromium-review.googlesource.com/492446 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/file/file_io.cc | 20 ++++++++++++++++++++ util/file/file_io.h | 8 ++++++++ util/posix/process_info_linux.cc | 17 ++--------------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/util/file/file_io.cc b/util/file/file_io.cc index 6cae837a..d196c9eb 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -156,6 +156,26 @@ void CheckedReadFileAtEOF(FileHandle file) { } } +bool LoggingReadEntireFile(const base::FilePath& path, std::string* contents) { + FileHandle handle = LoggingOpenFileForRead(path); + if (handle == kInvalidFileHandle) { + return false; + } + + char buffer[4096]; + FileOperationResult rv; + std::string local_contents; + while ((rv = ReadFile(handle, buffer, sizeof(buffer))) > 0) { + local_contents.append(buffer, rv); + } + if (rv < 0) { + PLOG(ERROR) << internal::kNativeReadFunctionName; + return false; + } + contents->swap(local_contents); + return true; +} + void CheckedCloseFile(FileHandle file) { CHECK(LoggingCloseFile(file)); } diff --git a/util/file/file_io.h b/util/file/file_io.h index 87c40702..c70dff30 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -17,6 +17,8 @@ #include +#include + #include "build/build_config.h" #if defined(OS_POSIX) @@ -306,6 +308,12 @@ void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); //! \sa ReadFile void CheckedReadFileAtEOF(FileHandle file); +//! brief Wraps LoggingOpenFileForRead() and ReadFile() reading the entire file +//! into \a contents. +//! +//! \return `true` on success, or `false` with a message logged. +bool LoggingReadEntireFile(const base::FilePath& path, std::string* contents); + //! \brief Wraps `open()` or `CreateFile()`, opening an existing file for //! reading. //! diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 60b7b7f8..65d96442 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -31,6 +31,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_piece.h" #include "util/file/delimited_file_reader.h" +#include "util/file/file_io.h" #include "util/file/file_reader.h" #include "util/linux/scoped_ptrace_attach.h" @@ -78,20 +79,6 @@ bool AdvancePastNumber(const char** input, T* value) { return false; } -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; @@ -391,7 +378,7 @@ bool ProcessInfo::StartTime(timeval* start_time) const { char path[32]; snprintf(path, sizeof(path), "/proc/%d/stat", pid_); std::string stat_contents; - if (!ReadEntireFile(path, &stat_contents)) { + if (!LoggingReadEntireFile(base::FilePath(path), &stat_contents)) { return false; } From 51779ce639564cf1cdd68d12565534bfbc1553a5 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 2 May 2017 10:07:21 -0700 Subject: [PATCH 11/32] linux: Make MemoryMap retry when duplicates are detected When the /proc/pid/maps file is not read atomically and the target process is actively mapping memory, entries can be read multiple times or missed entirely. This change makes MemoryMap read the whole contents of the maps file before attempting to parse it as well as check for duplication/overlap errors, retrying on failure. This change also adds ptrace attachements to unit tests to reflect actual intended usage. Bug: crashpad:30 Change-Id: Ie8549548e25c47baa418ee7439d82743f84ff41e Reviewed-on: https://chromium-review.googlesource.com/491950 Reviewed-by: Joshua Peraza Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- util/linux/memory_map.cc | 267 ++++++++++++++++++++-------------- util/linux/memory_map_test.cc | 7 + 2 files changed, 166 insertions(+), 108 deletions(-) diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc index cbf0c4e4..d1ef6c09 100644 --- a/util/linux/memory_map.cc +++ b/util/linux/memory_map.cc @@ -22,7 +22,8 @@ #include "base/logging.h" #include "build/build_config.h" #include "util/file/delimited_file_reader.h" -#include "util/file/file_reader.h" +#include "util/file/file_io.h" +#include "util/file/string_file.h" #include "util/stdlib/string_number_conversion.h" namespace crashpad { @@ -56,6 +57,134 @@ bool HexStringToNumber(const std::string& string, Type* number) { return LocalStringToNumber("0x" + string, number); } +// The result from parsing a line from the maps file. +enum class ParseResult { + // A line was successfully parsed. + kSuccess = 0, + + // The end of the file was successfully reached. + kEndOfFile, + + // There was an error in the file, likely because it was read non-atmoically. + // We should try to read it again. + kRetry, + + // An error with a message logged. + kError +}; + +// Reads a line from a maps file being read by maps_file_reader and extends +// mappings with a new MemoryMap::Mapping describing the line. +ParseResult ParseMapsLine(DelimitedFileReader* maps_file_reader, + std::vector* mappings) { + std::string field; + LinuxVMAddress start_address; + switch (maps_file_reader->GetDelim('-', &field)) { + case DelimitedFileReader::Result::kError: + return ParseResult::kError; + case DelimitedFileReader::Result::kEndOfFile: + return ParseResult::kEndOfFile; + case DelimitedFileReader::Result::kSuccess: + field.pop_back(); + if (!HexStringToNumber(field, &start_address)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (!mappings->empty() && start_address < mappings->back().range.End()) { + return ParseResult::kRetry; + } + } + + LinuxVMAddress end_address; + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &end_address))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (end_address <= start_address) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + // TODO(jperaza): set bitness properly +#if defined(ARCH_CPU_64_BITS) + const bool is_64_bit = true; +#else + const bool is_64_bit = false; +#endif + + MemoryMap::Mapping mapping; + mapping.range.SetRange(is_64_bit, start_address, end_address - start_address); + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 4)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } +#define SET_FIELD(actual_c, outval, true_chars, false_chars) \ + do { \ + if (strchr(true_chars, actual_c)) { \ + *outval = true; \ + } else if (strchr(false_chars, actual_c)) { \ + *outval = false; \ + } else { \ + LOG(ERROR) << "format error"; \ + return ParseResult::kError; \ + } \ + } while (false) + SET_FIELD(field[0], &mapping.readable, "r", "-"); + SET_FIELD(field[1], &mapping.writable, "w", "-"); + SET_FIELD(field[2], &mapping.executable, "x", "-"); + SET_FIELD(field[3], &mapping.shareable, "sS", "p"); +#undef SET_FIELD + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &mapping.offset))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + int major, minor; + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 5) || + !HexStringToNumber(field.substr(0, 2), &major) || + !HexStringToNumber(field.substr(3, 2), &minor)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + mapping.device = MKDEV(major, minor); + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !LocalStringToNumber(field, &mapping.inode))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + if (maps_file_reader->GetDelim('\n', &field) != + DelimitedFileReader::Result::kSuccess) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (field.back() != '\n') { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + field.pop_back(); + + mappings->push_back(mapping); + + size_t path_start = field.find_first_not_of(' '); + if (path_start != std::string::npos) { + mappings->back().name = field.substr(path_start); + } + return ParseResult::kSuccess; +} + } // namespace MemoryMap::Mapping::Mapping() @@ -76,121 +205,43 @@ MemoryMap::~MemoryMap() {} bool MemoryMap::Initialize(pid_t pid) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); - char path[32]; - snprintf(path, sizeof(path), "/proc/%d/maps", pid); - FileReader maps_file; - if (!maps_file.Open(base::FilePath(path))) { - return false; - } - DelimitedFileReader maps_file_reader(&maps_file); - - DelimitedFileReader::Result result; - std::string field; - while ((result = maps_file_reader.GetDelim('-', &field)) == - DelimitedFileReader::Result::kSuccess) { - field.pop_back(); - - LinuxVMAddress start_address; - if (!HexStringToNumber(field, &start_address)) { - LOG(ERROR) << "format error"; + // If the maps file is not read atomically, entries can be read multiple times + // or missed entirely. The kernel reads entries from this file into a page + // sized buffer, so maps files larger than a page require multiple reads. + // Attempt to reduce the time between reads by reading the entire file into a + // StringFile before attempting to parse it. If ParseMapsLine detects + // duplicate, overlapping, or out-of-order entries, it will trigger restarting + // the read up to |attempts| times. + int attempts = 3; + do { + std::string contents; + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/maps", pid); + if (!LoggingReadEntireFile(base::FilePath(path), &contents)) { return false; } - LinuxVMAddress end_address; - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !HexStringToNumber(field, &end_address))) { - LOG(ERROR) << "format error"; - return false; + StringFile maps_file; + maps_file.SetString(contents); + DelimitedFileReader maps_file_reader(&maps_file); + + ParseResult result; + while ((result = ParseMapsLine(&maps_file_reader, &mappings_)) == + ParseResult::kSuccess) { } - if (end_address <= start_address) { - LOG(ERROR) << "format error"; + if (result == ParseResult::kEndOfFile) { + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; + } + if (result == ParseResult::kError) { return false; } - // TODO(jperaza): set bitness properly -#if defined(ARCH_CPU_64_BITS) - const bool is_64_bit = true; -#else - const bool is_64_bit = false; -#endif + DCHECK(result == ParseResult::kRetry); + } while (--attempts > 0); - Mapping mapping; - mapping.range.SetRange( - is_64_bit, start_address, end_address - start_address); - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), field.size() != 4)) { - LOG(ERROR) << "format error"; - return false; - } -#define SET_FIELD(actual_c, outval, true_chars, false_chars) \ - do { \ - if (strchr(true_chars, actual_c)) { \ - *outval = true; \ - } else if (strchr(false_chars, actual_c)) { \ - *outval = false; \ - } else { \ - LOG(ERROR) << "format error"; \ - return false; \ - } \ - } while (false) - SET_FIELD(field[0], &mapping.readable, "r", "-"); - SET_FIELD(field[1], &mapping.writable, "w", "-"); - SET_FIELD(field[2], &mapping.executable, "x", "-"); - SET_FIELD(field[3], &mapping.shareable, "sS", "p"); -#undef SET_FIELD - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !HexStringToNumber(field, &mapping.offset))) { - LOG(ERROR) << "format error"; - return false; - } - - int major, minor; - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), field.size() != 5) || - !HexStringToNumber(field.substr(0, 2), &major) || - !HexStringToNumber(field.substr(3, 2), &minor)) { - LOG(ERROR) << "format error"; - return false; - } - mapping.device = MKDEV(major, minor); - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !LocalStringToNumber(field, &mapping.inode))) { - LOG(ERROR) << "format error"; - return false; - } - - if (maps_file_reader.GetDelim('\n', &field) != - DelimitedFileReader::Result::kSuccess) { - LOG(ERROR) << "format error"; - return false; - } - if (field.back() != '\n') { - LOG(ERROR) << "format error"; - return false; - } - field.pop_back(); - - mappings_.push_back(mapping); - - size_t path_start = field.find_first_not_of(' '); - if (path_start != std::string::npos) { - mappings_.back().name = field.substr(path_start); - } - } - if (result != DelimitedFileReader::Result::kEndOfFile) { - return false; - } - - INITIALIZATION_STATE_SET_VALID(initialized_); - return true; + LOG(ERROR) << "retry count exceeded"; + return false; } const MemoryMap::Mapping* MemoryMap::FindMapping(LinuxVMAddress address) const { diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index 9258f83e..23894b9b 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -28,6 +28,7 @@ #include "test/multiprocess.h" #include "test/scoped_temp_dir.h" #include "util/file/file_io.h" +#include "util/linux/scoped_ptrace_attach.h" #include "util/misc/clock.h" #include "util/misc/from_pointer_cast.h" #include "util/posix/scoped_mmap.h" @@ -104,6 +105,9 @@ class MapChildTest : public Multiprocess { std::string mapped_file_name(path_length, std::string::value_type()); CheckedReadFileExactly(ReadPipeHandle(), &mapped_file_name[0], path_length); + ScopedPtraceAttach attachment; + attachment.ResetAttach(ChildPID()); + MemoryMap map; ASSERT_TRUE(map.Initialize(ChildPID())); @@ -279,6 +283,9 @@ class MapRunningChildTest : public Multiprocess { // Let the child get back to its work SleepNanoseconds(1000); + ScopedPtraceAttach attachment; + attachment.ResetAttach(ChildPID()); + MemoryMap map; ASSERT_TRUE(map.Initialize(ChildPID())); From abbeffead98e57874fab3bba58906485edb89f7f Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 2 May 2017 13:52:55 -0400 Subject: [PATCH 12/32] Fix file descriptor/handle leak in LoggingReadEntireFile() 8af3203d811c introduced a resource leak. Change-Id: Ia909eef39b6b772d8808dd6f5770c06add6467bc Reviewed-on: https://chromium-review.googlesource.com/493946 Reviewed-by: Joshua Peraza --- util/file/file_io.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/util/file/file_io.cc b/util/file/file_io.cc index d196c9eb..f8a66304 100644 --- a/util/file/file_io.cc +++ b/util/file/file_io.cc @@ -157,15 +157,16 @@ void CheckedReadFileAtEOF(FileHandle file) { } bool LoggingReadEntireFile(const base::FilePath& path, std::string* contents) { - FileHandle handle = LoggingOpenFileForRead(path); - if (handle == kInvalidFileHandle) { + ScopedFileHandle handle(LoggingOpenFileForRead(path)); + if (!handle.is_valid()) { return false; } char buffer[4096]; FileOperationResult rv; std::string local_contents; - while ((rv = ReadFile(handle, buffer, sizeof(buffer))) > 0) { + while ((rv = ReadFile(handle.get(), buffer, sizeof(buffer))) > 0) { + DCHECK_LE(static_cast(rv), sizeof(buffer)); local_contents.append(buffer, rv); } if (rv < 0) { From dc60e106f332294eb143f85581f8dbe6b5d63246 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 2 May 2017 16:50:14 -0400 Subject: [PATCH 13/32] =?UTF-8?q?linux:=20Make=20fewer=20(but=20still=20a?= =?UTF-8?q?=20lot=20of)=20regions=20in=20MemoryMap=E2=80=99s=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lots-of-regions tests in the MemoryMap test case were very time-consuming, particularly in debug mode. MemoryMap.MapRunningChild took as long as 15 seconds on-device (Nexus 5X), and the best result was in the neighborhood of 7 seconds. The bulk of the time spent in these tests was in ExpectMappings(), which calls MemoryMap::FindMapping() in a loop to verify each region. Each call to FindMapping() traverses the MemoryMap (internally, currently just a std::vector<>) from the beginning. With the need to verify 4,096 regions, a single call to ExpectMappings() had to perform over 8,000,000 checks to find the regions it needed. In turn, ExpectMappings() is called once by the SelfLargeMapFile test, and eight times by MapRunningChild. By reducing the number of regions to 1,024, each call to ExpectMappings() needs to perform “only” fewer than 600,000 checks. After this change, MemoryMap.MapRunningChild completes in about a half a second on-device. https://crashpad.chromium.org/bug/181 is concerned with implementing a RangeMap to serve MemoryMap and other similar code. After that’s done, it, it should be feasible to raise the number of regions used for these tests again. Bug: crashpad:30, crashpad:181 Test: crashpad_util_test MemoryMap.SelfLargeMapFile:MemoryMap.MapRunningChild Change-Id: I8ff88dac72a63c97ac937304b578fbe3b4ebf316 Reviewed-on: https://chromium-review.googlesource.com/494128 Reviewed-by: Joshua Peraza --- util/linux/memory_map_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index 23894b9b..cf5c9e22 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -252,7 +252,7 @@ void ExpectMappings(const MemoryMap& map, } TEST(MemoryMap, SelfLargeMapFile) { - constexpr size_t kNumMappings = 4096; + constexpr size_t kNumMappings = 1024; const size_t page_size = getpagesize(); ScopedMmap mappings; @@ -308,7 +308,7 @@ class MapRunningChildTest : public Multiprocess { CheckedWriteFile(WritePipeHandle(), ®ion_addr, sizeof(region_addr)); // But don't stop there! - constexpr size_t kNumExtraMappings = 1024; + constexpr size_t kNumExtraMappings = 256; ScopedMmap extra_mappings; while (true) { @@ -325,7 +325,7 @@ class MapRunningChildTest : public Multiprocess { } } - static constexpr size_t kNumMappings = 4096; + static constexpr size_t kNumMappings = 1024; const size_t page_size_; DISALLOW_COPY_AND_ASSIGN(MapRunningChildTest); From d9ca2ad21f311f29418fac133284ff31f8aeb20b Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 3 May 2017 11:59:56 -0400 Subject: [PATCH 14/32] Give group project-crashpad-tryjob-access access to the commit queue Bug: chromium:717982 Change-Id: I826f7520409656f5f549a110895e46de111d17f4 Reviewed-on: https://chromium-review.googlesource.com/494666 Reviewed-by: Mark Mentovai --- infra/config/cq.cfg | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/infra/config/cq.cfg b/infra/config/cq.cfg index 74aa1620..b436f9f8 100644 --- a/infra/config/cq.cfg +++ b/infra/config/cq.cfg @@ -27,8 +27,8 @@ gerrit {} verifiers { gerrit_cq_ability { - committer_list: "project-crashpad-committers" - dry_run_access_list: "project-crashpad-committers" + committer_list: "project-crashpad-tryjob-access" + dry_run_access_list: "project-crashpad-tryjob-access" } try_job { buckets { From 1969a5d7587b632335be2d083bed4c51ce29b6b1 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 3 May 2017 13:44:56 -0400 Subject: [PATCH 15/32] Document who has access to the try server and commit queue Drop the text recommending the PolyGerrit UI, since it is now the default Gerrit UI. Bug: chromium:717982 Change-Id: I7041ee51670a7a18b510ed7a55045cc2eb09983e Reviewed-on: https://chromium-review.googlesource.com/494726 Reviewed-by: Scott Graham --- doc/developing.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/doc/developing.md b/doc/developing.md index b5ee935e..6a4c4fd1 100644 --- a/doc/developing.md +++ b/doc/developing.md @@ -268,11 +268,6 @@ $ git commit $ git cl upload ``` -The [PolyGerrit interface](https://polygerrit.appspot.com/) to Gerrit, -undergoing active development, is recommended. To switch from the classic -GWT-based Gerrit UI to PolyGerrit, click the PolyGerrit link in a Gerrit review -page’s footer. - Uploading a patch to Gerrit does not automatically request a review. You must select a reviewer on the Gerrit review page after running `git cl upload`. This action notifies your reviewer of the code review request. If you have lost track @@ -292,7 +287,8 @@ server](https://dev.chromium.org/developers/testing/try-server-usage) by running “Commit-Queue: +1” label. This does not mean that the patch will be committed, but the try server and commit queue share infrastructure and a Gerrit label. The patch will be tested on try bots in a variety of configurations. Status -information will be available on Gerrit. +information will be available on Gerrit. Try server access is available to +Crashpad and Chromium committers. ### Landing Changes @@ -300,7 +296,8 @@ After code review is complete and “Code-Review: +1” has been received from a reviewers, the patch can be submitted to Crashpad’s [commit queue](https://dev.chromium.org/developers/testing/commit-queue) by clicking the “Submit to CQ” button in Gerrit. This sets the “Commit-Queue: +2” label, which -tests the patch on the try server before landing it. +tests the patch on the try server before landing it. Commit queue access is +available to Crashpad and Chromium committers. Although the commit queue is recommended, if needed, project members can bypass the commit queue and land patches without testing by using the “Submit” button From 7d56fd2386067a26878324ab48314e58e2b242ea Mon Sep 17 00:00:00 2001 From: Pierre-Antoine Manzagol Date: Tue, 2 May 2017 16:59:23 -0400 Subject: [PATCH 16/32] Rely on winsock2.h for timeval Bug: crashpad: Change-Id: Iee8ebfaf7c4a1e8e87fcfcbc6ee8a4529a2f7c52 Reviewed-on: https://chromium-review.googlesource.com/493893 Reviewed-by: Scott Graham Commit-Queue: Pierre-Antoine Manzagol --- compat/win/sys/time.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compat/win/sys/time.h b/compat/win/sys/time.h index 46bdef2a..71ddcdc2 100644 --- a/compat/win/sys/time.h +++ b/compat/win/sys/time.h @@ -15,9 +15,6 @@ #ifndef CRASHPAD_COMPAT_WIN_SYS_TIME_H_ #define CRASHPAD_COMPAT_WIN_SYS_TIME_H_ -struct timeval { - long tv_sec; - long tv_usec; -}; +#include #endif // CRASHPAD_COMPAT_WIN_SYS_TIME_H_ From f53f2c84cc53b5f7dab8ecd7b2b43dcd063b5606 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 3 May 2017 16:32:16 -0400 Subject: [PATCH 17/32] Fix comments identifying the source of module TimeDateStamp information Change-Id: I164f0208db103410c3133a67a994a4f603ce1b27 Reviewed-on: https://chromium-review.googlesource.com/494827 Reviewed-by: Scott Graham --- compat/non_win/dbghelp.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compat/non_win/dbghelp.h b/compat/non_win/dbghelp.h index dedd8ade..a8d5a0f3 100644 --- a/compat/non_win/dbghelp.h +++ b/compat/non_win/dbghelp.h @@ -533,11 +533,11 @@ struct __attribute__((packed, aligned(4))) MINIDUMP_MODULE { uint32_t CheckSum; //! \brief The module’s timestamp, in `time_t` units, seconds since the POSIX - //! epoch. + //! epoch, or `0` if unknown. //! //! On Windows, this field comes from the `TimeDateStamp` field of the - //! module’s `IMAGE_HEADER` structure. It reflects the timestamp at the time - //! the module was linked. + //! module’s `IMAGE_FILE_HEADER` structure. It reflects the timestamp at the + //! time the module was linked. uint32_t TimeDateStamp; //! \brief ::RVA of a MINIDUMP_STRING containing the module’s path or file @@ -723,11 +723,11 @@ struct __attribute__((packed, aligned(4))) MINIDUMP_UNLOADED_MODULE { uint32_t CheckSum; //! \brief The module’s timestamp, in `time_t` units, seconds since the POSIX - //! epoch. + //! epoch, or `0` if unknown. //! //! On Windows, this field comes from the `TimeDateStamp` field of the - //! module’s `IMAGE_HEADER` structure. It reflects the timestamp at the time - //! the module was linked. + //! module’s `IMAGE_FILE_HEADER` structure. It reflects the timestamp at the + //! time the module was linked. uint32_t TimeDateStamp; //! \brief ::RVA of a MINIDUMP_STRING containing the module’s path or file From 5ebd24e96ed2fea246c180ca278d5bf887f4c155 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 19 May 2017 16:44:10 -0400 Subject: [PATCH 18/32] Upload to the production Chromium Gerrit instance, not the canary Change-Id: Iad3bf52ba5f7babb1c6b3508fc034ab78949967d Reviewed-on: https://chromium-review.googlesource.com/509933 Reviewed-by: Scott Graham --- codereview.settings | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codereview.settings b/codereview.settings index bf4a3a0a..d8630972 100644 --- a/codereview.settings +++ b/codereview.settings @@ -14,7 +14,7 @@ GERRIT_HOST: True GERRIT_SQUASH_UPLOADS: True -CODE_REVIEW_SERVER: https://canary-chromium-review.googlesource.com/ +CODE_REVIEW_SERVER: https://chromium-review.googlesource.com/ VIEW_VC: https://chromium.googlesource.com/crashpad/crashpad/+/ PROJECT: crashpad BUG_PREFIX: crashpad: From 8fb23f2accf843d47d06304857e92b27fe2ff978 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 1 Jun 2017 12:13:05 -0700 Subject: [PATCH 19/32] linux: Provide ThreadInfo to collect register sets with ptrace ThreadInfo provides a uniform interface to collect register sets or the thread-local storage address across bitness for x86 and ARM family architectures. Additionally, ThreadInfo.h defines context structs which mirror those provided in sys/user.h. This allows tracing across bitness as the structs in sys/user.h are only provided for a single target architecture. Bug: crashpad:30 Change-Id: I91d0d788927bdac5fb630a6ad3c6ea6d3645ef8a Reviewed-on: https://chromium-review.googlesource.com/494075 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- compat/android/{ => linux}/elf.h | 25 ++- util/linux/thread_info.cc | 332 +++++++++++++++++++++++++++++++ util/linux/thread_info.h | 284 ++++++++++++++++++++++++++ util/linux/thread_info_test.cc | 100 ++++++++++ util/posix/process_info_linux.cc | 58 +----- util/util.gyp | 2 + util/util_test.gyp | 1 + 7 files changed, 742 insertions(+), 60 deletions(-) rename compat/android/{ => linux}/elf.h (57%) create mode 100644 util/linux/thread_info.cc create mode 100644 util/linux/thread_info.h create mode 100644 util/linux/thread_info_test.cc diff --git a/compat/android/elf.h b/compat/android/linux/elf.h similarity index 57% rename from compat/android/elf.h rename to compat/android/linux/elf.h index a3fc2a9a..e65d1532 100644 --- a/compat/android/elf.h +++ b/compat/android/linux/elf.h @@ -12,14 +12,27 @@ // 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_ +#ifndef CRASHPAD_COMPAT_ANDROID_LINUX_ELF_H_ +#define CRASHPAD_COMPAT_ANDROID_LINUX_ELF_H_ -#include_next +#include_next // Android 5.0.0 (API 21) NDK -#if !defined(NT_PRSTATUS) -#define NT_PRSTATUS 1 + +#if defined(__i386__) || defined(__x86_64__) +#if !defined(NT_386_TLS) +#define NT_386_TLS 0x200 +#endif +#endif // __i386__ || __x86_64__ + +#if defined(__ARMEL__) || defined(__aarch64__) +#if !defined(NT_ARM_VFP) +#define NT_ARM_VFP 0x400 #endif -#endif // CRASHPAD_COMPAT_ANDROID_ELF_H_ +#if !defined(NT_ARM_TLS) +#define NT_ARM_TLS 0x401 +#endif +#endif // __ARMEL__ || __aarch64__ + +#endif // CRASHPAD_COMPAT_ANDROID_LINUX_ELF_H_ diff --git a/util/linux/thread_info.cc b/util/linux/thread_info.cc new file mode 100644 index 00000000..d3897631 --- /dev/null +++ b/util/linux/thread_info.cc @@ -0,0 +1,332 @@ +// 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/linux/thread_info.h" + +#include +#include +#include +#include + +#include "base/logging.h" +#include "util/misc/from_pointer_cast.h" + +#if defined(ARCH_CPU_X86_FAMILY) +#include +#endif + +namespace crashpad { + +namespace { + +#if defined(ARCH_CPU_ARMEL) +// 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). Older Linux kernels support PTRACE_GETREGS, +// PTRACE_GETFPREGS, and PTRACE_GETVFPREGS instead, which don't allow checking +// the size of data copied. +// +// 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. + +bool GetGeneralPurposeRegistersLegacy(pid_t tid, ThreadContext* context) { + if (ptrace(PTRACE_GETREGS, tid, nullptr, &context->t32) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + return true; +} + +bool GetFloatingPointRegistersLegacy(pid_t tid, FloatContext* context) { + if (ptrace(PTRACE_GETFPREGS, tid, nullptr, &context->f32.fpregs) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + context->f32.have_fpregs = true; + + if (ptrace(PTRACE_GETVFPREGS, tid, nullptr, &context->f32.vfp) != 0) { + switch (errno) { + case EINVAL: + // These registers are optional on 32-bit ARM cpus + break; + default: + PLOG(ERROR) << "ptrace"; + return false; + } + } else { + context->f32.have_vfp = true; + } + return true; +} +#endif // ARCH_CPU_ARMEL + +#if defined(ARCH_CPU_ARM_FAMILY) +// Normally, the Linux kernel will copy out register sets according to the size +// of the struct that contains them. Tracing a 32-bit ARM process running in +// compatibility mode on a 64-bit ARM cpu will only copy data for the number of +// registers times the size of the register, which won't include any possible +// trailing padding in the struct. These are the sizes of the register data, not +// including any possible padding. +constexpr size_t kArmVfpSize = 32 * 8 + 4; + +// Target is 32-bit +bool GetFloatingPointRegisters32(pid_t tid, FloatContext* context) { + context->f32.have_fpregs = false; + context->f32.have_vfp = false; + + iovec iov; + iov.iov_base = &context->f32.fpregs; + iov.iov_len = sizeof(context->f32.fpregs); + if (ptrace( + PTRACE_GETREGSET, tid, reinterpret_cast(NT_PRFPREG), &iov) != + 0) { + switch (errno) { +#if defined(ARCH_CPU_ARMEL) + case EIO: + return GetFloatingPointRegistersLegacy(tid, context); +#endif // ARCH_CPU_ARMEL + case EINVAL: + // A 32-bit process running on a 64-bit CPU doesn't have this register + // set. It should have a VFP register set instead. + break; + default: + PLOG(ERROR) << "ptrace"; + return false; + } + } else { + if (iov.iov_len != sizeof(context->f32.fpregs)) { + LOG(ERROR) << "Unexpected registers size"; + return false; + } + context->f32.have_fpregs = true; + } + + iov.iov_base = &context->f32.vfp; + iov.iov_len = sizeof(context->f32.vfp); + if (ptrace( + PTRACE_GETREGSET, tid, reinterpret_cast(NT_ARM_VFP), &iov) != + 0) { + switch (errno) { + case EINVAL: + // VFP may not be present on 32-bit ARM cpus. + break; + default: + PLOG(ERROR) << "ptrace"; + return false; + } + } else { + if (iov.iov_len != kArmVfpSize && iov.iov_len != sizeof(context->f32.vfp)) { + LOG(ERROR) << "Unexpected registers size"; + return false; + } + context->f32.have_vfp = true; + } + + if (!(context->f32.have_fpregs || context->f32.have_vfp)) { + LOG(ERROR) << "Unable to collect registers"; + return false; + } + return true; +} + +// Target is 64-bit +bool GetFloatingPointRegisters64(pid_t tid, FloatContext* context) { + iovec iov; + iov.iov_base = context; + iov.iov_len = sizeof(*context); + if (ptrace( + PTRACE_GETREGSET, tid, reinterpret_cast(NT_PRFPREG), &iov) != + 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + if (iov.iov_len != sizeof(context->f64)) { + LOG(ERROR) << "Unexpected registers size"; + return false; + } + return true; +} +#endif // ARCH_CPU_ARM_FAMILY + +} // namespace + +ThreadContext::ThreadContext() { + memset(this, 0, sizeof(*this)); +} + +ThreadContext::~ThreadContext() {} + +FloatContext::FloatContext() { + memset(this, 0, sizeof(*this)); +} + +FloatContext::~FloatContext() {} + +ThreadInfo::ThreadInfo() + : context_(), attachment_(), tid_(-1), initialized_(), is_64_bit_(false) {} + +ThreadInfo::~ThreadInfo() {} + +bool ThreadInfo::Initialize(pid_t tid) { + INITIALIZATION_STATE_SET_INITIALIZING(initialized_); + + if (!attachment_.ResetAttach(tid)) { + return false; + } + tid_ = tid; + + size_t length = GetGeneralPurposeRegistersAndLength(&context_); + if (length == sizeof(context_.t64)) { + is_64_bit_ = true; + } else if (length == sizeof(context_.t32)) { + is_64_bit_ = false; + } else { + LOG(ERROR) << "Unexpected registers size"; + return false; + } + + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; +} + +bool ThreadInfo::Is64Bit() { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return is_64_bit_; +} + +void ThreadInfo::GetGeneralPurposeRegisters(ThreadContext* context) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + *context = context_; +} + +size_t ThreadInfo::GetGeneralPurposeRegistersAndLength(ThreadContext* context) { + iovec iov; + iov.iov_base = context; + iov.iov_len = sizeof(*context); + if (ptrace( + PTRACE_GETREGSET, tid_, reinterpret_cast(NT_PRSTATUS), &iov) != + 0) { + switch (errno) { +#if defined(ARCH_CPU_ARMEL) + case EIO: + if (GetGeneralPurposeRegistersLegacy(tid_, context)) { + return sizeof(context->t32); + } +#endif // ARCH_CPU_ARMEL + default: + PLOG(ERROR) << "ptrace"; + return 0; + } + } + return iov.iov_len; +} + +bool ThreadInfo::GetFloatingPointRegisters(FloatContext* context) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + +#if defined(ARCH_CPU_X86_FAMILY) + iovec iov; + iov.iov_base = context; + iov.iov_len = sizeof(*context); + if (ptrace( + PTRACE_GETREGSET, tid_, reinterpret_cast(NT_PRFPREG), &iov) != + 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + if (is_64_bit_ && iov.iov_len == sizeof(context->f64)) { + return true; + } + if (!is_64_bit_ && iov.iov_len == sizeof(context->f32)) { + return true; + } + LOG(ERROR) << "Unexpected registers size"; + return false; + +#elif defined(ARCH_CPU_ARM_FAMILY) + return is_64_bit_ ? GetFloatingPointRegisters64(tid_, context) + : GetFloatingPointRegisters32(tid_, context); +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY +} + +bool ThreadInfo::GetThreadArea(LinuxVMAddress* address) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + +#if defined(ARCH_CPU_X86_FAMILY) + if (is_64_bit_) { + *address = context_.t64.fs_base; + return true; + } + + user_desc desc; + iovec iov; + iov.iov_base = &desc; + iov.iov_len = sizeof(desc); + *address = 0; + if (ptrace( + PTRACE_GETREGSET, tid_, reinterpret_cast(NT_386_TLS), &iov) != + 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + *address = desc.base_addr; + return true; + +#elif defined(ARCH_CPU_ARM_FAMILY) + if (is_64_bit_) { + iovec iov; + iov.iov_base = address; + iov.iov_len = sizeof(*address); + if (ptrace(PTRACE_GETREGSET, + tid_, + reinterpret_cast(NT_ARM_TLS), + &iov) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + if (iov.iov_len != 8) { + LOG(ERROR) << "address size mismatch"; + return false; + } + return true; + } + +#if defined(ARCH_CPU_ARMEL) + void* result; + if (ptrace(PTRACE_GET_THREAD_AREA, tid_, nullptr, &result) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + *address = FromPointerCast(result); + return true; +#else + // TODO(jperaza): it doesn't look like there is a way for a 64-bit ARM process + // to get the thread area for a 32-bit ARM process with ptrace. + LOG(WARNING) << "64-bit ARM cannot trace TLS area for a 32-bit process"; + return false; +#endif // ARCH_CPU_ARMEL +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY +} + +} // namespace crashpad diff --git a/util/linux/thread_info.h b/util/linux/thread_info.h new file mode 100644 index 00000000..6fb2621d --- /dev/null +++ b/util/linux/thread_info.h @@ -0,0 +1,284 @@ +// 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_LINUX_THREAD_INFO_H_ +#define CRASHPAD_UTIL_LINUX_THREAD_INFO_H_ + +#include +#include +#include + +#include + +#include "build/build_config.h" +#include "util/linux/address_types.h" +#include "util/linux/scoped_ptrace_attach.h" +#include "util/misc/initialization_state_dcheck.h" +#include "util/numeric/int128.h" + +namespace crashpad { + +//! \brief The set of general purpose registers for an architecture family. +union ThreadContext { + ThreadContext(); + ~ThreadContext(); + + //! \brief The general purpose registers used by the 32-bit variant of the + //! architecture. + struct t32 { +#if defined(ARCH_CPU_X86_FAMILY) + // Reflects user_regs_struct in sys/user.h. + uint32_t ebx; + uint32_t ecx; + uint32_t edx; + uint32_t esi; + uint32_t edi; + uint32_t ebp; + uint32_t eax; + uint32_t xds; + uint32_t xes; + uint32_t xfs; + uint32_t xgs; + uint32_t orig_eax; + uint32_t eip; + uint32_t xcs; + uint32_t eflags; + uint32_t esp; + uint32_t xss; +#elif defined(ARCH_CPU_ARM_FAMILY) + // Reflects user_regs in sys/user.h. + uint32_t regs[11]; + uint32_t fp; + uint32_t ip; + uint32_t sp; + uint32_t lr; + uint32_t pc; + uint32_t cpsr; + uint32_t orig_r0; +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY + } t32; + + //! \brief The general purpose registers used by the 64-bit variant of the + //! architecture. + struct t64 { +#if defined(ARCH_CPU_X86_FAMILY) + // Reflects user_regs_struct in sys/user.h. + uint64_t r15; + uint64_t r14; + uint64_t r13; + uint64_t r12; + uint64_t rbp; + uint64_t rbx; + uint64_t r11; + uint64_t r10; + uint64_t r9; + uint64_t r8; + uint64_t rax; + uint64_t rcx; + uint64_t rdx; + uint64_t rsi; + uint64_t rdi; + uint64_t orig_rax; + uint64_t rip; + uint64_t cs; + uint64_t eflags; + uint64_t rsp; + uint64_t ss; + uint64_t fs_base; + uint64_t gs_base; + uint64_t ds; + uint64_t es; + uint64_t fs; + uint64_t gs; +#elif defined(ARCH_CPU_ARM_FAMILY) + // Reflects user_regs_struct in sys/user.h. + uint64_t regs[31]; + uint64_t sp; + uint64_t pc; + uint64_t pstate; +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY + } t64; + +#if defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARM64) + using NativeThreadContext = user_regs_struct; +#elif defined(ARCH_CPU_ARMEL) + using NativeThreadContext = user_regs; +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY || ARCH_CPU_ARM64 + +#if defined(ARCH_CPU_32_BITS) + static_assert(sizeof(t32) == sizeof(NativeThreadContext), "Size mismatch"); +#else // ARCH_CPU_64_BITS + static_assert(sizeof(t64) == sizeof(NativeThreadContext), "Size mismatch"); +#endif // ARCH_CPU_32_BITS +}; +static_assert(std::is_standard_layout::value, + "Not standard layout"); + +//! \brief The floating point registers used for an architecture family. +union FloatContext { + FloatContext(); + ~FloatContext(); + + //! \brief The floating point registers used by the 32-bit variant of the + //! architecture. + struct f32 { +#if defined(ARCH_CPU_X86_FAMILY) + // Reflects user_fpregs_struct in sys/user.h. + uint32_t cwd; + uint32_t swd; + uint32_t twd; + uint32_t fip; + uint32_t fcs; + uint32_t foo; + uint32_t fos; + uint32_t st_space[20]; +#elif defined(ARCH_CPU_ARM_FAMILY) + // Reflects user_fpregs in sys/user.h. + struct fpregs { + struct fp_reg { + uint32_t sign1 : 1; + uint32_t unused : 15; + uint32_t sign2 : 1; + uint32_t exponent : 14; + uint32_t j : 1; + uint32_t mantissa1 : 31; + uint32_t mantisss0 : 32; + } fpregs[8]; + uint32_t fpsr : 32; + uint32_t fpcr : 32; + uint8_t type[8]; + uint32_t init_flag; + } fpregs; + + // Reflects user_vfp in sys/user.h. + struct vfp { + uint64_t fpregs[32]; + uint32_t fpscr; + } vfp; + + bool have_fpregs; + bool have_vfp; +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY + } f32; + + //! \brief The floating point registers used by the 64-bit variant of the + //! architecture. + struct f64 { +#if defined(ARCH_CPU_X86_FAMILY) + uint16_t cwd; + uint16_t swd; + uint16_t ftw; + uint16_t fop; + uint64_t rip; + uint64_t rdp; + uint32_t mxcsr; + uint32_t mxcr_mask; + uint32_t st_space[32]; + uint32_t xmm_space[64]; + uint32_t padding[24]; +#elif defined(ARCH_CPU_ARM_FAMILY) + uint128_struct vregs[32]; + uint32_t fpsr; + uint32_t fpcr; + uint8_t padding[8]; +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY + } f64; + +#if defined(ARCH_CPU_X86) + static_assert(sizeof(f32) == sizeof(user_fpregs_struct), "Size mismatch"); +#elif defined(ARCH_CPU_X86_64) + static_assert(sizeof(f64) == sizeof(user_fpregs_struct), "Size mismatch"); +#elif defined(ARCH_CPU_ARMEL) + static_assert(sizeof(f32::fpregs) == sizeof(user_fpregs), "Size mismatch"); + static_assert(sizeof(f32::vfp) == sizeof(user_vfp), "Size mismatch"); +#elif defined(ARCH_CPU_ARM64) + static_assert(sizeof(f64) == sizeof(user_fpsimd_struct), "Size mismatch"); +#else +#error Port. +#endif // ARCH_CPU_X86 +}; +static_assert(std::is_standard_layout::value, + "Not standard layout"); + +class ThreadInfo { + public: + ThreadInfo(); + ~ThreadInfo(); + + //! \brief Initializes this object with information about the thread whose ID + //! is \a tid. + //! + //! This method must be called successfully prior to calling any other method + //! in this class. This method may only be called once. + //! + //! It is unspecified whether the information that an object of this class + //! returns is loaded at the time Initialize() is called or subsequently, and + //! whether this information is cached in the object or not. + //! + //! \param[in] tid The thread ID to obtain information for. + //! + //! \return `true` on success, `false` on failure with a message logged. + bool Initialize(pid_t tid); + + //! \brief Determines the target thread’s bitness. + //! + //! \return `true` if the target is 64-bit. + bool Is64Bit(); + + //! \brief Uses `ptrace` to collect general purpose registers from the target + //! thread and places the result in \a context. + //! + //! \param[out] context The registers read from the target thread. + void GetGeneralPurposeRegisters(ThreadContext* context); + + //! \brief Uses `ptrace` to collect floating point registers from the target + //! thread and places the result in \a context. + //! + //! \param[out] context The registers read from the target thread. + //! + //! \return `true` on success, with \a context set. Otherwise, `false` with a + //! message logged. + bool GetFloatingPointRegisters(FloatContext* context); + + //! \brief Uses `ptrace` to determine the thread-local storage address for the + //! target thread and places the result in \a address. + //! + //! \param[out] address The address of the TLS area. + //! + //! \return `true` on success. `false` on failure with a message logged. + bool GetThreadArea(LinuxVMAddress* address); + + private: + size_t GetGeneralPurposeRegistersAndLength(ThreadContext* context); + + ThreadContext context_; + ScopedPtraceAttach attachment_; + pid_t tid_; + InitializationStateDcheck initialized_; + bool is_64_bit_; +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_LINUX_THREAD_INFO_H_ diff --git a/util/linux/thread_info_test.cc b/util/linux/thread_info_test.cc new file mode 100644 index 00000000..fe05b4a8 --- /dev/null +++ b/util/linux/thread_info_test.cc @@ -0,0 +1,100 @@ +// 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/linux/thread_info.h" + +#include "build/build_config.h" +#include "gtest/gtest.h" +#include "test/multiprocess.h" +#include "util/file/file_io.h" +#include "util/misc/from_pointer_cast.h" + +namespace crashpad { +namespace test { +namespace { + +class SameBitnessTest : public Multiprocess { + public: + SameBitnessTest() : Multiprocess() {} + ~SameBitnessTest() {} + + private: + void MultiprocessParent() override { + LinuxVMAddress expected_tls; + CheckedReadFileExactly( + ReadPipeHandle(), &expected_tls, sizeof(expected_tls)); + + ThreadInfo thread_info; + ASSERT_TRUE(thread_info.Initialize(ChildPID())); + +#if defined(ARCH_CPU_64_BITS) + const bool am_64_bit = true; +#else + const bool am_64_bit = false; +#endif // ARCH_CPU_64_BITS + + EXPECT_EQ(thread_info.Is64Bit(), am_64_bit); + + ThreadContext thread_context; + thread_info.GetGeneralPurposeRegisters(&thread_context); + +#if defined(ARCH_CPU_X86_64) + EXPECT_EQ(thread_context.t64.fs_base, expected_tls); +#endif // ARCH_CPU_X86_64 + + FloatContext float_context; + EXPECT_TRUE(thread_info.GetFloatingPointRegisters(&float_context)); + + LinuxVMAddress tls_address; + ASSERT_TRUE(thread_info.GetThreadArea(&tls_address)); + EXPECT_EQ(tls_address, expected_tls); + } + + void MultiprocessChild() override { + LinuxVMAddress expected_tls; +#if defined(ARCH_CPU_ARMEL) + // 0xffff0fe0 is the address of the kernel user helper __kuser_get_tls(). + auto kuser_get_tls = reinterpret_cast(0xffff0fe0); + expected_tls = FromPointerCast(kuser_get_tls()); +#elif defined(ARCH_CPU_ARM64) + // Linux/aarch64 places the tls address in system register tpidr_el0. + asm("mrs %0, tpidr_el0" : "=r"(expected_tls)); +#elif defined(ARCH_CPU_X86) + uint32_t expected_tls_32; + asm("movl %%gs:0x0, %0" : "=r"(expected_tls_32)); + expected_tls = expected_tls_32; +#elif defined(ARCH_CPU_X86_64) + asm("movq %%fs:0x0, %0" : "=r"(expected_tls)); +#else +#error Port. +#endif // ARCH_CPU_ARMEL + + CheckedWriteFile(WritePipeHandle(), &expected_tls, sizeof(expected_tls)); + + CheckedReadFileAtEOF(ReadPipeHandle()); + } + + DISALLOW_COPY_AND_ASSIGN(SameBitnessTest); +}; + +TEST(ThreadInfo, SameBitness) { + SameBitnessTest test; + test.Run(); +} + +// TODO(jperaza): Test against a process with different bitness. + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/posix/process_info_linux.cc b/util/posix/process_info_linux.cc index 65d96442..ac3a2aea 100644 --- a/util/posix/process_info_linux.cc +++ b/util/posix/process_info_linux.cc @@ -15,12 +15,8 @@ #include "util/posix/process_info.h" #include -#include #include #include -#include -#include -#include #include #include @@ -33,7 +29,7 @@ #include "util/file/delimited_file_reader.h" #include "util/file/file_io.h" #include "util/file/file_reader.h" -#include "util/linux/scoped_ptrace_attach.h" +#include "util/linux/thread_info.h" namespace crashpad { @@ -305,57 +301,11 @@ bool ProcessInfo::Is64Bit(bool* is_64_bit) const { if (pid_ == getpid()) { is_64_bit_ = am_64_bit; } else { - ScopedPtraceAttach ptrace_attach; - if (!ptrace_attach.ResetAttach(pid_)) { + ThreadInfo thread_info; + if (!thread_info.Initialize(pid_)) { 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) || defined(ARCH_CPU_ARM64) - using PrStatusType = user_regs_struct; -#elif defined(ARCH_CPU_ARMEL) - using PrStatusType = user_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) { - 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_ = thread_info.Is64Bit(); } is_64_bit_initialized_.set_valid(); diff --git a/util/util.gyp b/util/util.gyp index e3074e9a..aae764e5 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -50,6 +50,8 @@ 'linux/memory_map.h', 'linux/process_memory.cc', 'linux/process_memory.h', + 'linux/thread_info.cc', + 'linux/thread_info.h', 'linux/scoped_ptrace_attach.cc', 'linux/scoped_ptrace_attach.h', 'mac/checked_mach_address_range.h', diff --git a/util/util_test.gyp b/util/util_test.gyp index db54a9b0..63829b32 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -41,6 +41,7 @@ 'file/string_file_test.cc', 'linux/memory_map_test.cc', 'linux/process_memory_test.cc', + 'linux/thread_info_test.cc', 'linux/scoped_ptrace_attach_test.cc', 'mac/launchd_test.mm', 'mac/mac_util_test.mm', From dbc229a2d7737acf38bcb67cae0cf386b7f37243 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 9 Jun 2017 14:32:59 -0700 Subject: [PATCH 20/32] Update mini_chromium to 606ff8a3 > git log --oneline ef0ded87..606ff8a3 606ff8a Remove base/memory/aligned_memory.* Change-Id: Id3b1b75f2e18437543dc4703f6b2dc578ac7fa75 Reviewed-on: https://chromium-review.googlesource.com/530071 Reviewed-by: Mark Mentovai Commit-Queue: Scott Graham --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 0c7f66be..e6f4770e 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - 'ef0ded8717340c9fe48e8e0f34f3e0e74d10a392', + '606ff8a31ae16f54a9c425880e57b3027f52db0c', 'crashpad/third_party/zlib/zlib': Var('chromium_git') + '/chromium/src/third_party/zlib@' + '13dc246a58e4b72104d35f9b1809af95221ebda7', From 8e2e805fa5b0091deecc56ea903fb534fefb19f0 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Mon, 12 Jun 2017 12:59:27 -0700 Subject: [PATCH 21/32] linux: Add AuxiliaryVector for reading other process' aux vectors Bug: crashpad:30 Change-Id: Ief19be7d60decb17f159b3d740ac9d15a034b807 Reviewed-on: https://chromium-review.googlesource.com/526533 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/linux/auxiliary_vector.cc | 104 ++++++++++++++ util/linux/auxiliary_vector.h | 76 ++++++++++ util/linux/auxiliary_vector_test.cc | 209 ++++++++++++++++++++++++++++ util/util.gyp | 2 + util/util_test.gyp | 1 + 5 files changed, 392 insertions(+) create mode 100644 util/linux/auxiliary_vector.cc create mode 100644 util/linux/auxiliary_vector.h create mode 100644 util/linux/auxiliary_vector_test.cc diff --git a/util/linux/auxiliary_vector.cc b/util/linux/auxiliary_vector.cc new file mode 100644 index 00000000..8852e614 --- /dev/null +++ b/util/linux/auxiliary_vector.cc @@ -0,0 +1,104 @@ +// 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/linux/auxiliary_vector.h" + +#include +#include +#include + +#include + +#include "base/files/file_path.h" +#include "base/logging.h" +#include "util/file/file_reader.h" +#include "util/stdlib/map_insert.h" + +namespace crashpad { + +AuxiliaryVector::AuxiliaryVector() : values_() {} + +AuxiliaryVector::~AuxiliaryVector() {} + +bool AuxiliaryVector::Initialize(pid_t pid, bool is_64_bit) { + return is_64_bit ? Read(pid) : Read(pid); +} + +template +bool AuxiliaryVector::Read(pid_t pid) { + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/auxv", pid); + FileReader aux_file; + if (!aux_file.Open(base::FilePath(path))) { + return false; + } + + ULong type; + ULong value; + while (aux_file.ReadExactly(&type, sizeof(type)) && + aux_file.ReadExactly(&value, sizeof(value))) { + if (type == AT_NULL && value == 0) { + return true; + } + if (type == AT_IGNORE) { + continue; + } + if (!MapInsertOrReplace(&values_, type, value, nullptr)) { + LOG(ERROR) << "duplicate auxv entry"; + return false; + } + } + return false; +} + +// static +bool AuxiliaryVector::VariableSizeBitCast(uint64_t data, + char* dest, + size_t dest_size) { + auto data_p = reinterpret_cast(&data); + constexpr size_t data_size = sizeof(uint64_t); + + // Verify that any unused bytes from data are zero. + // The unused bytes are at the start of the data buffer for big-endian and the + // end of the buffer for little-endian. + if (dest_size < data_size) { + uint64_t zero = 0; + auto extra_bytes = data_p; +#if defined(ARCH_CPU_LITTLE_ENDIAN) + extra_bytes += dest_size; +#endif // ARCH_CPU_LITTLE_ENDIAN + if (memcmp(extra_bytes, &zero, data_size - dest_size) != 0) { + LOG(ERROR) << "information loss"; + return false; + } + } + + // Zero out the destination, in case it is larger than data. + memset(dest, 0, dest_size); + +#if defined(ARCH_CPU_LITTLE_ENDIAN) + // Copy a prefix of data to a prefix of dest for little-endian + memcpy(dest, data_p, std::min(dest_size, data_size)); +#else + // or the suffix of data to the suffix of dest for big-endian + if (data_size >= dest_size) { + memcpy(dest, data_p + data_size - dest_size, dest_size); + } else { + memcpy(dest + dest_size - data_size, data_p, data_size); + } +#endif // ARCH_CPU_LITTLE_ENDIAN + return true; +} + +} // namespace crashpad diff --git a/util/linux/auxiliary_vector.h b/util/linux/auxiliary_vector.h new file mode 100644 index 00000000..904eada4 --- /dev/null +++ b/util/linux/auxiliary_vector.h @@ -0,0 +1,76 @@ +// 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_LINUX_AUXILIARY_VECTOR_H_ +#define CRASHPAD_UTIL_LINUX_AUXILIARY_VECTOR_H_ + +#include + +#include + +#include "base/logging.h" +#include "base/macros.h" + +namespace crashpad { + +//! \brief Read the auxiliary vector for a target process. +class AuxiliaryVector { + public: + AuxiliaryVector(); + ~AuxiliaryVector(); + + //! \brief Initializes this object with the auxiliary vector for the process + //! with process ID \a pid. + //! + //! This method must be called successfully prior to calling any other method + //! in this class. + //! + //! \param[in] pid The process ID of a target process. + //! \param[in] is_64_bit Whether the target process is 64-bit. + //! + //! \return `true` on success, `false` on failure with a message logged. + bool Initialize(pid_t pid, bool is_64_bit); + + //! \brief Retrieve a value from the vector. + //! + //! \param[in] type Specifies which value should be retrieved. The possible + //! values for this parameter are defined by ``. + //! \param[out] value The value, casted to an appropriate type, if found. + //! \return `true` if the value is found. + template + bool GetValue(uint64_t type, V* value) const { + auto iter = values_.find(type); + if (iter == values_.end()) { + LOG(ERROR) << "value not found"; + return false; + } + return VariableSizeBitCast( + iter->second, reinterpret_cast(value), sizeof(V)); + } + + protected: + std::map values_; + + private: + template + bool Read(pid_t pid); + + static bool VariableSizeBitCast(uint64_t data, char* dest, size_t dest_size); + + DISALLOW_COPY_AND_ASSIGN(AuxiliaryVector); +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_LINUX_AUXILIARY_VECTOR_H_ diff --git a/util/linux/auxiliary_vector_test.cc b/util/linux/auxiliary_vector_test.cc new file mode 100644 index 00000000..89bc3c7e --- /dev/null +++ b/util/linux/auxiliary_vector_test.cc @@ -0,0 +1,209 @@ +// 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/linux/auxiliary_vector.h" + +#include +#include +#include + +#include + +#include "base/macros.h" +#include "gtest/gtest.h" +#include "test/errors.h" +#include "test/multiprocess.h" +#include "util/linux/address_types.h" +#include "util/linux/memory_map.h" +#include "util/linux/process_memory.h" +#include "util/misc/from_pointer_cast.h" +#include "util/numeric/int128.h" + +extern "C" { +extern void _start(); +} // extern "C" + +namespace crashpad { +namespace test { +namespace { + +void TestAgainstCloneOrSelf(pid_t pid) { +#if defined(ARCH_CPU_64_BITS) + constexpr bool am_64_bit = true; +#else + constexpr bool am_64_bit = false; +#endif + AuxiliaryVector aux; + ASSERT_TRUE(aux.Initialize(pid, am_64_bit)); + + MemoryMap mappings; + ASSERT_TRUE(mappings.Initialize(pid)); + + LinuxVMAddress phdrs; + ASSERT_TRUE(aux.GetValue(AT_PHDR, &phdrs)); + EXPECT_TRUE(mappings.FindMapping(phdrs)); + + int pagesize; + ASSERT_TRUE(aux.GetValue(AT_PAGESZ, &pagesize)); + EXPECT_EQ(pagesize, getpagesize()); + + LinuxVMAddress interp_base; + ASSERT_TRUE(aux.GetValue(AT_BASE, &interp_base)); + EXPECT_TRUE(mappings.FindMapping(interp_base)); + + LinuxVMAddress entry_addr; + ASSERT_TRUE(aux.GetValue(AT_ENTRY, &entry_addr)); + EXPECT_EQ(entry_addr, FromPointerCast(_start)); + + uid_t uid; + ASSERT_TRUE(aux.GetValue(AT_UID, &uid)); + EXPECT_EQ(uid, getuid()); + + uid_t euid; + ASSERT_TRUE(aux.GetValue(AT_EUID, &euid)); + EXPECT_EQ(euid, geteuid()); + + gid_t gid; + ASSERT_TRUE(aux.GetValue(AT_GID, &gid)); + EXPECT_EQ(gid, getgid()); + + gid_t egid; + ASSERT_TRUE(aux.GetValue(AT_EGID, &egid)); + EXPECT_EQ(egid, getegid()); + + ProcessMemory memory; + ASSERT_TRUE(memory.Initialize(pid)); + + LinuxVMAddress platform_addr; + ASSERT_TRUE(aux.GetValue(AT_PLATFORM, &platform_addr)); + std::string platform; + ASSERT_TRUE(memory.ReadCStringSizeLimited(platform_addr, 10, &platform)); +#if defined(ARCH_CPU_X86) + EXPECT_STREQ(platform.c_str(), "i686"); +#elif defined(ARCH_CPU_X86_64) + EXPECT_STREQ(platform.c_str(), "x86_64"); +#elif defined(ARCH_CPU_ARMEL) + // Machine name and platform are set in Linux:/arch/arm/kernel/setup.c + // Machine typically looks like "armv7l". + // Platform typically looks like "v7l". + utsname sys_names; + ASSERT_EQ(uname(&sys_names), 0); + std::string machine_name(sys_names.machine); + EXPECT_NE(machine_name.find(platform), std::string::npos); +#elif defined(ARCH_CPU_ARM64) + EXPECT_STREQ(platform.c_str(), "aarch64"); +#endif // ARCH_CPU_X86 + +#if defined(AT_SYSINFO_EHDR) + LinuxVMAddress vdso_addr; + ASSERT_TRUE(aux.GetValue(AT_SYSINFO_EHDR, &vdso_addr)); + EXPECT_TRUE(mappings.FindMapping(vdso_addr)); +#endif // AT_SYSINFO_EHDR + +#if defined(AT_EXECFN) + LinuxVMAddress filename_addr; + ASSERT_TRUE(aux.GetValue(AT_EXECFN, &filename_addr)); + std::string filename; + ASSERT_TRUE(memory.ReadCStringSizeLimited(filename_addr, 4096, &filename)); + EXPECT_TRUE(filename.find("crashpad_util_test") != std::string::npos); +#endif // AT_EXECFN + + int ignore; + EXPECT_FALSE(aux.GetValue(AT_NULL, &ignore)); + + char too_small; + EXPECT_FALSE(aux.GetValue(AT_PAGESZ, &too_small)); + + uint128_struct big_dest; + memset(&big_dest, 0xf, sizeof(big_dest)); + ASSERT_TRUE(aux.GetValue(AT_PHDR, &big_dest)); + EXPECT_EQ(big_dest.lo, phdrs); +} + +TEST(AuxiliaryVector, ReadSelf) { + TestAgainstCloneOrSelf(getpid()); +} + +class ReadChildTest : public Multiprocess { + public: + ReadChildTest() : Multiprocess() {} + ~ReadChildTest() {} + + private: + void MultiprocessParent() override { TestAgainstCloneOrSelf(ChildPID()); } + + void MultiprocessChild() override { CheckedReadFileAtEOF(ReadPipeHandle()); } + + DISALLOW_COPY_AND_ASSIGN(ReadChildTest); +}; + +TEST(AuxiliaryVector, ReadChild) { + ReadChildTest test; + test.Run(); +} + +class AuxVecTester : public AuxiliaryVector { + public: + AuxVecTester() : AuxiliaryVector() {} + void Insert(uint64_t type, uint64_t value) { values_[type] = value; } +}; + +TEST(AuxiliaryVector, SignedBit) { +#if defined(ARCH_CPU_64_BITS) + constexpr bool am_64_bit = true; +#else + constexpr bool am_64_bit = false; +#endif + AuxVecTester aux; + ASSERT_TRUE(aux.Initialize(getpid(), am_64_bit)); + constexpr uint64_t type = 0x0000000012345678; + + constexpr int32_t neg1_32 = -1; + aux.Insert(type, bit_cast(neg1_32)); + int32_t outval32s; + ASSERT_TRUE(aux.GetValue(type, &outval32s)); + EXPECT_EQ(outval32s, neg1_32); + + constexpr int32_t int32_max = std::numeric_limits::max(); + aux.Insert(type, bit_cast(int32_max)); + ASSERT_TRUE(aux.GetValue(type, &outval32s)); + EXPECT_EQ(outval32s, int32_max); + + constexpr uint32_t uint32_max = std::numeric_limits::max(); + aux.Insert(type, uint32_max); + uint32_t outval32u; + ASSERT_TRUE(aux.GetValue(type, &outval32u)); + EXPECT_EQ(outval32u, uint32_max); + + constexpr int64_t neg1_64 = -1; + aux.Insert(type, bit_cast(neg1_64)); + int64_t outval64s; + ASSERT_TRUE(aux.GetValue(type, &outval64s)); + EXPECT_EQ(outval64s, neg1_64); + + constexpr int64_t int64_max = std::numeric_limits::max(); + aux.Insert(type, bit_cast(int64_max)); + ASSERT_TRUE(aux.GetValue(type, &outval64s)); + EXPECT_EQ(outval64s, int64_max); + + constexpr uint64_t uint64_max = std::numeric_limits::max(); + aux.Insert(type, uint64_max); + uint64_t outval64u; + ASSERT_TRUE(aux.GetValue(type, &outval64u)); + EXPECT_EQ(outval64u, uint64_max); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index aae764e5..8e5c7cfe 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -45,6 +45,8 @@ 'file/string_file.cc', 'file/string_file.h', 'linux/address_types.h', + 'linux/auxiliary_vector.cc', + 'linux/auxiliary_vector.h', 'linux/checked_address_range.h', 'linux/memory_map.cc', 'linux/memory_map.h', diff --git a/util/util_test.gyp b/util/util_test.gyp index 63829b32..ef5a681f 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -39,6 +39,7 @@ 'file/file_io_test.cc', 'file/file_reader_test.cc', 'file/string_file_test.cc', + 'linux/auxiliary_vector_test.cc', 'linux/memory_map_test.cc', 'linux/process_memory_test.cc', 'linux/thread_info_test.cc', From 1c0c305bc90def9442212020fa6a883aa3a07695 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 13 Jun 2017 08:39:46 -0700 Subject: [PATCH 22/32] linux: Add FindMappingWithName to MemoryMap Bug: crashpad:30 Change-Id: I5e03dc14e3cd1e09ac45cba97922499ec48ea389 Reviewed-on: https://chromium-review.googlesource.com/532753 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/linux/memory_map.cc | 12 ++++++++++++ util/linux/memory_map.h | 12 +++++++++--- util/linux/memory_map_test.cc | 1 + 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc index d1ef6c09..4283cb84 100644 --- a/util/linux/memory_map.cc +++ b/util/linux/memory_map.cc @@ -255,4 +255,16 @@ const MemoryMap::Mapping* MemoryMap::FindMapping(LinuxVMAddress address) const { return nullptr; } +const MemoryMap::Mapping* MemoryMap::FindMappingWithName( + const std::string& name) const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + for (const auto& mapping : mappings_) { + if (mapping.name == name) { + return &mapping; + } + } + return nullptr; +} + } // namespace crashpad diff --git a/util/linux/memory_map.h b/util/linux/memory_map.h index 2d7a01a1..22f4448b 100644 --- a/util/linux/memory_map.h +++ b/util/linux/memory_map.h @@ -63,11 +63,17 @@ class MemoryMap { //! \return `true` on success, `false` on failure with a message logged. bool Initialize(pid_t pid); - //! \return The Mapping containing \a address. The caller does not take - //! ownership of this object. It is scoped to the lifetime of the - //! MemoryMap object that it was obtained from. + //! \return The Mapping containing \a address or `nullptr` if no match is + //! found. The caller does not take ownership of this object. It is scoped + //! to the lifetime of the MemoryMap object that it was obtained from. const Mapping* FindMapping(LinuxVMAddress address) const; + //! \return The Mapping with the lowest base address whose name is \a name or + //! `nullptr` if no match is found. The caller does not take ownership of + //! this object. It is scoped to the lifetime of the MemoryMap object that + //! it was obtained from. + const Mapping* FindMappingWithName(const std::string& name) const; + private: std::vector mappings_; InitializationStateDcheck initialized_; diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index cf5c9e22..aab5c4ac 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -149,6 +149,7 @@ class MapChildTest : public Multiprocess { << ErrnoMessage("stat"); EXPECT_EQ(mapping->device, file_stat.st_dev); EXPECT_EQ(mapping->inode, file_stat.st_ino); + EXPECT_EQ(map.FindMappingWithName(mapping->name), mapping); } void MultiprocessChild() override { From 8c35d92ae403b9eb7c93578b3209f4767b981e64 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 13 Jun 2017 13:48:07 -0400 Subject: [PATCH 23/32] Use the C++11-standardized alignof instead of ALIGNOF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use the standard alignas instead of ALIGNAS in cases where this is possible too. It’s not currently possible where ALIGNAS may be mixed with other attributes, although the not-landed https://codereview.chromium.org/2670873002/ suggests that where ALIGNAS is mixed with __attribute__((packed)), it’s viable to write “struct alignas(4) S { /* … */ } __attribute__((packed));”. This includes an update of mini_chromium to 723e840a2f100a525f7feaad2e93df31d701780a, picking up: 723e840a2f10 Remove ALIGNOF This tracks upstream https://codereview.chromium.org/2932053002/. Change-Id: I7ddaf829020ef3be0512f803cecbb7c543294f07 Reviewed-on: https://chromium-review.googlesource.com/533356 Reviewed-by: Scott Graham Commit-Queue: Mark Mentovai --- DEPS | 2 +- minidump/minidump_context.h | 2 +- util/stdlib/aligned_allocator.h | 4 ++-- util/stdlib/aligned_allocator_test.cc | 32 +++++++++++++-------------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/DEPS b/DEPS index e6f4770e..742430d5 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '606ff8a31ae16f54a9c425880e57b3027f52db0c', + '723e840a2f100a525f7feaad2e93df31d701780a', 'crashpad/third_party/zlib/zlib': Var('chromium_git') + '/chromium/src/third_party/zlib@' + '13dc246a58e4b72104d35f9b1809af95221ebda7', diff --git a/minidump/minidump_context.h b/minidump/minidump_context.h index b71b57cc..f558832f 100644 --- a/minidump/minidump_context.h +++ b/minidump/minidump_context.h @@ -252,7 +252,7 @@ enum MinidumpContextAMD64Flags : uint32_t { //! normally alias `dr6` and `dr7`, respectively. See Intel Software //! Developer’s Manual, Volume 3B: System Programming, Part 2 (253669-052), //! 17.2.2 “Debug Registers DR4 and DR5”. -struct ALIGNAS(16) MinidumpContextAMD64 { +struct alignas(16) MinidumpContextAMD64 { //! \brief Register parameter home address. //! //! On Windows, this field may contain the “home” address (on-stack, in the diff --git a/util/stdlib/aligned_allocator.h b/util/stdlib/aligned_allocator.h index 04d3dc46..91012551 100644 --- a/util/stdlib/aligned_allocator.h +++ b/util/stdlib/aligned_allocator.h @@ -55,7 +55,7 @@ void AlignedFree(void* pointer); //! This is similar to `std::allocator`, with the addition of an alignment //! guarantee. \a Alignment must be a power of 2. If \a Alignment is not //! specified, the default alignment for type \a T is used. -template +template struct AlignedAllocator { public: using value_type = T; @@ -130,7 +130,7 @@ bool operator!=(const AlignedAllocator& lhs, //! This is similar to `std::vector`, with the addition of an alignment //! guarantee. \a Alignment must be a power of 2. If \a Alignment is not //! specified, the default alignment for type \a T is used. -template +template using AlignedVector = std::vector>; } // namespace crashpad diff --git a/util/stdlib/aligned_allocator_test.cc b/util/stdlib/aligned_allocator_test.cc index 66ee6e6c..2799558e 100644 --- a/util/stdlib/aligned_allocator_test.cc +++ b/util/stdlib/aligned_allocator_test.cc @@ -40,30 +40,30 @@ TEST(AlignedAllocator, AlignedVector) { AlignedVector natural_aligned_vector; natural_aligned_vector.push_back(NaturalAlignedStruct()); EXPECT_TRUE( - IsAligned(&natural_aligned_vector[0], ALIGNOF(NaturalAlignedStruct))); + IsAligned(&natural_aligned_vector[0], alignof(NaturalAlignedStruct))); natural_aligned_vector.resize(3); EXPECT_TRUE( - IsAligned(&natural_aligned_vector[0], ALIGNOF(NaturalAlignedStruct))); + IsAligned(&natural_aligned_vector[0], alignof(NaturalAlignedStruct))); EXPECT_TRUE( - IsAligned(&natural_aligned_vector[1], ALIGNOF(NaturalAlignedStruct))); + IsAligned(&natural_aligned_vector[1], alignof(NaturalAlignedStruct))); EXPECT_TRUE( - IsAligned(&natural_aligned_vector[2], ALIGNOF(NaturalAlignedStruct))); + IsAligned(&natural_aligned_vector[2], alignof(NaturalAlignedStruct))); // Test a structure that declares its own alignment. - struct ALIGNAS(32) AlignedStruct { + struct alignas(32) AlignedStruct { int i; }; - ASSERT_EQ(ALIGNOF(AlignedStruct), 32u); + ASSERT_EQ(alignof(AlignedStruct), 32u); AlignedVector aligned_vector; aligned_vector.push_back(AlignedStruct()); - EXPECT_TRUE(IsAligned(&aligned_vector[0], ALIGNOF(AlignedStruct))); + EXPECT_TRUE(IsAligned(&aligned_vector[0], alignof(AlignedStruct))); aligned_vector.resize(3); - EXPECT_TRUE(IsAligned(&aligned_vector[0], ALIGNOF(AlignedStruct))); - EXPECT_TRUE(IsAligned(&aligned_vector[1], ALIGNOF(AlignedStruct))); - EXPECT_TRUE(IsAligned(&aligned_vector[2], ALIGNOF(AlignedStruct))); + EXPECT_TRUE(IsAligned(&aligned_vector[0], alignof(AlignedStruct))); + EXPECT_TRUE(IsAligned(&aligned_vector[1], alignof(AlignedStruct))); + EXPECT_TRUE(IsAligned(&aligned_vector[2], alignof(AlignedStruct))); // Try a custom alignment. Since the structure itself doesn’t specify an // alignment constraint, only the base address will be aligned to the @@ -73,19 +73,19 @@ TEST(AlignedAllocator, AlignedVector) { EXPECT_TRUE(IsAligned(&custom_aligned_vector[0], 64)); // Try a structure with a pretty big alignment request. - struct ALIGNAS(1024) BigAlignedStruct { + struct alignas(1024) BigAlignedStruct { int i; }; - ASSERT_EQ(ALIGNOF(BigAlignedStruct), 1024u); + ASSERT_EQ(alignof(BigAlignedStruct), 1024u); AlignedVector big_aligned_vector; big_aligned_vector.push_back(BigAlignedStruct()); - EXPECT_TRUE(IsAligned(&big_aligned_vector[0], ALIGNOF(BigAlignedStruct))); + EXPECT_TRUE(IsAligned(&big_aligned_vector[0], alignof(BigAlignedStruct))); big_aligned_vector.resize(3); - EXPECT_TRUE(IsAligned(&big_aligned_vector[0], ALIGNOF(BigAlignedStruct))); - EXPECT_TRUE(IsAligned(&big_aligned_vector[1], ALIGNOF(BigAlignedStruct))); - EXPECT_TRUE(IsAligned(&big_aligned_vector[2], ALIGNOF(BigAlignedStruct))); + EXPECT_TRUE(IsAligned(&big_aligned_vector[0], alignof(BigAlignedStruct))); + EXPECT_TRUE(IsAligned(&big_aligned_vector[1], alignof(BigAlignedStruct))); + EXPECT_TRUE(IsAligned(&big_aligned_vector[2], alignof(BigAlignedStruct))); } void BadAlignmentTest() { From f8457977326f405517d246d395b17caca320d933 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 13 Jun 2017 14:00:36 -0400 Subject: [PATCH 24/32] mac: 10.13 SDK compatibility, adapt to x86_state_hdr changes In the 10.12 SDK, x86_state_hdr from was defined as: struct x86_state_hdr { int flavor; int count; }; This has changed in the 10.13 SDK to: struct x86_state_hdr { uint32_t flavor; uint32_t count; }; This triggers signedness mismatch errors where these values are used with CHECK/DCHECK macros and gtest EXPECT/ASSERT macros. Compatibility with existing and new SDKs must be maintained, so more casts must be used. Bug: crashpad:185, crashpad:188 Change-Id: I8844d6a78520430a8b5b90a35403896c3c6cfa37 Reviewed-on: https://chromium-review.googlesource.com/533375 Reviewed-by: Robert Sesek Commit-Queue: Mark Mentovai --- client/capture_context_mac_test.cc | 12 ++++++++---- client/simulate_crash_mac.cc | 4 ++-- client/simulate_crash_mac_test.cc | 24 ++++++++++++------------ 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/client/capture_context_mac_test.cc b/client/capture_context_mac_test.cc index ca52961e..15640210 100644 --- a/client/capture_context_mac_test.cc +++ b/client/capture_context_mac_test.cc @@ -35,11 +35,15 @@ namespace { // gtest assertions. void SanityCheckContext(const NativeCPUContext& context) { #if defined(ARCH_CPU_X86) - ASSERT_EQ(context.tsh.flavor, x86_THREAD_STATE32); - ASSERT_EQ(context.tsh.count, implicit_cast(x86_THREAD_STATE32_COUNT)); + ASSERT_EQ(implicit_cast(context.tsh.flavor), + implicit_cast(x86_THREAD_STATE32)); + ASSERT_EQ(implicit_cast(context.tsh.count), + implicit_cast(x86_THREAD_STATE32_COUNT)); #elif defined(ARCH_CPU_X86_64) - ASSERT_EQ(context.tsh.flavor, x86_THREAD_STATE64); - ASSERT_EQ(context.tsh.count, implicit_cast(x86_THREAD_STATE64_COUNT)); + ASSERT_EQ(implicit_cast(context.tsh.flavor), + implicit_cast(x86_THREAD_STATE64)); + ASSERT_EQ(implicit_cast(context.tsh.count), + implicit_cast(x86_THREAD_STATE64_COUNT)); #endif #if defined(ARCH_CPU_X86_FAMILY) diff --git a/client/simulate_crash_mac.cc b/client/simulate_crash_mac.cc index 7e279015..27864388 100644 --- a/client/simulate_crash_mac.cc +++ b/client/simulate_crash_mac.cc @@ -177,12 +177,12 @@ bool DeliverException(thread_t thread, void SimulateCrash(const NativeCPUContext& cpu_context) { #if defined(ARCH_CPU_X86) - DCHECK_EQ(cpu_context.tsh.flavor, + DCHECK_EQ(implicit_cast(cpu_context.tsh.flavor), implicit_cast(x86_THREAD_STATE32)); DCHECK_EQ(implicit_cast(cpu_context.tsh.count), x86_THREAD_STATE32_COUNT); #elif defined(ARCH_CPU_X86_64) - DCHECK_EQ(cpu_context.tsh.flavor, + DCHECK_EQ(implicit_cast(cpu_context.tsh.flavor), implicit_cast(x86_THREAD_STATE64)); DCHECK_EQ(implicit_cast(cpu_context.tsh.count), x86_THREAD_STATE64_COUNT); diff --git a/client/simulate_crash_mac_test.cc b/client/simulate_crash_mac_test.cc index de91ebb2..c1953524 100644 --- a/client/simulate_crash_mac_test.cc +++ b/client/simulate_crash_mac_test.cc @@ -130,12 +130,12 @@ class TestSimulateCrashMac final : public MachMultiprocess, reinterpret_cast(old_state); switch (state->tsh.flavor) { case x86_THREAD_STATE32: - EXPECT_EQ(state->tsh.count, - implicit_cast(x86_THREAD_STATE32_COUNT)); + EXPECT_EQ(implicit_cast(state->tsh.count), + implicit_cast(x86_THREAD_STATE32_COUNT)); break; case x86_THREAD_STATE64: - EXPECT_EQ(state->tsh.count, - implicit_cast(x86_THREAD_STATE64_COUNT)); + EXPECT_EQ(implicit_cast(state->tsh.count), + implicit_cast(x86_THREAD_STATE64_COUNT)); break; default: ADD_FAILURE() << "unexpected tsh.flavor " << state->tsh.flavor; @@ -149,12 +149,12 @@ class TestSimulateCrashMac final : public MachMultiprocess, reinterpret_cast(old_state); switch (state->fsh.flavor) { case x86_FLOAT_STATE32: - EXPECT_EQ(state->fsh.count, - implicit_cast(x86_FLOAT_STATE32_COUNT)); + EXPECT_EQ(implicit_cast(state->fsh.count), + implicit_cast(x86_FLOAT_STATE32_COUNT)); break; case x86_FLOAT_STATE64: - EXPECT_EQ(state->fsh.count, - implicit_cast(x86_FLOAT_STATE64_COUNT)); + EXPECT_EQ(implicit_cast(state->fsh.count), + implicit_cast(x86_FLOAT_STATE64_COUNT)); break; default: ADD_FAILURE() << "unexpected fsh.flavor " << state->fsh.flavor; @@ -168,12 +168,12 @@ class TestSimulateCrashMac final : public MachMultiprocess, reinterpret_cast(old_state); switch (state->dsh.flavor) { case x86_DEBUG_STATE32: - EXPECT_EQ(state->dsh.count, - implicit_cast(x86_DEBUG_STATE32_COUNT)); + EXPECT_EQ(implicit_cast(state->dsh.count), + implicit_cast(x86_DEBUG_STATE32_COUNT)); break; case x86_DEBUG_STATE64: - EXPECT_EQ(state->dsh.count, - implicit_cast(x86_DEBUG_STATE64_COUNT)); + EXPECT_EQ(implicit_cast(state->dsh.count), + implicit_cast(x86_DEBUG_STATE64_COUNT)); break; default: ADD_FAILURE() << "unexpected dsh.flavor " << state->dsh.flavor; From 2851e5cfc8619d9c9a87a84dcc8d36c106de9bdb Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 13 Jun 2017 14:55:17 -0400 Subject: [PATCH 25/32] mac: Update cl_kernels workaround for macOS 10.13 (and later) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since Apple closed https://openradar.appspot.com/20239912 without fixing anything, it looks like we’ll be stuck with these quriky cl_kernels modules for quite some time. Allow these modules to be tolerated on any OS version >= 10.10, where they first appeared in a broken state, by removing the upper bound for the OS version to tolerate with this quirk. The tolerance was previously expanded to include 10.11 in cd1f8fa3d2f2c76802952beac71ad85f51bbf771 and 10.12 in 6fe7c5414e46acfa30e8984513bf0896e91b9407. After this third update, this should hopefully no longer be an annual exercise. Bug: crashpad:185, crashpad:186 Change-Id: I66d409f2d1638bcf7601b6622f000be245230f34 Reviewed-on: https://chromium-review.googlesource.com/534253 Reviewed-by: Robert Sesek Commit-Queue: Mark Mentovai --- snapshot/mac/mach_o_image_segment_reader.cc | 53 +++++++++------------ 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/snapshot/mac/mach_o_image_segment_reader.cc b/snapshot/mac/mach_o_image_segment_reader.cc index 9837a9c5..06e1daf4 100644 --- a/snapshot/mac/mach_o_image_segment_reader.cc +++ b/snapshot/mac/mach_o_image_segment_reader.cc @@ -120,36 +120,29 @@ bool MachOImageSegmentReader::Initialize(ProcessReader* process_reader, sections_.size(), load_command_info.c_str()); - if (section_segment_name != segment_name) { - // cl_kernels modules (for OpenCL) aren’t ld output, and they’re formatted - // incorrectly on OS X 10.10 and later. They have a single __TEXT segment, - // but one of the sections within it claims to belong to the __LD segment. - // This mismatch shouldn’t happen. This errant section also has the - // S_ATTR_DEBUG flag set, which shouldn’t happen unless all of the other - // sections in the segment also have this bit set (they don’t). These odd - // sections are reminiscent of unwind information stored in MH_OBJECT - // images, although cl_kernels images claim to be MH_BUNDLE. Because at - // least one cl_kernels module will commonly be found in a process, and - // sometimes more will be, tolerate this quirk. - // - // https://openradar.appspot.com/20239912 - bool ok = false; - if (file_type == MH_BUNDLE && module_name == "cl_kernels") { - int mac_os_x_minor_version = MacOSXMinorVersion(); - if ((mac_os_x_minor_version >= 10 && mac_os_x_minor_version <= 12) && - segment_name == SEG_TEXT && - section_segment_name == "__LD" && - section_name == "__compact_unwind" && - (section.flags & S_ATTR_DEBUG)) { - ok = true; - } - } - - if (!ok) { - LOG(WARNING) << "section.segname incorrect in segment " << segment_name - << section_info; - return false; - } + // cl_kernels modules (for OpenCL) aren’t ld output, and they’re formatted + // incorrectly on OS X 10.10 and later. They have a single __TEXT segment, + // but one of the sections within it claims to belong to the __LD segment. + // This mismatch shouldn’t happen. This errant section also has the + // S_ATTR_DEBUG flag set, which shouldn’t happen unless all of the other + // sections in the segment also have this bit set (they don’t). These odd + // sections are reminiscent of unwind information stored in MH_OBJECT + // images, although cl_kernels images claim to be MH_BUNDLE. Because at + // least one cl_kernels module will commonly be found in a process, and + // sometimes more will be, tolerate this quirk. + // + // https://openradar.appspot.com/20239912 + if (section_segment_name != segment_name && + !(file_type == MH_BUNDLE && + module_name == "cl_kernels" && + MacOSXMinorVersion() >= 10 && + segment_name == SEG_TEXT && + section_segment_name == "__LD" && + section_name == "__compact_unwind" && + (section.flags & S_ATTR_DEBUG))) { + LOG(WARNING) << "section.segname incorrect in segment " << segment_name + << section_info; + return false; } CheckedMachAddressRange section_range( From 107fb76317885000081ed9cefd6f9a3c504fb7b5 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 14 Jun 2017 10:48:30 -0400 Subject: [PATCH 26/32] mac: Handle _dyld_get_all_image_infos() not being available on 10.13 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _dyld_get_all_image_infos() was only used in test code in Crashpad. This addresses two related problems. When running on 10.13 or later, _dyld_get_all_image_infos() is not available. It appears to still be implemented in dyld, but its symbol is now private. This was always known to be an “internal” interface. When it’s not available, fall back to obtaining the address of the process’ dyld_all_image_infos structure by calling task_info(…, TASK_DYLD_INFO, …). Note that this is the same thing that the code being tested does, although the tests are not rendered entirely pointless because the code being tested consumes dyld_all_image_infos through its own implementation of an out-of-process reader interface, while the dyld_all_image_infos data obtained by _dyld_get_all_image_infos() is handled strictly in-process by ordinary memory reads. This is covered by bug 187. When building with the 10.13 SDK, no _dyld_get_all_image_infos symbol is available to link against. In this case, access the symbol strictly at runtime via dlopen() if it may be available, or when expecting to only run on 10.13 and later, don’t even bother looking for this symbol. This is covered by part of bug 188. Bug: crashpad:185, crashpad:187, crashpad:188 Change-Id: Ib283e070faf5d1ec35deee420213b53ec24fb1d3 Reviewed-on: https://chromium-review.googlesource.com/534633 Reviewed-by: Robert Sesek --- compat/mac/AvailabilityMacros.h | 6 ++ snapshot/crashpad_info_client_options_test.cc | 59 ++--------- snapshot/mac/mach_o_image_reader_test.cc | 3 +- snapshot/mac/process_reader.cc | 82 ++++++++------- snapshot/mac/process_reader.h | 15 +++ snapshot/mac/process_reader_test.cc | 13 ++- snapshot/mac/process_types_test.cc | 3 +- test/mac/dyld.cc | 99 +++++++++++++++++++ test/mac/dyld.h | 16 +-- test/scoped_module_handle.cc | 46 +++++++++ test/scoped_module_handle.h | 81 +++++++++++++++ test/test.gyp | 4 + 12 files changed, 326 insertions(+), 101 deletions(-) create mode 100644 test/mac/dyld.cc create mode 100644 test/scoped_module_handle.cc create mode 100644 test/scoped_module_handle.h diff --git a/compat/mac/AvailabilityMacros.h b/compat/mac/AvailabilityMacros.h index 4037eca6..f105f944 100644 --- a/compat/mac/AvailabilityMacros.h +++ b/compat/mac/AvailabilityMacros.h @@ -53,4 +53,10 @@ #define MAC_OS_X_VERSION_10_12 101200 #endif +// 10.13 SDK + +#ifndef MAC_OS_X_VERSION_10_13 +#define MAC_OS_X_VERSION_10_13 101300 +#endif + #endif // CRASHPAD_COMPAT_MAC_AVAILABILITYMACROS_H_ diff --git a/snapshot/crashpad_info_client_options_test.cc b/snapshot/crashpad_info_client_options_test.cc index 53d4f62e..697c14ec 100644 --- a/snapshot/crashpad_info_client_options_test.cc +++ b/snapshot/crashpad_info_client_options_test.cc @@ -21,6 +21,7 @@ #include "client/crashpad_info.h" #include "gtest/gtest.h" #include "test/errors.h" +#include "test/scoped_module_handle.h" #include "test/test_paths.h" #if defined(OS_MACOSX) @@ -138,48 +139,6 @@ TEST(CrashpadInfoClientOptions, OneModule) { } } -#if defined(OS_POSIX) -using DlHandle = void*; -#elif defined(OS_WIN) -using DlHandle = HMODULE; -#endif // OS_POSIX - -class ScopedDlHandle { - public: - explicit ScopedDlHandle(DlHandle dl_handle) - : dl_handle_(dl_handle) { - } - - ~ScopedDlHandle() { - if (dl_handle_) { -#if defined(OS_POSIX) - if (dlclose(dl_handle_) != 0) { - LOG(ERROR) << "dlclose: " << dlerror(); - } -#elif defined(OS_WIN) - if (!FreeLibrary(dl_handle_)) - PLOG(ERROR) << "FreeLibrary"; -#endif // OS_POSIX - } - } - - bool valid() const { return dl_handle_ != nullptr; } - - template - T LookUpSymbol(const char* symbol_name) { -#if defined(OS_POSIX) - return reinterpret_cast(dlsym(dl_handle_, symbol_name)); -#elif defined(OS_WIN) - return reinterpret_cast(GetProcAddress(dl_handle_, symbol_name)); -#endif // OS_POSIX - } - - private: - DlHandle dl_handle_; - - DISALLOW_COPY_AND_ASSIGN(ScopedDlHandle); -}; - TEST(CrashpadInfoClientOptions, TwoModules) { // Open the module, which has its own CrashpadInfo structure. #if defined(OS_MACOSX) @@ -190,15 +149,15 @@ TEST(CrashpadInfoClientOptions, TwoModules) { base::FilePath module_path = TestPaths::Executable().DirName().Append( FILE_PATH_LITERAL("crashpad_snapshot_test_module") + kDlExtension); #if defined(OS_MACOSX) - ScopedDlHandle dl_handle( + ScopedModuleHandle module( dlopen(module_path.value().c_str(), RTLD_LAZY | RTLD_LOCAL)); - ASSERT_TRUE(dl_handle.valid()) << "dlopen " << module_path.value() << ": " - << dlerror(); + ASSERT_TRUE(module.valid()) << "dlopen " << module_path.value() << ": " + << dlerror(); #elif defined(OS_WIN) - ScopedDlHandle dl_handle(LoadLibrary(module_path.value().c_str())); - ASSERT_TRUE(dl_handle.valid()) - << "LoadLibrary " << base::UTF16ToUTF8(module_path.value()) << ": " - << ErrorMessage(); + ScopedModuleHandle module(LoadLibrary(module_path.value().c_str())); + ASSERT_TRUE(module.valid()) << "LoadLibrary " + << base::UTF16ToUTF8(module_path.value()) << ": " + << ErrorMessage(); #else #error Port. #endif // OS_MACOSX @@ -207,7 +166,7 @@ TEST(CrashpadInfoClientOptions, TwoModules) { // because it runs in the module, it returns the remote module’s CrashpadInfo // structure. CrashpadInfo* (*TestModule_GetCrashpadInfo)() = - dl_handle.LookUpSymbol("TestModule_GetCrashpadInfo"); + module.LookUpSymbol("TestModule_GetCrashpadInfo"); ASSERT_TRUE(TestModule_GetCrashpadInfo); auto options = SelfProcessSnapshotAndGetCrashpadOptions(); diff --git a/snapshot/mac/mach_o_image_reader_test.cc b/snapshot/mac/mach_o_image_reader_test.cc index 6c545bc1..6d6e49d0 100644 --- a/snapshot/mac/mach_o_image_reader_test.cc +++ b/snapshot/mac/mach_o_image_reader_test.cc @@ -577,8 +577,7 @@ TEST(MachOImageReader, Self_DyldImages) { // Now that all of the modules have been verified, make sure that dyld itself // can be read properly too. - const struct dyld_all_image_infos* dyld_image_infos = - _dyld_get_all_image_infos(); + const dyld_all_image_infos* dyld_image_infos = DyldGetAllImageInfos(); ASSERT_GE(dyld_image_infos->version, 1u); EXPECT_EQ(dyld_image_infos->infoArrayCount, count); diff --git a/snapshot/mac/process_reader.cc b/snapshot/mac/process_reader.cc index 46083ab3..5e1f1c7b 100644 --- a/snapshot/mac/process_reader.cc +++ b/snapshot/mac/process_reader.cc @@ -198,6 +198,46 @@ const std::vector& ProcessReader::Modules() { return modules_; } +mach_vm_address_t ProcessReader::DyldAllImageInfo( + mach_vm_size_t* all_image_info_size) { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + + task_dyld_info_data_t dyld_info; + mach_msg_type_number_t count = TASK_DYLD_INFO_COUNT; + kern_return_t kr = task_info( + task_, TASK_DYLD_INFO, reinterpret_cast(&dyld_info), &count); + if (kr != KERN_SUCCESS) { + MACH_LOG(WARNING, kr) << "task_info"; + return 0; + } + + // TODO(mark): Deal with statically linked executables which don’t use dyld. + // This may look for the module that matches the executable path in the same + // data set that vmmap uses. + +#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7 + // The task_dyld_info_data_t struct grew in 10.7, adding the format field. + // Don’t check this field if it’s not present, which can happen when either + // the SDK used at compile time or the kernel at run time are too old and + // don’t know about it. + if (count >= TASK_DYLD_INFO_COUNT) { + const integer_t kExpectedFormat = + !Is64Bit() ? TASK_DYLD_ALL_IMAGE_INFO_32 : TASK_DYLD_ALL_IMAGE_INFO_64; + if (dyld_info.all_image_info_format != kExpectedFormat) { + LOG(WARNING) << "unexpected task_dyld_info_data_t::all_image_info_format " + << dyld_info.all_image_info_format; + DCHECK_EQ(dyld_info.all_image_info_format, kExpectedFormat); + return 0; + } + } +#endif + + if (all_image_info_size) { + *all_image_info_size = dyld_info.all_image_info_size; + } + return dyld_info.all_image_info_addr; +} + void ProcessReader::InitializeThreads() { DCHECK(!initialized_threads_); DCHECK(threads_.empty()); @@ -345,38 +385,12 @@ void ProcessReader::InitializeModules() { initialized_modules_ = true; - task_dyld_info_data_t dyld_info; - mach_msg_type_number_t count = TASK_DYLD_INFO_COUNT; - kern_return_t kr = task_info( - task_, TASK_DYLD_INFO, reinterpret_cast(&dyld_info), &count); - if (kr != KERN_SUCCESS) { - MACH_LOG(WARNING, kr) << "task_info"; - return; - } - - // TODO(mark): Deal with statically linked executables which don’t use dyld. - // This may look for the module that matches the executable path in the same - // data set that vmmap uses. - -#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7 - // The task_dyld_info_data_t struct grew in 10.7, adding the format field. - // Don’t check this field if it’s not present, which can happen when either - // the SDK used at compile time or the kernel at run time are too old and - // don’t know about it. - if (count >= TASK_DYLD_INFO_COUNT) { - const integer_t kExpectedFormat = - !Is64Bit() ? TASK_DYLD_ALL_IMAGE_INFO_32 : TASK_DYLD_ALL_IMAGE_INFO_64; - if (dyld_info.all_image_info_format != kExpectedFormat) { - LOG(WARNING) << "unexpected task_dyld_info_data_t::all_image_info_format " - << dyld_info.all_image_info_format; - DCHECK_EQ(dyld_info.all_image_info_format, kExpectedFormat); - return; - } - } -#endif + mach_vm_size_t all_image_info_size; + mach_vm_address_t all_image_info_addr = + DyldAllImageInfo(&all_image_info_size); process_types::dyld_all_image_infos all_image_infos; - if (!all_image_infos.Read(this, dyld_info.all_image_info_addr)) { + if (!all_image_infos.Read(this, all_image_info_addr)) { LOG(WARNING) << "could not read dyld_all_image_infos"; return; } @@ -390,10 +404,10 @@ void ProcessReader::InitializeModules() { size_t expected_size = process_types::dyld_all_image_infos::ExpectedSizeForVersion( this, all_image_infos.version); - if (dyld_info.all_image_info_size < expected_size) { - LOG(WARNING) << "small dyld_all_image_infos size " - << dyld_info.all_image_info_size << " < " << expected_size - << " for version " << all_image_infos.version; + if (all_image_info_size < expected_size) { + LOG(WARNING) << "small dyld_all_image_infos size " << all_image_info_size + << " < " << expected_size << " for version " + << all_image_infos.version; return; } diff --git a/snapshot/mac/process_reader.h b/snapshot/mac/process_reader.h index 72bd1dc6..ddb49cb6 100644 --- a/snapshot/mac/process_reader.h +++ b/snapshot/mac/process_reader.h @@ -150,6 +150,21 @@ class ProcessReader { //! corresponds to the dynamic loader, dyld. const std::vector& Modules(); + //! \brief Determines the location of the `dyld_all_image_infos` structure in + //! the process’ address space. + //! + //! This function is an internal implementation detail of Modules(), and + //! should not normally be used directly. It is exposed solely for use by test + //! code. + //! + //! \param[out] all_image_info_size The size of the `dyld_all_image_infos` + //! structure. Optional, may be `nullptr` if not required. + //! + //! \return The address of the `dyld_all_image_infos` structure in the + //! process’ address space, with \a all_image_info_size set appropriately. + //! On failure, returns `0` with a message logged. + mach_vm_address_t DyldAllImageInfo(mach_vm_size_t* all_image_info_size); + private: //! Performs lazy initialization of the \a threads_ vector on behalf of //! Threads(). diff --git a/snapshot/mac/process_reader_test.cc b/snapshot/mac/process_reader_test.cc index 3554326c..3a6a1c8b 100644 --- a/snapshot/mac/process_reader_test.cc +++ b/snapshot/mac/process_reader_test.cc @@ -44,12 +44,13 @@ #include "util/stdlib/pointer_container.h" #include "util/synchronization/semaphore.h" -#if !defined(MAC_OS_X_VERSION_10_10) || \ - MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_10 +#if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_10 extern "C" { -// Redeclare a typedef whose availability (OSX 10.10) is newer than the + +// Redeclare a typedef whose availability (OS X 10.10) is newer than the // deployment target. typedef struct _cl_device_id* cl_device_id; + } // extern "C" #endif @@ -698,8 +699,7 @@ TEST(ProcessReader, SelfModules) { // is also reported as 0. EXPECT_EQ(modules[index].timestamp, 0); - const struct dyld_all_image_infos* dyld_image_infos = - _dyld_get_all_image_infos(); + const dyld_all_image_infos* dyld_image_infos = DyldGetAllImageInfos(); if (dyld_image_infos->version >= 2) { ASSERT_TRUE(modules[index].reader); EXPECT_EQ(modules[index].reader->Address(), @@ -781,8 +781,7 @@ class ProcessReaderModulesChild final : public MachMultiprocess { FileHandle write_handle = WritePipeHandle(); uint32_t dyld_image_count = _dyld_image_count(); - const struct dyld_all_image_infos* dyld_image_infos = - _dyld_get_all_image_infos(); + const dyld_all_image_infos* dyld_image_infos = DyldGetAllImageInfos(); uint32_t write_image_count = dyld_image_count; if (dyld_image_infos->version >= 2) { diff --git a/snapshot/mac/process_types_test.cc b/snapshot/mac/process_types_test.cc index 18595f02..4f5ce7af 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -46,8 +46,7 @@ namespace { TEST(ProcessTypes, DyldImagesSelf) { // Get the in-process view of dyld_all_image_infos, and check it for sanity. - const struct dyld_all_image_infos* self_image_infos = - _dyld_get_all_image_infos(); + const dyld_all_image_infos* self_image_infos = DyldGetAllImageInfos(); int mac_os_x_minor_version = MacOSXMinorVersion(); if (mac_os_x_minor_version >= 12) { EXPECT_GE(self_image_infos->version, 15u); diff --git a/test/mac/dyld.cc b/test/mac/dyld.cc new file mode 100644 index 00000000..6fa2176a --- /dev/null +++ b/test/mac/dyld.cc @@ -0,0 +1,99 @@ +// 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/mac/dyld.h" + +#include +#include +#include +#include +#include + +#include "base/logging.h" +#include "snapshot/mac/process_reader.h" +#include "test/scoped_module_handle.h" +#include "util/numeric/safe_assignment.h" + +#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_13 +extern "C" { + +// A non-public dyld API, declared in 10.12.4 +// dyld-433.5/include/mach-o/dyld_priv.h. The code still exists in 10.13, but +// its symbol is no longer public, so it can’t be used there. +const dyld_all_image_infos* _dyld_get_all_image_infos() + __attribute__((weak_import)); + +} // extern "C" +#endif + +namespace crashpad { +namespace test { + +const dyld_all_image_infos* DyldGetAllImageInfos() { +#if MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_13 + // When building with the pre-10.13 SDK, the weak_import declaration above is + // available and a symbol will be present in the SDK to link against. If the + // old interface is also available at run time (running on pre-10.13), use it. + if (_dyld_get_all_image_infos) { + return _dyld_get_all_image_infos(); + } +#elif MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_13 + // When building with the 10.13 SDK or later, but able to run on pre-10.13, + // look for _dyld_get_all_image_infos in the same module that provides + // _dyld_image_count. There’s no symbol in the SDK to link against, so this is + // a little more involved than the pre-10.13 SDK case above. + Dl_info dli; + if (!dladdr(reinterpret_cast(_dyld_image_count), &dli)) { + LOG(WARNING) << "dladdr: failed"; + } else { + ScopedModuleHandle module( + dlopen(dli.dli_fname, RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD)); + if (!module.valid()) { + LOG(WARNING) << "dlopen: " << dlerror(); + } else { + using DyldGetAllImageInfosType = const dyld_all_image_infos*(*)(); + const auto _dyld_get_all_image_infos = + module.LookUpSymbol( + "_dyld_get_all_image_infos"); + if (_dyld_get_all_image_infos) { + return _dyld_get_all_image_infos(); + } + } + } +#endif + + // On 10.13 and later, do it the hard way. + ProcessReader process_reader; + if (!process_reader.Initialize(mach_task_self())) { + return nullptr; + } + + mach_vm_address_t all_image_info_addr_m = + process_reader.DyldAllImageInfo(nullptr); + if (!all_image_info_addr_m) { + return nullptr; + } + + uintptr_t all_image_info_addr_u; + if (!AssignIfInRange(&all_image_info_addr_u, all_image_info_addr_m)) { + LOG(ERROR) << "all_image_info_addr_m " << all_image_info_addr_m + << " out of range"; + return nullptr; + } + + return reinterpret_cast(all_image_info_addr_u); +} + +} // namespace test +} // namespace crashpad diff --git a/test/mac/dyld.h b/test/mac/dyld.h index e436c5d1..0a688f3e 100644 --- a/test/mac/dyld.h +++ b/test/mac/dyld.h @@ -17,13 +17,17 @@ #include -extern "C" { +namespace crashpad { +namespace test { -// Returns a pointer to this process’ dyld_all_image_infos structure. This is -// implemented as a non-public dyld API, declared in 10.9.2 -// dyld-239.4/include/mach-o/dyld_priv.h. -const struct dyld_all_image_infos* _dyld_get_all_image_infos(); +//! \brief Calls or emulates the `_dyld_get_all_image_infos()` private/internal +//! function. +//! +//! \return A pointer to this process’ dyld_all_image_infos structure, or +//! `nullptr` on failure with a message logged. +const dyld_all_image_infos* DyldGetAllImageInfos(); -} // extern "C" +} // namespace test +} // namespace crashpad #endif // CRASHPAD_TEST_MAC_DYLD_H_ diff --git a/test/scoped_module_handle.cc b/test/scoped_module_handle.cc new file mode 100644 index 00000000..3854222b --- /dev/null +++ b/test/scoped_module_handle.cc @@ -0,0 +1,46 @@ +// 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/scoped_module_handle.h" + +#include "base/logging.h" + +namespace crashpad { +namespace test { + +// static +void ScopedModuleHandle::Impl::Close(ModuleHandle handle) { +#if defined(OS_POSIX) + if (dlclose(handle) != 0) { + LOG(ERROR) << "dlclose: " << dlerror(); + } +#elif defined(OS_WIN) + if (!FreeLibrary(handle)) { + PLOG(ERROR) << "FreeLibrary"; + } +#else +#error Port +#endif +} + +ScopedModuleHandle::ScopedModuleHandle(ModuleHandle handle) : handle_(handle) {} + +ScopedModuleHandle::~ScopedModuleHandle() { + if (valid()) { + Impl::Close(handle_); + } +} + +} // namespace test +} // namespace crashpad diff --git a/test/scoped_module_handle.h b/test/scoped_module_handle.h new file mode 100644 index 00000000..3863cad3 --- /dev/null +++ b/test/scoped_module_handle.h @@ -0,0 +1,81 @@ +// 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_SCOPED_MODULE_HANDLE_H_ +#define CRASHPAD_TEST_SCOPED_MODULE_HANDLE_H_ + +#include "base/macros.h" +#include "build/build_config.h" + +#if defined(OS_POSIX) +#include +#elif defined(OS_WIN) +#include +#endif + +namespace crashpad { +namespace test { + +//! \brief Maintains ownership of a loadable module handle, releasing it as +//! appropriate on destruction. +class ScopedModuleHandle { + private: + class Impl { + public: +#if defined(OS_POSIX) + using ModuleHandle = void*; + + static void* LookUpSymbol(ModuleHandle handle, const char* symbol_name) { + return dlsym(handle, symbol_name); + } +#elif defined(OS_WIN) + using ModuleHandle = HMODULE; + + static void* LookUpSymbol(ModuleHandle handle, const char* symbol_name) { + return GetProcAddress(handle, symbol_name); + } +#endif + + static void Close(ModuleHandle handle); + + private: + DISALLOW_IMPLICIT_CONSTRUCTORS(Impl); + }; + + public: + using ModuleHandle = Impl::ModuleHandle; + + explicit ScopedModuleHandle(ModuleHandle handle); + ~ScopedModuleHandle(); + + //! \return `true` if this object manages a valid loadable module handle. + bool valid() const { return handle_ != nullptr; } + + //! \return The value of the symbol named by \a symbol_name, or `nullptr` on + //! failure. + template + T LookUpSymbol(const char* symbol_name) const { + return reinterpret_cast(Impl::LookUpSymbol(handle_, symbol_name)); + } + + private: + ModuleHandle handle_; + + DISALLOW_COPY_AND_ASSIGN(ScopedModuleHandle); +}; + +} // namespace test +} // namespace crashpad + +#endif // CRASHPAD_TEST_SCOPED_MODULE_HANDLE_H_ diff --git a/test/test.gyp b/test/test.gyp index 94b198c7..f2682c02 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -22,6 +22,7 @@ 'type': 'static_library', 'dependencies': [ '../compat/compat.gyp:crashpad_compat', + '../snapshot/snapshot.gyp:crashpad_snapshot', '../third_party/gtest/gtest.gyp:gtest', '../third_party/mini_chromium/mini_chromium.gyp:base', '../util/util.gyp:crashpad_util', @@ -37,6 +38,7 @@ 'gtest_death_check.h', 'hex_string.cc', 'hex_string.h', + 'mac/dyld.cc', 'mac/dyld.h', 'mac/mach_errors.cc', 'mac/mach_errors.h', @@ -49,6 +51,8 @@ 'multiprocess_exec_posix.cc', 'multiprocess_exec_win.cc', 'multiprocess_posix.cc', + 'scoped_module_handle.cc', + 'scoped_module_handle.h', 'scoped_temp_dir.cc', 'scoped_temp_dir.h', 'scoped_temp_dir_posix.cc', From 6108d2523297a7f3f0ea714b4e6229325f5396ce Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 14 Jun 2017 13:24:35 -0400 Subject: [PATCH 27/32] mac: Update the process_types version of dyld_all_image_infos for 10.13 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 10.13 introduces two new fields to dyld_all_image_infos. Oddly, it doesn’t put them in the “reserved” area that was defined in this structure. This addition made it necessary for the padding problem in the 32-bit structure previously worked around in Crashpad to be addressed in the native structure, so Crashpad’s definition is adapted to match. This fixes tests on 10.13 that verify that dyld_all_image_infos can be interpreted correctly. Note that although the 10.13 SDK includes this structure extension, numbered version 16, 10.13db1 17A264c continues to use version 15 as used on 10.12, at least in crashpad_snapshot_test. Bug: crashpad:185 Test: crashpad_snapshot_test ProcessTypes.DyldImagesSelf Change-Id: I59a80c85bb234ef698c65a0ac5bbeac5b40fda77 Reviewed-on: https://chromium-review.googlesource.com/535394 Reviewed-by: Robert Sesek --- snapshot/mac/process_types.cc | 9 +++++ snapshot/mac/process_types.h | 6 +++- snapshot/mac/process_types/custom.cc | 4 ++- .../mac/process_types/dyld_images.proctype | 24 ++++++++----- snapshot/mac/process_types/traits.h | 1 + snapshot/mac/process_types_test.cc | 34 ++++++++++++++----- 6 files changed, 60 insertions(+), 18 deletions(-) diff --git a/snapshot/mac/process_types.cc b/snapshot/mac/process_types.cc index 53d89035..aeff88bb 100644 --- a/snapshot/mac/process_types.cc +++ b/snapshot/mac/process_types.cc @@ -41,6 +41,15 @@ inline void Assign(Type* destination, const Type& source) { memcpy(destination, &source, sizeof(source)); } +template <> +inline void Assign( + process_types::internal::Reserved32_32Only64* destination, + const process_types::internal::Reserved32_32Only32& source) { + // Reserved32_32Only32 carries no data and has no storage in the 64-bit + // structure. +} + template <> inline void Assign( diff --git a/snapshot/mac/process_types.h b/snapshot/mac/process_types.h index 7274515f..eb397b9b 100644 --- a/snapshot/mac/process_types.h +++ b/snapshot/mac/process_types.h @@ -37,8 +37,10 @@ using Nothing = char[0]; // Some structure definitions differ in 32-bit and 64-bit environments by having // additional “reserved” padding fields present only in the 64-bit environment. -// These Reserved*_64Only* types allow the process_types system to replicate +// These Reserved*_*Only* types allow the process_types system to replicate // these structures more precisely. +using Reserved32_32Only32 = uint32_t; +using Reserved32_32Only64 = Nothing; using Reserved32_64Only32 = Nothing; using Reserved32_64Only64 = uint32_t; using Reserved64_64Only32 = Nothing; @@ -71,6 +73,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using Pointer = internal::TraitsGeneric::Pointer; \ using IntPtr = internal::TraitsGeneric::IntPtr; \ using UIntPtr = internal::TraitsGeneric::UIntPtr; \ + using Reserved32_32Only = internal::TraitsGeneric::Reserved32_32Only; \ using Reserved32_64Only = internal::TraitsGeneric::Reserved32_64Only; \ using Reserved64_64Only = internal::TraitsGeneric::Reserved64_64Only; \ using Nothing = internal::TraitsGeneric::Nothing; \ @@ -162,6 +165,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using Pointer = typename Traits::Pointer; \ using IntPtr = typename Traits::IntPtr; \ using UIntPtr = typename Traits::UIntPtr; \ + using Reserved32_32Only = typename Traits::Reserved32_32Only; \ using Reserved32_64Only = typename Traits::Reserved32_64Only; \ using Reserved64_64Only = typename Traits::Reserved64_64Only; \ using Nothing = typename Traits::Nothing; \ diff --git a/snapshot/mac/process_types/custom.cc b/snapshot/mac/process_types/custom.cc index c896ebf6..7839b403 100644 --- a/snapshot/mac/process_types/custom.cc +++ b/snapshot/mac/process_types/custom.cc @@ -80,7 +80,9 @@ size_t dyld_all_image_infos::ExpectedSizeForVersion( offsetof(dyld_all_image_infos, sharedCacheSlide), // 11 offsetof(dyld_all_image_infos, sharedCacheUUID), // 12 offsetof(dyld_all_image_infos, infoArrayChangeTimestamp), // 13 - offsetof(dyld_all_image_infos, end), // 14 + offsetof(dyld_all_image_infos, end_14_15), // 14 + offsetof(dyld_all_image_infos, end_14_15), // 15 + sizeof(dyld_all_image_infos), // 16 }; if (version >= arraysize(kSizeForVersion)) { diff --git a/snapshot/mac/process_types/dyld_images.proctype b/snapshot/mac/process_types/dyld_images.proctype index 3470ee83..f92a8009 100644 --- a/snapshot/mac/process_types/dyld_images.proctype +++ b/snapshot/mac/process_types/dyld_images.proctype @@ -116,20 +116,28 @@ PROCESS_TYPE_STRUCT_BEGIN(dyld_all_image_infos) // Version 14 (OS X 10.9) // As of the 10.12 SDK, this is declared as reserved[9] for 64-bit platforms - // and reserved[4] for 32-bit platforms. + // and reserved[4] for 32-bit platforms. It was expanded to reserved[5] for + // 32-bit platforms in the 10.13 SDK to provide proper padding, but because + // the runtimes that use versions 14 and 15 were built with SDKs that did not + // have this extra padding, it’s necessary to treat the element at index 4 on + // 32-bit systems as outside of the version 14 and 15 structure. This is why + // |reserved| is only declared a 4-element array, with a special end_14_15 + // member (not present in the native definition) available to indicate the + // end of the native version 14 and 15 structure, preceding the padding in the + // 32-bit structure that would natively be addressed at index 4 of |reserved|. + // Treat reserved_4_32 as only available in version 16 of the structure. PROCESS_TYPE_STRUCT_MEMBER(UIntPtr, reserved, [4]) - PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_4) + PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_4_64) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_5) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_6) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_7) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_8) + PROCESS_TYPE_STRUCT_MEMBER(Nothing, end_14_15) + PROCESS_TYPE_STRUCT_MEMBER(Reserved32_32Only, reserved_4_32) - // The 32-bit version of the structure will have four extra bytes of tail - // padding when built for 64-bit systems than it does natively and when built - // for 32-bit systems. Instead of using sizeof(dyld_all_image_infos), use - // offsetof(dyld_all_image_infos, end) to avoid taking this tail padding into - // account. - PROCESS_TYPE_STRUCT_MEMBER(Nothing, end) + // Version 16 (macOS 10.13) + PROCESS_TYPE_STRUCT_MEMBER(UIntPtr, compact_dyld_image_info_addr) + PROCESS_TYPE_STRUCT_MEMBER(ULong, compact_dyld_image_info_size) // size_t PROCESS_TYPE_STRUCT_END(dyld_all_image_infos) #endif // ! PROCESS_TYPE_STRUCT_IMPLEMENT_INTERNAL_READ_INTO && diff --git a/snapshot/mac/process_types/traits.h b/snapshot/mac/process_types/traits.h index 396799be..613f4f57 100644 --- a/snapshot/mac/process_types/traits.h +++ b/snapshot/mac/process_types/traits.h @@ -36,6 +36,7 @@ using Pointer = uint##lp_bits##_t __VA_ARGS__; \ using IntPtr = int##lp_bits##_t __VA_ARGS__; \ using UIntPtr = uint##lp_bits##_t __VA_ARGS__; \ + using Reserved32_32Only = Reserved32_32Only##lp_bits; \ using Reserved32_64Only = Reserved32_64Only##lp_bits; \ using Reserved64_64Only = Reserved64_64Only##lp_bits; \ using Nothing = Nothing; \ diff --git a/snapshot/mac/process_types_test.cc b/snapshot/mac/process_types_test.cc index 4f5ce7af..8ab15c8d 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -48,6 +48,11 @@ TEST(ProcessTypes, DyldImagesSelf) { // Get the in-process view of dyld_all_image_infos, and check it for sanity. const dyld_all_image_infos* self_image_infos = DyldGetAllImageInfos(); int mac_os_x_minor_version = MacOSXMinorVersion(); + + // The 10.13 SDK defines dyld_all_image_infos version 16 and says that it’s + // used on 10.13, but 10.13db1 17A264c uses version 15. + // + // TODO(mark): Recheck later in the beta period, up to the 10.13 release. if (mac_os_x_minor_version >= 12) { EXPECT_GE(self_image_infos->version, 15u); } else if (mac_os_x_minor_version >= 9) { @@ -99,16 +104,18 @@ TEST(ProcessTypes, DyldImagesSelf) { ProcessReader process_reader; ASSERT_TRUE(process_reader.Initialize(mach_task_self())); -#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_12 - const uint32_t kDyldAllImageInfosVersionInSDK = 15; +#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_13 + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 16; +#elif MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_12 + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 15; #elif MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_9 - const uint32_t kDyldAllImageInfosVersionInSDK = 14; + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 14; #elif MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_7 - const uint32_t kDyldAllImageInfosVersionInSDK = 12; + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 12; #elif MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_6 - const uint32_t kDyldAllImageInfosVersionInSDK = 7; + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 7; #else - const uint32_t kDyldAllImageInfosVersionInSDK = 1; + constexpr uint32_t kDyldAllImageInfosVersionInSDK = 1; #endif // Make sure that the size of the structure as declared in the SDK matches the @@ -119,7 +126,7 @@ TEST(ProcessTypes, DyldImagesSelf) { // Make sure that the computed sizes of various versions of this structure are // correct at different bitnessses. - const struct { + constexpr struct { uint32_t version; size_t size_32; size_t size_64; @@ -138,6 +145,7 @@ TEST(ProcessTypes, DyldImagesSelf) { {13, 104, 184}, {14, 164, 304}, {15, 164, 304}, + {16, 176, 320}, }; for (size_t index = 0; index < arraysize(kVersionsAndSizes); ++index) { uint32_t version = kVersionsAndSizes[index].version; @@ -289,7 +297,8 @@ TEST(ProcessTypes, DyldImagesSelf) { << "index " << index; } #if defined(ARCH_CPU_64_BITS) - EXPECT_EQ(proctype_image_infos.reserved_4, self_image_infos->reserved[4]); + EXPECT_EQ(proctype_image_infos.reserved_4_64, + self_image_infos->reserved[4]); EXPECT_EQ(proctype_image_infos.reserved_5, self_image_infos->reserved[5]); EXPECT_EQ(proctype_image_infos.reserved_6, self_image_infos->reserved[6]); EXPECT_EQ(proctype_image_infos.reserved_7, self_image_infos->reserved[7]); @@ -298,6 +307,15 @@ TEST(ProcessTypes, DyldImagesSelf) { } #endif +#if MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_13 + if (proctype_image_infos.version >= 16) { + EXPECT_EQ(proctype_image_infos.compact_dyld_image_info_addr, + self_image_infos->compact_dyld_image_info_addr); + EXPECT_EQ(proctype_image_infos.compact_dyld_image_info_size, + self_image_infos->compact_dyld_image_info_size); + } +#endif + if (proctype_image_infos.version >= 1) { std::vector proctype_image_info_vector( proctype_image_infos.infoArrayCount); From 890ad441b317ffdd676f4ab3b7f2ace402912095 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 14 Jun 2017 15:01:53 -0400 Subject: [PATCH 28/32] =?UTF-8?q?mac:=20Accept=20modules=20in=2010.13?= =?UTF-8?q?=E2=80=99s=20dyld=20shared=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In 10.13, modules loaded from the dyld shared cache appear with __TEXT segments that have a nonzero “fileoff” (file offset). Previously, the fileoff was always 0. Previously, the fileoff for segments in the dyld shared cache was the actual offset into the shared cache (not 0), but special consideration was given to __TEXT segments which were forced to 0. See 10.12.4 dyld-433.5/interlinked-dylibs/OptimizerLinkedit.cpp LinkeditOptimizer<>::updateLoadCommands(). Note the comment there where the __TEXT segment’s apparent fileoff is set to 0: // HACK until lldb fixed in: // DynamicLoaderMacOSXDYLD fixes for Monarch dyld shared cache Refer also to the lldb commit that references the above, http://llvm.org/viewvc/llvm-project?view=revision&revision=233714. Evidently, update_dyld_shared_cache has been revised to no longer apply this hack in 10.13. Crashpad’s sanity check for __TEXT segments having a fileoff of 0 is no longer valid, and causes it to reject modules loaded from the dyld shared cache. Since this was just a sanity check, remove it entirely. This caused module information for modules loaded from the dyld shared cache to be missing from minidumps produced on 10.13, which in turn prevented symbolization in frames belonging to most system libraries. For reasons not yet understood, I don’t see this problem in Chrome on 10.13db1 17A264c on a test virtual machine (HFS+ filesystem), although I do see it on actual hardware (APFS filesystem), and I do see it in Crashpad’s tests and reduced testcases on both as well. Bug: crashpad:185, crashpad:189 Test: crashpad_snapshot_test MachOImageReader.Self_DyldImages:ProcessReader.SelfModules:ProcessReader.ChildModules:ProcessTypes.DyldImagesSelf Change-Id: I8b0a22c55c33ce920804a879f6fab67272f3556e Reviewed-on: https://chromium-review.googlesource.com/535576 Reviewed-by: Robert Sesek --- snapshot/mac/mach_o_image_reader.cc | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/snapshot/mac/mach_o_image_reader.cc b/snapshot/mac/mach_o_image_reader.cc index d0b7a087..f16aa554 100644 --- a/snapshot/mac/mach_o_image_reader.cc +++ b/snapshot/mac/mach_o_image_reader.cc @@ -555,22 +555,14 @@ bool MachOImageReader::ReadSegmentCommand( return false; } - mach_vm_size_t vmsize = segment->vmsize(); - if (segment_name == SEG_TEXT) { + mach_vm_size_t vmsize = segment->vmsize(); + if (vmsize == 0) { LOG(WARNING) << "zero-sized " SEG_TEXT " segment" << load_command_info; return false; } - mach_vm_size_t fileoff = segment->fileoff(); - if (fileoff != 0) { - LOG(WARNING) << base::StringPrintf( - SEG_TEXT " segment has unexpected fileoff 0x%llx", - fileoff) << load_command_info; - return false; - } - size_ = vmsize; // The slide is computed as the difference between the __TEXT segment’s From 63ccbd0e4c2fa9e1f69c0b1e5f4627ad66dcc4f1 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 14 Jun 2017 16:12:16 -0400 Subject: [PATCH 29/32] Remove compiler_specific.h #include from aligned_allocator.h This was missed in Crashpad 8c35d92ae403. It syncs with Chromium 16289b3ef759. Change-Id: I7e92e71fc940e25e751e7487d100b5684bdbf667 Reviewed-on: https://chromium-review.googlesource.com/535577 Commit-Queue: Mark Mentovai Reviewed-by: Scott Graham Reviewed-by: Mark Mentovai --- util/stdlib/aligned_allocator.h | 1 - util/stdlib/aligned_allocator_test.cc | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/util/stdlib/aligned_allocator.h b/util/stdlib/aligned_allocator.h index 91012551..e8b72c7a 100644 --- a/util/stdlib/aligned_allocator.h +++ b/util/stdlib/aligned_allocator.h @@ -23,7 +23,6 @@ #include #include -#include "base/compiler_specific.h" #include "build/build_config.h" #include "util/stdlib/cxx.h" diff --git a/util/stdlib/aligned_allocator_test.cc b/util/stdlib/aligned_allocator_test.cc index 2799558e..4a018f58 100644 --- a/util/stdlib/aligned_allocator_test.cc +++ b/util/stdlib/aligned_allocator_test.cc @@ -16,6 +16,7 @@ #include +#include "base/compiler_specific.h" #include "gtest/gtest.h" #if defined(OS_WIN) From d3e4f09742d60237965f8d069f8500b397fe4495 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 15 Jun 2017 15:51:41 -0700 Subject: [PATCH 30/32] linux: Collect fxsave instead of fsave in ThreadInfo Bug: crashpad:30 Change-Id: Ib4abf0ad60b792c8241b28e6b5e47970fdfcf451 Reviewed-on: https://chromium-review.googlesource.com/537532 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- util/linux/thread_info.cc | 56 +++++++++++++++++++--------------- util/linux/thread_info.h | 64 +++++++++++++++++++++++++-------------- 2 files changed, 73 insertions(+), 47 deletions(-) diff --git a/util/linux/thread_info.cc b/util/linux/thread_info.cc index d3897631..300ae7d3 100644 --- a/util/linux/thread_info.cc +++ b/util/linux/thread_info.cc @@ -30,6 +30,34 @@ namespace crashpad { namespace { +#if defined(ARCH_CPU_X86_FAMILY) + +template +bool GetRegisterSet(pid_t tid, int set, Destination* dest) { + iovec iov; + iov.iov_base = dest; + iov.iov_len = sizeof(*dest); + if (ptrace(PTRACE_GETREGSET, tid, reinterpret_cast(set), &iov) != 0) { + PLOG(ERROR) << "ptrace"; + return false; + } + if (iov.iov_len != sizeof(*dest)) { + LOG(ERROR) << "Unexpected registers size"; + return false; + } + return true; +} + +bool GetFloatingPointRegisters32(pid_t tid, FloatContext* context) { + return GetRegisterSet(tid, NT_PRXFPREG, &context->f32.fxsave); +} + +bool GetFloatingPointRegisters64(pid_t tid, FloatContext* context) { + return GetRegisterSet(tid, NT_PRFPREG, &context->f64.fxsave); +} + +#elif defined(ARCH_CPU_ARM_FAMILY) + #if defined(ARCH_CPU_ARMEL) // PTRACE_GETREGSET, introduced in Linux 2.6.34 (2225a122ae26), requires kernel // support enabled by HAVE_ARCH_TRACEHOOK. This has been set for x86 (including @@ -77,7 +105,6 @@ bool GetFloatingPointRegistersLegacy(pid_t tid, FloatContext* context) { } #endif // ARCH_CPU_ARMEL -#if defined(ARCH_CPU_ARM_FAMILY) // Normally, the Linux kernel will copy out register sets according to the size // of the struct that contains them. Tracing a 32-bit ARM process running in // compatibility mode on a 64-bit ARM cpu will only copy data for the number of @@ -163,7 +190,9 @@ bool GetFloatingPointRegisters64(pid_t tid, FloatContext* context) { } return true; } -#endif // ARCH_CPU_ARM_FAMILY +#else +#error Port. +#endif // ARCH_CPU_X86_FAMILY } // namespace @@ -241,31 +270,8 @@ size_t ThreadInfo::GetGeneralPurposeRegistersAndLength(ThreadContext* context) { bool ThreadInfo::GetFloatingPointRegisters(FloatContext* context) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); -#if defined(ARCH_CPU_X86_FAMILY) - iovec iov; - iov.iov_base = context; - iov.iov_len = sizeof(*context); - if (ptrace( - PTRACE_GETREGSET, tid_, reinterpret_cast(NT_PRFPREG), &iov) != - 0) { - PLOG(ERROR) << "ptrace"; - return false; - } - if (is_64_bit_ && iov.iov_len == sizeof(context->f64)) { - return true; - } - if (!is_64_bit_ && iov.iov_len == sizeof(context->f32)) { - return true; - } - LOG(ERROR) << "Unexpected registers size"; - return false; - -#elif defined(ARCH_CPU_ARM_FAMILY) return is_64_bit_ ? GetFloatingPointRegisters64(tid_, context) : GetFloatingPointRegisters32(tid_, context); -#else -#error Port. -#endif // ARCH_CPU_X86_FAMILY } bool ThreadInfo::GetThreadArea(LinuxVMAddress* address) { diff --git a/util/linux/thread_info.h b/util/linux/thread_info.h index 6fb2621d..e9eeff31 100644 --- a/util/linux/thread_info.h +++ b/util/linux/thread_info.h @@ -27,6 +27,10 @@ #include "util/misc/initialization_state_dcheck.h" #include "util/numeric/int128.h" +#if defined(OS_ANDROID) +#include +#endif + namespace crashpad { //! \brief The set of general purpose registers for an architecture family. @@ -140,15 +144,22 @@ union FloatContext { //! architecture. struct f32 { #if defined(ARCH_CPU_X86_FAMILY) - // Reflects user_fpregs_struct in sys/user.h. - uint32_t cwd; - uint32_t swd; - uint32_t twd; - uint32_t fip; - uint32_t fcs; - uint32_t foo; - uint32_t fos; - uint32_t st_space[20]; + // Reflects user_fpxregs_struct in sys/user.h + struct fxsave { + uint16_t cwd; + uint16_t swd; + uint16_t twd; + uint16_t fop; + uint32_t fip; + uint32_t fcs; + uint32_t foo; + uint32_t fos; + uint32_t mxcsr; + uint32_t reserved; + uint32_t st_space[32]; + uint32_t xmm_space[32]; + uint32_t padding[56]; + } fxsave; #elif defined(ARCH_CPU_ARM_FAMILY) // Reflects user_fpregs in sys/user.h. struct fpregs { @@ -184,17 +195,20 @@ union FloatContext { //! architecture. struct f64 { #if defined(ARCH_CPU_X86_FAMILY) - uint16_t cwd; - uint16_t swd; - uint16_t ftw; - uint16_t fop; - uint64_t rip; - uint64_t rdp; - uint32_t mxcsr; - uint32_t mxcr_mask; - uint32_t st_space[32]; - uint32_t xmm_space[64]; - uint32_t padding[24]; + // Refelects user_fpregs_struct in sys/user.h + struct fxsave { + uint16_t cwd; + uint16_t swd; + uint16_t ftw; + uint16_t fop; + uint64_t rip; + uint64_t rdp; + uint32_t mxcsr; + uint32_t mxcr_mask; + uint32_t st_space[32]; + uint32_t xmm_space[64]; + uint32_t padding[24]; + } fxsave; #elif defined(ARCH_CPU_ARM_FAMILY) uint128_struct vregs[32]; uint32_t fpsr; @@ -206,9 +220,15 @@ union FloatContext { } f64; #if defined(ARCH_CPU_X86) - static_assert(sizeof(f32) == sizeof(user_fpregs_struct), "Size mismatch"); +#if defined(OS_ANDROID) && __ANDROID_API__ <= 19 + using NativeFpxregs = user_fxsr_struct; +#else + using NativeFpxregs = user_fpxregs_struct; +#endif // OS_ANDROID + static_assert(sizeof(f32::fxsave) == sizeof(NativeFpxregs), "Size mismatch"); #elif defined(ARCH_CPU_X86_64) - static_assert(sizeof(f64) == sizeof(user_fpregs_struct), "Size mismatch"); + static_assert(sizeof(f64::fxsave) == sizeof(user_fpregs_struct), + "Size mismatch"); #elif defined(ARCH_CPU_ARMEL) static_assert(sizeof(f32::fpregs) == sizeof(user_fpregs), "Size mismatch"); static_assert(sizeof(f32::vfp) == sizeof(user_vfp), "Size mismatch"); From 8c802aace4076bba2223666fff27373e24519eaf Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Mon, 19 Jun 2017 15:56:07 -0700 Subject: [PATCH 31/32] Add ReinterpretBytes which does a checked, variable size bit cast This renames and improves the VariableSizeBitCast helper from util/linux/auxiliary_vector.* and moves it to misc. Change-Id: I4bf46f4cfc0e60c900ff9bde467a21ad43c684cd Reviewed-on: https://chromium-review.googlesource.com/534174 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- util/linux/auxiliary_vector.cc | 42 ----------- util/linux/auxiliary_vector.h | 6 +- util/misc/reinterpret_bytes.cc | 70 +++++++++++++++++++ util/misc/reinterpret_bytes.h | 48 +++++++++++++ util/misc/reinterpret_bytes_test.cc | 104 ++++++++++++++++++++++++++++ util/util.gyp | 2 + util/util_test.gyp | 1 + 7 files changed, 227 insertions(+), 46 deletions(-) create mode 100644 util/misc/reinterpret_bytes.cc create mode 100644 util/misc/reinterpret_bytes.h create mode 100644 util/misc/reinterpret_bytes_test.cc diff --git a/util/linux/auxiliary_vector.cc b/util/linux/auxiliary_vector.cc index 8852e614..143ab6e3 100644 --- a/util/linux/auxiliary_vector.cc +++ b/util/linux/auxiliary_vector.cc @@ -16,9 +16,6 @@ #include #include -#include - -#include #include "base/files/file_path.h" #include "base/logging.h" @@ -62,43 +59,4 @@ bool AuxiliaryVector::Read(pid_t pid) { return false; } -// static -bool AuxiliaryVector::VariableSizeBitCast(uint64_t data, - char* dest, - size_t dest_size) { - auto data_p = reinterpret_cast(&data); - constexpr size_t data_size = sizeof(uint64_t); - - // Verify that any unused bytes from data are zero. - // The unused bytes are at the start of the data buffer for big-endian and the - // end of the buffer for little-endian. - if (dest_size < data_size) { - uint64_t zero = 0; - auto extra_bytes = data_p; -#if defined(ARCH_CPU_LITTLE_ENDIAN) - extra_bytes += dest_size; -#endif // ARCH_CPU_LITTLE_ENDIAN - if (memcmp(extra_bytes, &zero, data_size - dest_size) != 0) { - LOG(ERROR) << "information loss"; - return false; - } - } - - // Zero out the destination, in case it is larger than data. - memset(dest, 0, dest_size); - -#if defined(ARCH_CPU_LITTLE_ENDIAN) - // Copy a prefix of data to a prefix of dest for little-endian - memcpy(dest, data_p, std::min(dest_size, data_size)); -#else - // or the suffix of data to the suffix of dest for big-endian - if (data_size >= dest_size) { - memcpy(dest, data_p + data_size - dest_size, dest_size); - } else { - memcpy(dest + dest_size - data_size, data_p, data_size); - } -#endif // ARCH_CPU_LITTLE_ENDIAN - return true; -} - } // namespace crashpad diff --git a/util/linux/auxiliary_vector.h b/util/linux/auxiliary_vector.h index 904eada4..65d0e3cf 100644 --- a/util/linux/auxiliary_vector.h +++ b/util/linux/auxiliary_vector.h @@ -21,6 +21,7 @@ #include "base/logging.h" #include "base/macros.h" +#include "util/misc/reinterpret_bytes.h" namespace crashpad { @@ -55,8 +56,7 @@ class AuxiliaryVector { LOG(ERROR) << "value not found"; return false; } - return VariableSizeBitCast( - iter->second, reinterpret_cast(value), sizeof(V)); + return ReinterpretBytes(iter->second, value); } protected: @@ -66,8 +66,6 @@ class AuxiliaryVector { template bool Read(pid_t pid); - static bool VariableSizeBitCast(uint64_t data, char* dest, size_t dest_size); - DISALLOW_COPY_AND_ASSIGN(AuxiliaryVector); }; diff --git a/util/misc/reinterpret_bytes.cc b/util/misc/reinterpret_bytes.cc new file mode 100644 index 00000000..65ec33f3 --- /dev/null +++ b/util/misc/reinterpret_bytes.cc @@ -0,0 +1,70 @@ +// 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/misc/reinterpret_bytes.h" + +#include + +#include + +#include "base/logging.h" + +namespace crashpad { +namespace internal { + +bool ReinterpretBytesImpl(const char* data, + size_t data_size, + char* dest, + size_t dest_size) { + // Verify that any unused bytes from data are zero. + // The unused bytes are at the start of the data buffer for big-endian and the + // end of the buffer for little-endian. + if (dest_size < data_size) { + auto extra_bytes = data; +#if defined(ARCH_CPU_LITTLE_ENDIAN) + extra_bytes += dest_size; +#endif // ARCH_CPU_LITTLE_ENDIAN + + uint64_t zero = 0; + size_t extra_bytes_size = data_size - dest_size; + while (extra_bytes_size > 0) { + size_t to_check = std::min(extra_bytes_size, sizeof(zero)); + if (memcmp(extra_bytes, &zero, to_check) != 0) { + LOG(ERROR) << "information loss"; + return false; + } + extra_bytes += to_check; + extra_bytes_size -= to_check; + } + } + + // Zero out the destination, in case it is larger than data. + memset(dest, 0, dest_size); + +#if defined(ARCH_CPU_LITTLE_ENDIAN) + // Copy a prefix of data to a prefix of dest for little-endian + memcpy(dest, data, std::min(dest_size, data_size)); +#else + // or the suffix of data to the suffix of dest for big-endian + if (data_size >= dest_size) { + memcpy(dest, data + data_size - dest_size, dest_size); + } else { + memcpy(dest + dest_size - data_size, data, data_size); + } +#endif // ARCH_CPU_LITTLE_ENDIAN + return true; +} + +} // namespace internal +} // namespace crashpad diff --git a/util/misc/reinterpret_bytes.h b/util/misc/reinterpret_bytes.h new file mode 100644 index 00000000..561180da --- /dev/null +++ b/util/misc/reinterpret_bytes.h @@ -0,0 +1,48 @@ +// 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_MISC_REINTERPRET_BYTES_H_ +#define CRASHPAD_UTIL_MISC_REINTERPRET_BYTES_H_ + +#include + +namespace crashpad { + +namespace internal { + +bool ReinterpretBytesImpl(const char* from, + size_t from_size, + char* to, + size_t to_size); + +} // namespace internal + +//! \brief Copies the bytes of \a from to \to. +//! +//! This function is similar to `bit_cast`, except that it can operate on +//! differently sized types. +//! +//! \return `true` if the copy is possible without information loss, otherwise +//! `false` with a message logged. +template +bool ReinterpretBytes(const From& from, To* to) { + return internal::ReinterpretBytesImpl(reinterpret_cast(&from), + sizeof(From), + reinterpret_cast(to), + sizeof(To)); +} + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_MISC_REINTERPRET_BYTES_H_ diff --git a/util/misc/reinterpret_bytes_test.cc b/util/misc/reinterpret_bytes_test.cc new file mode 100644 index 00000000..47badb97 --- /dev/null +++ b/util/misc/reinterpret_bytes_test.cc @@ -0,0 +1,104 @@ +// 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/misc/reinterpret_bytes.h" + +#include + +#include + +#include "base/bit_cast.h" +#include "gtest/gtest.h" +#include "util/numeric/int128.h" + +namespace crashpad { +namespace test { +namespace { + +template +void ExpectReinterpret(From from, To* to, To expected) { + ASSERT_TRUE(ReinterpretBytes(from, to)); + EXPECT_EQ(*to, expected); +} + +template +void ExpectUnsignedEqual(From from, To* to) { + To expected = static_cast(from); + ExpectReinterpret(from, to, expected); +} + +TEST(ReinterpretBytes, ToUnsigned) { + uint64_t from64, to64; + uint32_t from32, to32; + + from32 = 0; + ExpectUnsignedEqual(from32, &to32); + ExpectUnsignedEqual(from32, &to64); + + from32 = std::numeric_limits::max(); + ExpectUnsignedEqual(from32, &to32); + ExpectUnsignedEqual(from32, &to64); + + from64 = 0; + ExpectUnsignedEqual(from64, &to32); + ExpectUnsignedEqual(from64, &to64); + + from64 = std::numeric_limits::max(); + ExpectUnsignedEqual(from64, &to64); + EXPECT_FALSE(ReinterpretBytes(from64, &to32)); + + uint8_t to8 = std::numeric_limits::max(); + uint128_struct from128; + from128.lo = to8; + from128.hi = 0; + ExpectReinterpret(from128, &to8, to8); +} + +TEST(ReinterpretBytes, ToSigned) { + uint64_t from64; + int64_t to64; + int32_t to32; + + from64 = 0; + ExpectReinterpret(from64, &to32, static_cast(0)); + ExpectReinterpret(from64, &to64, static_cast(0)); + + to32 = -1; + from64 = bit_cast(to32); + ExpectReinterpret(from64, &to32, to32); + + to32 = std::numeric_limits::max(); + from64 = bit_cast(to32); + ExpectReinterpret(from64, &to32, to32); + + to32 = std::numeric_limits::min(); + from64 = bit_cast(to32); + ExpectReinterpret(from64, &to32, to32); + + to64 = -1; + from64 = bit_cast(to64); + ExpectReinterpret(from64, &to64, to64); + + to64 = std::numeric_limits::max(); + from64 = bit_cast(to64); + ExpectReinterpret(from64, &to64, to64); + + to64 = std::numeric_limits::min(); + from64 = bit_cast(to64); + ExpectReinterpret(from64, &to64, to64); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index 8e5c7cfe..60e41957 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -120,6 +120,8 @@ 'misc/pdb_structures.h', 'misc/random_string.cc', 'misc/random_string.h', + 'misc/reinterpret_bytes.cc', + 'misc/reinterpret_bytes.h', 'misc/scoped_forbid_return.cc', 'misc/scoped_forbid_return.h', 'misc/symbolic_constants_common.h', diff --git a/util/util_test.gyp b/util/util_test.gyp index ef5a681f..1b4cff95 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -71,6 +71,7 @@ 'misc/paths_test.cc', 'misc/scoped_forbid_return_test.cc', 'misc/random_string_test.cc', + 'misc/reinterpret_bytes_test.cc', 'misc/uuid_test.cc', 'net/http_body_gzip_test.cc', 'net/http_body_test.cc', From c4f6ca3c6ac50fe4724c126518337972e778f133 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 20 Jun 2017 09:50:37 -0400 Subject: [PATCH 32/32] mac: Provide a larger thread state buffer for AVX-512 on 10.13 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Crashpad doesn’t use AVX-512, but when receiving replies to exceptions forwarded to ReportCrash, may see buffers large enough to contain AVX-512 thread state. This can result in messages like “UniversalExceptionRaise: (ipc/rcv) msg too large (0x10004004)”. I386_THREAD_STATE_MAX has increased from 224 to 614 in the 10.13 SDK, meaning that the maximum supported size for old_state and new_state in [mach_]exception_raise_state[_identity]() has increased from 896 to 2,456 bytes. This constant defines the size of the buffer that these MIG-generated routines will work with. By providing this definition in compat, the buffer size is increased when building with older SDKs. Note that on the “send” side, the size of the message given to mach_msg() will be trimmed to include only the valid part of the state area based on the stateCnt field, so increasing the value to 614 here won’t result Crashpad sending messages this large. That would be a potential interoperability concern with older OS versions. Bug: crashpad:185, crashpad:190 Change-Id: Ia46091ae46fd6227a17f59eb4bc00914be471aa7 Reviewed-on: https://chromium-review.googlesource.com/541515 Reviewed-by: Robert Sesek --- compat/compat.gyp | 1 + compat/mac/mach/i386/thread_state.h | 28 ++++++++++++++++++++++++++++ compat/mac/mach/mach.h | 14 ++++++++++++++ util/mach/mig.py | 10 ++++++++-- util/util.gyp | 6 +++++- 5 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 compat/mac/mach/i386/thread_state.h diff --git a/compat/compat.gyp b/compat/compat.gyp index 56daa3f8..e2b2df6d 100644 --- a/compat/compat.gyp +++ b/compat/compat.gyp @@ -23,6 +23,7 @@ 'sources': [ 'mac/AvailabilityMacros.h', 'mac/kern/exc_resource.h', + 'mac/mach/i386/thread_state.h', 'mac/mach/mach.h', 'mac/mach-o/getsect.cc', 'mac/mach-o/getsect.h', diff --git a/compat/mac/mach/i386/thread_state.h b/compat/mac/mach/i386/thread_state.h new file mode 100644 index 00000000..de744d64 --- /dev/null +++ b/compat/mac/mach/i386/thread_state.h @@ -0,0 +1,28 @@ +// 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_MAC_MACH_I386_THREAD_STATE_H_ +#define CRASHPAD_COMPAT_MAC_MACH_I386_THREAD_STATE_H_ + +#include_next + +// 10.13 SDK +// +// This was defined as 244 in the 10.7 through 10.12 SDKs, and 144 previously. +#if I386_THREAD_STATE_MAX < 614 +#undef I386_THREAD_STATE_MAX +#define I386_THREAD_STATE_MAX (614) +#endif + +#endif // CRASHPAD_COMPAT_MAC_MACH_I386_THREAD_STATE_H_ diff --git a/compat/mac/mach/mach.h b/compat/mac/mach/mach.h index 9cfe6b62..55f5fdd2 100644 --- a/compat/mac/mach/mach.h +++ b/compat/mac/mach/mach.h @@ -93,6 +93,20 @@ #define x86_AVX_STATE 18 #endif +// 10.13 SDK + +#ifndef x86_AVX512_STATE32 +#define x86_AVX512_STATE32 19 +#endif + +#ifndef x86_AVX512_STATE64 +#define x86_AVX512_STATE64 20 +#endif + +#ifndef x86_AVX512_STATE +#define x86_AVX512_STATE 21 +#endif + #endif // defined(__i386__) || defined(__x86_64__) // diff --git a/util/mach/mig.py b/util/mach/mig.py index a41c4f89..992f3e1a 100755 --- a/util/mach/mig.py +++ b/util/mach/mig.py @@ -111,6 +111,10 @@ def main(args): parser = argparse.ArgumentParser() parser.add_argument('--developer-dir', help='Path to Xcode') parser.add_argument('--sdk', help='Path to SDK') + parser.add_argument('--include', + default=[], + action='append', + help='Additional include directory') parser.add_argument('defs') parser.add_argument('user_c') parser.add_argument('server_c') @@ -124,10 +128,12 @@ def main(args): '-header', parsed.user_h, '-sheader', parsed.server_h, ] - if parsed.sdk is not None: - command.extend(['-isysroot', parsed.sdk]) if parsed.developer_dir is not None: os.environ['DEVELOPER_DIR'] = parsed.developer_dir + if parsed.sdk is not None: + command.extend(['-isysroot', parsed.sdk]) + for include in parsed.include: + command.extend(['-I' + include]) command.append(parsed.defs) subprocess.check_call(command) FixUserImplementation(parsed.user_c) diff --git a/util/util.gyp b/util/util.gyp index 60e41957..6a052273 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -285,7 +285,11 @@ '<(INTERMEDIATE_DIR)/util/mach/<(RULE_INPUT_ROOT)Server.h', ], 'action': [ - 'python', '<@(_inputs)', '<(RULE_INPUT_PATH)', '<@(_outputs)' + 'python', + '<@(_inputs)', + '<(RULE_INPUT_PATH)', + '<@(_outputs)', + '--include=../compat/mac' ], 'process_outputs_as_sources': 1, },