From 94b7e45210a74b1eeb2cff981540312c9aaf58ed Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Mon, 3 Aug 2020 13:39:14 -0700 Subject: [PATCH 1/9] fix OS guards for attachments Change-Id: I10fdaf15ea4ba5906eeb81234508e987c29985ff Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2333082 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- handler/handler_main.cc | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/handler/handler_main.cc b/handler/handler_main.cc index 2c9bfbc979..c8001f4571 100644 --- a/handler/handler_main.cc +++ b/handler/handler_main.cc @@ -95,16 +95,21 @@ namespace crashpad { namespace { +#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) || \ + defined(OS_ANDROID) +#define ATTACHMENTS_SUPPORTED 1 +#endif // OS_WIN || OS_LINUX || OS_CHROMEOS || OS_ANDROID + void Usage(const base::FilePath& me) { fprintf(stderr, "Usage: %" PRFilePath " [OPTION]...\n" "Crashpad's exception handler server.\n" "\n" " --annotation=KEY=VALUE set a process annotation in each crash report\n" -#if defined(OS_WIN) || defined(OS_LINUX) +#if defined(ATTACHMENTS_SUPPORTED) " --attachment=FILE_PATH attach specified file to each crash report\n" " at the time of the crash\n" -#endif // OS_WIN || OS_LINUX +#endif // ATTACHMENTS_SUPPORTED " --database=PATH store the crash report database at PATH\n" #if defined(OS_APPLE) " --handshake-fd=FD establish communication with the client over FD\n" @@ -215,10 +220,9 @@ struct Options { base::FilePath minidump_dir_for_tests; bool always_allow_feedback = false; #endif // OS_CHROMEOS -#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) || \ - defined(OS_ANDROID) +#if defined(ATTACHMENTS_SUPPORTED) std::vector attachments; -#endif // OS_WIN || OS_LINUX +#endif // ATTACHMENTS_SUPPORTED }; // Splits |key_value| on '=' and inserts the resulting key and value into |map|. @@ -580,9 +584,9 @@ int HandlerMain(int argc, static constexpr option long_options[] = { {"annotation", required_argument, nullptr, kOptionAnnotation}, -#if defined(OS_WIN) || defined(OS_LINUX) +#if defined(ATTACHMENTS_SUPPORTED) {"attachment", required_argument, nullptr, kOptionAttachment}, -#endif // OS_WIN || OS_LINUX +#endif // ATTACHMENTS_SUPPORTED {"database", required_argument, nullptr, kOptionDatabase}, #if defined(OS_APPLE) {"handshake-fd", required_argument, nullptr, kOptionHandshakeFD}, @@ -692,14 +696,13 @@ int HandlerMain(int argc, } break; } -#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS) || \ - defined(OS_ANDROID) +#if defined(ATTACHMENTS_SUPPORTED) case kOptionAttachment: { options.attachments.push_back(base::FilePath( ToolSupport::CommandLineArgumentToFilePathStringType(optarg))); break; } -#endif // OS_WIN || OS_LINUX +#endif // ATTACHMENTS_SUPPORTED case kOptionDatabase: { options.database = base::FilePath( ToolSupport::CommandLineArgumentToFilePathStringType(optarg)); @@ -1012,9 +1015,9 @@ int HandlerMain(int argc, database.get(), static_cast(upload_thread.Get()), &options.annotations, -#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_ANDROID) +#if defined(ATTACHMENTS_SUPPORTED) &options.attachments, -#endif // OS_WIN || OS_LINUX +#endif // ATTACHMENTS_SUPPORTED #if defined(OS_ANDROID) options.write_minidump_to_database, options.write_minidump_to_log, From 2f66eefb79212213d6de5d089328ce884135d27d Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 6 Aug 2020 13:48:45 -0700 Subject: [PATCH 2/9] Update language to eliminate 'whitelist' Change-Id: I6afe27313093c6867d0276274e6b17b195d9d263 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2339536 Commit-Queue: Joshua Peraza Reviewed-by: Mark Mentovai --- build/run_tests.py | 2 +- handler/linux/capture_snapshot.cc | 23 +++---- .../sanitized/memory_snapshot_sanitized.h | 4 +- .../sanitized/module_snapshot_sanitized.cc | 24 +++---- .../sanitized/module_snapshot_sanitized.h | 9 ++- .../sanitized/process_snapshot_sanitized.cc | 14 ++-- .../sanitized/process_snapshot_sanitized.h | 12 ++-- .../process_snapshot_sanitized_test.cc | 58 ++++++++-------- .../sanitized/sanitization_information.cc | 68 +++++++++---------- snapshot/sanitized/sanitization_information.h | 48 ++++++------- .../sanitization_information_test.cc | 37 +++++----- util/process/process_memory_sanitized.cc | 12 ++-- util/process/process_memory_sanitized.h | 10 +-- util/process/process_memory_sanitized_test.cc | 20 +++--- 14 files changed, 169 insertions(+), 172 deletions(-) diff --git a/build/run_tests.py b/build/run_tests.py index 02639783fc..5e70d7b668 100755 --- a/build/run_tests.py +++ b/build/run_tests.py @@ -292,7 +292,7 @@ def _adb_shell(command_args, env={}): # pseudo-terminal device, Google Test will not normally enable colored # output, so mimic Google Test’s own logic for deciding whether to # enable color by checking this script’s own standard output connection. - # The whitelist of TERM values comes from Google Test’s + # The list of TERM values comes from Google Test’s # googletest/src/gtest.cc testing::internal::ShouldUseColor(). env = {'CRASHPAD_TEST_DATA_ROOT': device_temp_dir} gtest_color = os.environ.get('GTEST_COLOR') diff --git a/handler/linux/capture_snapshot.cc b/handler/linux/capture_snapshot.cc index 005402956e..12beddfccf 100644 --- a/handler/linux/capture_snapshot.cc +++ b/handler/linux/capture_snapshot.cc @@ -80,17 +80,16 @@ bool CaptureSnapshot( return false; } - auto annotations_whitelist = std::make_unique>(); - auto memory_range_whitelist = + auto allowed_annotations = std::make_unique>(); + auto allowed_memory_ranges = std::make_unique>>(); - if (!ReadAnnotationsWhitelist( + if (!ReadAllowedAnnotations(range, + sanitization_info.allowed_annotations_address, + allowed_annotations.get()) || + !ReadAllowedMemoryRanges( range, - sanitization_info.annotations_whitelist_address, - annotations_whitelist.get()) || - !ReadMemoryRangeWhitelist( - range, - sanitization_info.memory_range_whitelist_address, - memory_range_whitelist.get())) { + sanitization_info.allowed_memory_ranges_address, + allowed_memory_ranges.get())) { Metrics::ExceptionCaptureResult( Metrics::CaptureResult::kSanitizationInitializationFailed); return false; @@ -99,10 +98,10 @@ bool CaptureSnapshot( std::unique_ptr sanitized( new ProcessSnapshotSanitized()); if (!sanitized->Initialize(process_snapshot.get(), - sanitization_info.annotations_whitelist_address - ? std::move(annotations_whitelist) + sanitization_info.allowed_annotations_address + ? std::move(allowed_annotations) : nullptr, - std::move(memory_range_whitelist), + std::move(allowed_memory_ranges), sanitization_info.target_module_address, sanitization_info.sanitize_stacks)) { Metrics::ExceptionCaptureResult( diff --git a/snapshot/sanitized/memory_snapshot_sanitized.h b/snapshot/sanitized/memory_snapshot_sanitized.h index 41e324f50a..be3e5d4ac5 100644 --- a/snapshot/sanitized/memory_snapshot_sanitized.h +++ b/snapshot/sanitized/memory_snapshot_sanitized.h @@ -27,7 +27,7 @@ namespace internal { //! another MemorySnapshot. //! //! This class redacts all data from the wrapped MemorySnapshot unless: -//! 1. The data is pointer aligned and points into a whitelisted address range. +//! 1. The data is pointer aligned and points into an allowed address range. //! 2. The data is pointer aligned and is a small integer. class MemorySnapshotSanitized final : public MemorySnapshot { public: @@ -41,7 +41,7 @@ class MemorySnapshotSanitized final : public MemorySnapshot { //! \brief Constructs this object. //! //! \param[in] snapshot The MemorySnapshot to sanitize. - //! \param[in] ranges A set of whitelisted address ranges. + //! \param[in] ranges A set of allowed address ranges. //! \param[in] is_64_bit `true` if this memory is for a 64-bit process. MemorySnapshotSanitized(const MemorySnapshot* snapshot, RangeSet* ranges, diff --git a/snapshot/sanitized/module_snapshot_sanitized.cc b/snapshot/sanitized/module_snapshot_sanitized.cc index 32b135781e..1789fa2785 100644 --- a/snapshot/sanitized/module_snapshot_sanitized.cc +++ b/snapshot/sanitized/module_snapshot_sanitized.cc @@ -19,9 +19,9 @@ namespace internal { namespace { -bool KeyIsInWhitelist(const std::string& name, - const std::vector& whitelist) { - for (const auto& key : whitelist) { +bool KeyIsAllowed(const std::string& name, + const std::vector& allowed_keys) { + for (const auto& key : allowed_keys) { if (name == key) { return true; } @@ -33,8 +33,8 @@ bool KeyIsInWhitelist(const std::string& name, ModuleSnapshotSanitized::ModuleSnapshotSanitized( const ModuleSnapshot* snapshot, - const std::vector* annotations_whitelist) - : snapshot_(snapshot), annotations_whitelist_(annotations_whitelist) {} + const std::vector* allowed_annotations) + : snapshot_(snapshot), allowed_annotations_(allowed_annotations) {} ModuleSnapshotSanitized::~ModuleSnapshotSanitized() = default; @@ -96,9 +96,9 @@ std::map ModuleSnapshotSanitized::AnnotationsSimpleMap() const { std::map annotations = snapshot_->AnnotationsSimpleMap(); - if (annotations_whitelist_) { + if (allowed_annotations_) { for (auto kv = annotations.begin(); kv != annotations.end(); ++kv) { - if (!KeyIsInWhitelist(kv->first, *annotations_whitelist_)) { + if (!KeyIsAllowed(kv->first, *allowed_annotations_)) { annotations.erase(kv); } } @@ -109,14 +109,14 @@ ModuleSnapshotSanitized::AnnotationsSimpleMap() const { std::vector ModuleSnapshotSanitized::AnnotationObjects() const { std::vector annotations = snapshot_->AnnotationObjects(); - if (annotations_whitelist_) { - std::vector whitelisted; + if (allowed_annotations_) { + std::vector allowed; for (const auto& anno : annotations) { - if (KeyIsInWhitelist(anno.name, *annotations_whitelist_)) { - whitelisted.push_back(anno); + if (KeyIsAllowed(anno.name, *allowed_annotations_)) { + allowed.push_back(anno); } } - annotations.swap(whitelisted); + annotations.swap(allowed); } return annotations; } diff --git a/snapshot/sanitized/module_snapshot_sanitized.h b/snapshot/sanitized/module_snapshot_sanitized.h index bbe1f5f48b..b64999e178 100644 --- a/snapshot/sanitized/module_snapshot_sanitized.h +++ b/snapshot/sanitized/module_snapshot_sanitized.h @@ -31,12 +31,11 @@ class ModuleSnapshotSanitized final : public ModuleSnapshot { //! \brief Constructs this object. //! //! \param[in] snapshot The ModuleSnapshot to sanitize. - //! \param[in] annotations_whitelist A list of annotation names to allow to be + //! \param[in] allowed_annotations A list of annotation names to allow to be //! returned by AnnotationsSimpleMap() or AnnotationObjects(). If //! `nullptr`, all annotations will be returned. - ModuleSnapshotSanitized( - const ModuleSnapshot* snapshot, - const std::vector* annotations_whitelist); + ModuleSnapshotSanitized(const ModuleSnapshot* snapshot, + const std::vector* allowed_annotations); ~ModuleSnapshotSanitized() override; // ModuleSnapshot: @@ -65,7 +64,7 @@ class ModuleSnapshotSanitized final : public ModuleSnapshot { private: const ModuleSnapshot* snapshot_; - const std::vector* annotations_whitelist_; + const std::vector* allowed_annotations_; DISALLOW_COPY_AND_ASSIGN(ModuleSnapshotSanitized); }; diff --git a/snapshot/sanitized/process_snapshot_sanitized.cc b/snapshot/sanitized/process_snapshot_sanitized.cc index 722abacead..21cf5a628c 100644 --- a/snapshot/sanitized/process_snapshot_sanitized.cc +++ b/snapshot/sanitized/process_snapshot_sanitized.cc @@ -84,14 +84,14 @@ ProcessSnapshotSanitized::~ProcessSnapshotSanitized() = default; bool ProcessSnapshotSanitized::Initialize( const ProcessSnapshot* snapshot, - std::unique_ptr> annotations_whitelist, + std::unique_ptr> allowed_annotations, std::unique_ptr>> - memory_range_whitelist, + allowed_memory_ranges, VMAddress target_module_address, bool sanitize_stacks) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); snapshot_ = snapshot; - annotations_whitelist_ = std::move(annotations_whitelist); + allowed_annotations_ = std::move(allowed_annotations); sanitize_stacks_ = sanitize_stacks; if (target_module_address) { @@ -139,10 +139,10 @@ bool ProcessSnapshotSanitized::Initialize( } } - if (annotations_whitelist_) { + if (allowed_annotations_) { for (const auto module : snapshot_->Modules()) { modules_.emplace_back(std::make_unique( - module, annotations_whitelist_.get())); + module, allowed_annotations_.get())); } } @@ -159,7 +159,7 @@ bool ProcessSnapshotSanitized::Initialize( } } - process_memory_.Initialize(snapshot_->Memory(), memory_range_whitelist.get()); + process_memory_.Initialize(snapshot_->Memory(), allowed_memory_ranges.get()); INITIALIZATION_STATE_SET_VALID(initialized_); return true; @@ -227,7 +227,7 @@ std::vector ProcessSnapshotSanitized::Threads() const { std::vector ProcessSnapshotSanitized::Modules() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - if (!annotations_whitelist_) { + if (!allowed_annotations_) { return snapshot_->Modules(); } diff --git a/snapshot/sanitized/process_snapshot_sanitized.h b/snapshot/sanitized/process_snapshot_sanitized.h index 2d38798111..eb059732cb 100644 --- a/snapshot/sanitized/process_snapshot_sanitized.h +++ b/snapshot/sanitized/process_snapshot_sanitized.h @@ -47,10 +47,10 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot { //! this object. //! //! \param[in] snapshot The ProcessSnapshot to sanitize. - //! \param[in] annotations_whitelist A list of annotations names to allow to + //! \param[in] allowed_annotations A list of annotations names to allow to //! be returned by AnnotationsSimpleMap() or from this object's module //! snapshots. If `nullptr`, all annotations will be returned. - //! \param[in] memory_range_whitelist A list of memory ranges to allow to be + //! \param[in] allowed_memory_ranges A list of memory ranges to allow to be //! accessible via Memory(), or `nullptr` to allow all ranges. //! \param[in] target_module_address An address in the target process' //! address space within the bounds of a module to target. If the @@ -65,9 +65,9 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot { //! should be filtered entirely. Otherwise `true`. bool Initialize( const ProcessSnapshot* snapshot, - std::unique_ptr> annotations_whitelist, + std::unique_ptr> allowed_annotations, std::unique_ptr>> - memory_range_whitelist, + allowed_memory_ranges, VMAddress target_module_address, bool sanitize_stacks); @@ -93,7 +93,7 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot { const ProcessMemory* Memory() const override; private: - // Only used when annotations_whitelist_ != nullptr. + // Only used when allowed_annotations_ != nullptr. std::vector> modules_; // Only used when sanitize_stacks_ == true. @@ -102,7 +102,7 @@ class ProcessSnapshotSanitized final : public ProcessSnapshot { RangeSet address_ranges_; const ProcessSnapshot* snapshot_; ProcessMemorySanitized process_memory_; - std::unique_ptr> annotations_whitelist_; + std::unique_ptr> allowed_annotations_; bool sanitize_stacks_; InitializationStateDcheck initialized_; diff --git a/snapshot/sanitized/process_snapshot_sanitized_test.cc b/snapshot/sanitized/process_snapshot_sanitized_test.cc index 85e540235b..0484778344 100644 --- a/snapshot/sanitized/process_snapshot_sanitized_test.cc +++ b/snapshot/sanitized/process_snapshot_sanitized_test.cc @@ -74,10 +74,10 @@ class ExceptionGenerator { DISALLOW_COPY_AND_ASSIGN(ExceptionGenerator); }; -constexpr char kWhitelistedAnnotationName[] = "name_of_whitelisted_anno"; -constexpr char kWhitelistedAnnotationValue[] = "some_value"; -constexpr char kNonWhitelistedAnnotationName[] = "non_whitelisted_anno"; -constexpr char kNonWhitelistedAnnotationValue[] = "private_annotation"; +constexpr char kAllowedAnnotationName[] = "name_of_allowed_anno"; +constexpr char kAllowedAnnotationValue[] = "some_value"; +constexpr char kNonAllowedAnnotationName[] = "non_allowed_anno"; +constexpr char kNonAllowedAnnotationValue[] = "private_annotation"; constexpr char kSensitiveStackData[] = "sensitive_stack_data"; struct ChildTestAddresses { @@ -92,13 +92,11 @@ void ChildTestFunction() { FileHandle in = StdioFileHandle(StdioStream::kStandardInput); FileHandle out = StdioFileHandle(StdioStream::kStandardOutput); - static StringAnnotation<32> whitelisted_annotation( - kWhitelistedAnnotationName); - whitelisted_annotation.Set(kWhitelistedAnnotationValue); + static StringAnnotation<32> allowed_annotation(kAllowedAnnotationName); + allowed_annotation.Set(kAllowedAnnotationValue); - static StringAnnotation<32> non_whitelisted_annotation( - kNonWhitelistedAnnotationName); - non_whitelisted_annotation.Set(kNonWhitelistedAnnotationValue); + static StringAnnotation<32> non_allowed_annotation(kNonAllowedAnnotationName); + non_allowed_annotation.Set(kNonAllowedAnnotationValue); char string_data[strlen(kSensitiveStackData) + 1]; strcpy(string_data, kSensitiveStackData); @@ -126,39 +124,39 @@ CRASHPAD_CHILD_TEST_MAIN(ChildToBeSanitized) { } void ExpectAnnotations(ProcessSnapshot* snapshot, bool sanitized) { - bool found_whitelisted = false; - bool found_non_whitelisted = false; + bool found_allowed = false; + bool found_non_allowed = false; for (auto module : snapshot->Modules()) { for (const auto& anno : module->AnnotationObjects()) { - if (anno.name == kWhitelistedAnnotationName) { - found_whitelisted = true; - } else if (anno.name == kNonWhitelistedAnnotationName) { - found_non_whitelisted = true; + if (anno.name == kAllowedAnnotationName) { + found_allowed = true; + } else if (anno.name == kNonAllowedAnnotationName) { + found_non_allowed = true; } } } - EXPECT_TRUE(found_whitelisted); + EXPECT_TRUE(found_allowed); if (sanitized) { - EXPECT_FALSE(found_non_whitelisted); + EXPECT_FALSE(found_non_allowed); } else { - EXPECT_TRUE(found_non_whitelisted); + EXPECT_TRUE(found_non_allowed); } } void ExpectProcessMemory(ProcessSnapshot* snapshot, - VMAddress whitelisted_byte, + VMAddress allowed_byte, bool sanitized) { auto memory = snapshot->Memory(); char out; - EXPECT_TRUE(memory->Read(whitelisted_byte, 1, &out)); + EXPECT_TRUE(memory->Read(allowed_byte, 1, &out)); - bool unwhitelisted_read = memory->Read(whitelisted_byte + 1, 1, &out); + bool disallowed_read = memory->Read(allowed_byte + 1, 1, &out); if (sanitized) { - EXPECT_FALSE(unwhitelisted_read); + EXPECT_FALSE(disallowed_read); } else { - EXPECT_TRUE(unwhitelisted_read); + EXPECT_TRUE(disallowed_read); } } @@ -272,18 +270,18 @@ class SanitizeTest : public MultiprocessExec { addrs.string_address, /* sanitized= */ false); - auto annotations_whitelist = std::make_unique>(); - annotations_whitelist->push_back(kWhitelistedAnnotationName); + auto allowed_annotations = std::make_unique>(); + allowed_annotations->push_back(kAllowedAnnotationName); - auto memory_ranges_whitelist = + auto allowed_memory_ranges = std::make_unique>>(); - memory_ranges_whitelist->push_back( + allowed_memory_ranges->push_back( std::make_pair(addrs.string_address, addrs.string_address + 1)); ProcessSnapshotSanitized sanitized; ASSERT_TRUE(sanitized.Initialize(&snapshot, - std::move(annotations_whitelist), - std::move(memory_ranges_whitelist), + std::move(allowed_annotations), + std::move(allowed_memory_ranges), addrs.module_address, true)); diff --git a/snapshot/sanitized/sanitization_information.cc b/snapshot/sanitized/sanitization_information.cc index 325365bfd8..1b2b853932 100644 --- a/snapshot/sanitized/sanitization_information.cc +++ b/snapshot/sanitized/sanitization_information.cc @@ -24,18 +24,18 @@ namespace crashpad { namespace { template -bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory, - VMAddress whitelist_address, - std::vector* whitelist) { - if (!whitelist_address) { +bool ReadAllowedAnnotations(const ProcessMemoryRange& memory, + VMAddress list_address, + std::vector* allowed_annotations) { + if (!list_address) { return true; } - std::vector local_whitelist; + std::vector local_allowed_annotations; Pointer name_address; - while (memory.Read(whitelist_address, sizeof(name_address), &name_address)) { + while (memory.Read(list_address, sizeof(name_address), &name_address)) { if (!name_address) { - whitelist->swap(local_whitelist); + allowed_annotations->swap(local_allowed_annotations); return true; } @@ -44,8 +44,8 @@ bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory, name_address, Annotation::kNameMaxLength, &name)) { return false; } - local_whitelist.push_back(name); - whitelist_address += sizeof(Pointer); + local_allowed_annotations.push_back(name); + list_address += sizeof(Pointer); } return false; @@ -53,27 +53,27 @@ bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory, } // namespace -bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory, - VMAddress whitelist_address, - std::vector* whitelist) { - return memory.Is64Bit() ? ReadAnnotationsWhitelist( - memory, whitelist_address, whitelist) - : ReadAnnotationsWhitelist( - memory, whitelist_address, whitelist); +bool ReadAllowedAnnotations(const ProcessMemoryRange& memory, + VMAddress list_address, + std::vector* allowed_annotations) { + return memory.Is64Bit() ? ReadAllowedAnnotations( + memory, list_address, allowed_annotations) + : ReadAllowedAnnotations( + memory, list_address, allowed_annotations); } -bool ReadMemoryRangeWhitelist( +bool ReadAllowedMemoryRanges( const ProcessMemoryRange& memory, - VMAddress whitelist_address, - std::vector>* whitelist) { - whitelist->clear(); - if (!whitelist_address) { + VMAddress list_address, + std::vector>* allowed_memory_ranges) { + allowed_memory_ranges->clear(); + if (!list_address) { return true; } - SanitizationMemoryRangeWhitelist list; - if (!memory.Read(whitelist_address, sizeof(list), &list)) { - LOG(ERROR) << "Failed to read memory range whitelist."; + SanitizationAllowedMemoryRanges list; + if (!memory.Read(list_address, sizeof(list), &list)) { + LOG(ERROR) << "Failed to read allowed memory ranges"; return false; } @@ -84,32 +84,32 @@ bool ReadMemoryRangeWhitelist( // An upper bound of entries that we never expect to see more than. constexpr size_t kMaxListSize = 256; if (list.size > kMaxListSize) { - LOG(ERROR) << "Whitelist exceeded maximum, size=" << list.size; + LOG(ERROR) << "list exceeded maximum, size=" << list.size; return false; } - std::vector ranges(list.size); + std::vector ranges(list.size); if (!memory.Read(list.entries, sizeof(ranges[0]) * list.size, ranges.data())) { - LOG(ERROR) << "Failed to read memory range whitelist entries."; + LOG(ERROR) << "Failed to read allowed memory ranges"; return false; } const VMAddress vm_max = memory.Is64Bit() ? std::numeric_limits::max() : std::numeric_limits::max(); - std::vector> local_whitelist; - for (size_t i = 0; i < list.size; i++) { + std::vector> local_allowed_memory_ranges; + for (size_t i = 0; i < list.size; ++i) { if (ranges[i].base > vm_max || ranges[i].length > vm_max - ranges[i].base) { - LOG(ERROR) << "Invalid memory range whitelist entry base=" - << ranges[i].base << " length=" << ranges[i].length; + LOG(ERROR) << "Invalid range: base=" << ranges[i].base + << " length=" << ranges[i].length; return false; } - local_whitelist.emplace_back(ranges[i].base, - ranges[i].base + ranges[i].length); + local_allowed_memory_ranges.emplace_back(ranges[i].base, + ranges[i].base + ranges[i].length); } - whitelist->swap(local_whitelist); + allowed_memory_ranges->swap(local_allowed_memory_ranges); return true; } diff --git a/snapshot/sanitized/sanitization_information.h b/snapshot/sanitized/sanitization_information.h index 0717ba7f6d..d5afe0d576 100644 --- a/snapshot/sanitized/sanitization_information.h +++ b/snapshot/sanitized/sanitization_information.h @@ -35,9 +35,9 @@ namespace crashpad { struct SanitizationInformation { //! \brief The address in the client process' address space of a nullptr //! terminated array of NUL-terminated strings. The string values are the - //! names of whitelisted annotations. This value is 0 if there is no - //! whitelist and all annotations are allowed. - VMAddress annotations_whitelist_address; + //! names of allowed annotations. This value is 0 if all annotations are + //! allowed. + VMAddress allowed_annotations_address; //! \brief An address in the client process' address space within a module to //! target. When a target module is used, crash dumps are discarded unless @@ -47,17 +47,17 @@ struct SanitizationInformation { VMAddress target_module_address; //! \brief The address in the client process' address space of a - //! a \a SanitizationMemoryRangeWhitelist, a list of whitelisted address - //! ranges allowed to be accessed by ProcessMemorySanitized. This value - //! is 0 if no memory is allowed to be read using ProcessMemorySanitized. - VMAddress memory_range_whitelist_address; + //! \a SanitizationAllowedMemoryRanges, a list of address ranges allowed + //! to be accessed by ProcessMemorySanitized. This value is 0 if no memory + //! is allowed to be read using ProcessMemorySanitized. + VMAddress allowed_memory_ranges_address; //! \brief Non-zero if stacks should be sanitized for possible PII. uint8_t sanitize_stacks; }; -//! \brief Describes a list of white listed memory ranges. -struct SanitizationMemoryRangeWhitelist { +//! \brief Describes a list of allowed memory ranges. +struct SanitizationAllowedMemoryRanges { //! \brief Describes a range of memory. struct Range { VMAddress base; @@ -71,30 +71,30 @@ struct SanitizationMemoryRangeWhitelist { #pragma pack(pop) -//! \brief Reads an annotations whitelist from another process. +//! \brief Reads a list of allowed annotations from another process. //! //! \param[in] memory A memory reader for the target process. -//! \param[in] whitelist_address The address in the target process' address -//! space of a nullptr terminated array of NUL-terminated strings. -//! \param[out] whitelist The whitelist read, valid only if this function +//! \param[in] list_address The address in the target process' address space of +//! a nullptr terminated array of NUL-terminated strings. +//! \param[out] allowed_annotations The list read, valid only if this function //! returns `true`. //! \return `true` on success, `false` on failure with a message logged. -bool ReadAnnotationsWhitelist(const ProcessMemoryRange& memory, - VMAddress whitelist_address, - std::vector* whitelist); +bool ReadAllowedAnnotations(const ProcessMemoryRange& memory, + VMAddress list_address, + std::vector* allowed_annotations); -//! \brief Reads a memory range whitelist from another process. +//! \brief Reads a list of allowed memory ranges from another process. //! //! \param[in] memory A memory reader for the target process. -//! \param[in] whitelist_address The address in the target process' address -//! space of a nullptr terminated array of NUL-terminated strings. -//! \param[out] whitelist A list of whitelisted memory regions, valid only if -//! this function returns `true`. +//! \param[in] list_address The address in the target process' address space of +//! a nullptr terminated array of NUL-terminated strings. +//! \param[out] allowed_memory_ranges A list of allowed memory regions, valid +//! only if this function returns `true`. //! \return `true` on success, `false` on failure with a message logged. -bool ReadMemoryRangeWhitelist( +bool ReadAllowedMemoryRanges( const ProcessMemoryRange& memory, - VMAddress whitelist_address, - std::vector>* whitelist); + VMAddress list_address, + std::vector>* allowed_memory_ranges); } // namespace crashpad diff --git a/snapshot/sanitized/sanitization_information_test.cc b/snapshot/sanitized/sanitization_information_test.cc index e426f28bec..1be887ab72 100644 --- a/snapshot/sanitized/sanitization_information_test.cc +++ b/snapshot/sanitized/sanitization_information_test.cc @@ -24,7 +24,7 @@ namespace crashpad { namespace test { namespace { -class WhitelistTest : public testing::Test { +class AllowedAnnotationsTest : public testing::Test { public: void SetUp() override { ASSERT_TRUE(memory_.Initialize(getpid())); @@ -36,33 +36,34 @@ class WhitelistTest : public testing::Test { } protected: - bool ReadWhitelist(const char* const* address) { - return ReadAnnotationsWhitelist( - range_, FromPointerCast(address), &whitelist_); + bool DoReadAllowedAnnotations(const char* const* address) { + return ReadAllowedAnnotations( + range_, FromPointerCast(address), &allowed_annotations_); } ProcessMemoryLinux memory_; ProcessMemoryRange range_; - std::vector whitelist_; + std::vector allowed_annotations_; }; -const char* const kEmptyWhitelist[] = {nullptr}; +const char* const kEmptyAllowedAnnotations[] = {nullptr}; -TEST_F(WhitelistTest, EmptyWhitelist) { - ASSERT_TRUE(ReadWhitelist(kEmptyWhitelist)); - EXPECT_EQ(whitelist_, std::vector()); +TEST_F(AllowedAnnotationsTest, EmptyAllowedAnnotations) { + ASSERT_TRUE(DoReadAllowedAnnotations(kEmptyAllowedAnnotations)); + EXPECT_EQ(allowed_annotations_, std::vector()); } -const char* const kNonEmptyWhitelist[] = {"string1", - "another_string", - "", - nullptr}; +const char* const kNonEmptyAllowedAnnotations[] = {"string1", + "another_string", + "", + nullptr}; -TEST_F(WhitelistTest, NonEmptyWhitelist) { - ASSERT_TRUE(ReadWhitelist(kNonEmptyWhitelist)); - ASSERT_EQ(whitelist_.size(), base::size(kNonEmptyWhitelist) - 1); - for (size_t index = 0; index < base::size(kNonEmptyWhitelist) - 1; ++index) { - EXPECT_EQ(whitelist_[index], kNonEmptyWhitelist[index]); +TEST_F(AllowedAnnotationsTest, NonEmptyAllowedAnnotations) { + ASSERT_TRUE(DoReadAllowedAnnotations(kNonEmptyAllowedAnnotations)); + ASSERT_EQ(allowed_annotations_.size(), + base::size(kNonEmptyAllowedAnnotations) - 1); + for (size_t index = 0; index < allowed_annotations_.size(); ++index) { + EXPECT_EQ(allowed_annotations_[index], kNonEmptyAllowedAnnotations[index]); } } diff --git a/util/process/process_memory_sanitized.cc b/util/process/process_memory_sanitized.cc index cafe5aef72..4d7a6854d9 100644 --- a/util/process/process_memory_sanitized.cc +++ b/util/process/process_memory_sanitized.cc @@ -28,17 +28,17 @@ namespace crashpad { ProcessMemorySanitized::ProcessMemorySanitized() - : ProcessMemory(), memory_(nullptr), whitelist_() {} + : ProcessMemory(), memory_(nullptr), allowed_ranges_() {} ProcessMemorySanitized::~ProcessMemorySanitized() {} bool ProcessMemorySanitized::Initialize( const ProcessMemory* memory, - const std::vector>* whitelist) { + const std::vector>* allowed_ranges) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); memory_ = memory; - if (whitelist) - whitelist_ = *whitelist; + if (allowed_ranges) + allowed_ranges_ = *allowed_ranges; INITIALIZATION_STATE_SET_VALID(initialized_); return true; } @@ -49,7 +49,7 @@ ssize_t ProcessMemorySanitized::ReadUpTo(VMAddress address, INITIALIZATION_STATE_DCHECK_VALID(initialized_); VMAddress end = address + size; - for (auto&& entry : whitelist_) { + for (auto&& entry : allowed_ranges_) { if (address >= entry.first && address < entry.second && end >= entry.first && end <= entry.second) { return memory_->ReadUpTo(address, size, buffer); @@ -57,7 +57,7 @@ ssize_t ProcessMemorySanitized::ReadUpTo(VMAddress address, } DLOG(ERROR) - << "ProcessMemorySanitized failed to read unwhitelisted region. address=" + << "ProcessMemorySanitized failed to read disallowed region. address=" << address << " size=" << size; return 0; } diff --git a/util/process/process_memory_sanitized.h b/util/process/process_memory_sanitized.h index 44439636cc..e050a47f62 100644 --- a/util/process/process_memory_sanitized.h +++ b/util/process/process_memory_sanitized.h @@ -34,25 +34,25 @@ class ProcessMemorySanitized final : public ProcessMemory { ~ProcessMemorySanitized(); //! \brief Initializes this object to read memory from the underlying - //! \a memory object if the memory range is in the provided \a whitelist. + //! \a memory object if the memory range is in \a allowed_ranges. //! //! This method must be called successfully prior to calling any other method //! in this class. //! - //! \param[in] memory The memory object to read whitelisted regions from. - //! \param[in] whitelist A whitelist of memory regions. + //! \param[in] memory The memory object to read memory from. + //! \param[in] allowed_ranges A list of allowed memory ranges. //! //! \return `true` on success, `false` on failure with a message logged. bool Initialize( const ProcessMemory* memory, - const std::vector>* whitelist); + const std::vector>* allowed_ranges); private: ssize_t ReadUpTo(VMAddress address, size_t size, void* buffer) const override; const ProcessMemory* memory_; InitializationStateDcheck initialized_; - std::vector> whitelist_; + std::vector> allowed_ranges_; DISALLOW_COPY_AND_ASSIGN(ProcessMemorySanitized); }; diff --git a/util/process/process_memory_sanitized_test.cc b/util/process/process_memory_sanitized_test.cc index ff5c9445a3..1a7f9c933e 100644 --- a/util/process/process_memory_sanitized_test.cc +++ b/util/process/process_memory_sanitized_test.cc @@ -23,7 +23,7 @@ namespace crashpad { namespace test { namespace { -TEST(ProcessMemorySanitized, DenyOnEmptyWhitelist) { +TEST(ProcessMemorySanitized, DenyDisallowedMemory) { ProcessMemoryNative memory; ASSERT_TRUE(memory.Initialize(GetSelfProcess())); @@ -34,25 +34,25 @@ TEST(ProcessMemorySanitized, DenyOnEmptyWhitelist) { san_null.Initialize(&memory, nullptr); EXPECT_FALSE(san_null.Read(FromPointerCast(&c), 1, &out)); - std::vector> whitelist; - ProcessMemorySanitized san_blank; - san_blank.Initialize(&memory, &whitelist); - EXPECT_FALSE(san_blank.Read(FromPointerCast(&c), 1, &out)); + std::vector> allowed_memory; + ProcessMemorySanitized san_empty; + san_empty.Initialize(&memory, &allowed_memory); + EXPECT_FALSE(san_empty.Read(FromPointerCast(&c), 1, &out)); } -TEST(ProcessMemorySanitized, WhitelistingWorks) { +TEST(ProcessMemorySanitized, AllowedMemory) { ProcessMemoryNative memory; ASSERT_TRUE(memory.Initialize(GetSelfProcess())); char str[4] = "ABC"; char out[4]; - std::vector> whitelist; - whitelist.push_back(std::make_pair(FromPointerCast(str + 1), - FromPointerCast(str + 2))); + std::vector> allowed_memory; + allowed_memory.push_back(std::make_pair(FromPointerCast(str + 1), + FromPointerCast(str + 2))); ProcessMemorySanitized sanitized; - sanitized.Initialize(&memory, &whitelist); + sanitized.Initialize(&memory, &allowed_memory); EXPECT_FALSE(sanitized.Read(FromPointerCast(str), 1, &out)); EXPECT_TRUE(sanitized.Read(FromPointerCast(str + 1), 1, &out)); From 0cccdc0b7ec32bd31e789b3945a08c04ef775834 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 6 Aug 2020 15:25:10 -0700 Subject: [PATCH 3/9] Replace is_linux with is_linux || is_chromeos Upstreams https://chromium-review.googlesource.com/c/chromium/src/+/2326293 Change-Id: I74379d1fbb8f9ec94e3e7eb969af0c8c492c9c07 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2342044 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- build/crashpad_buildconfig.gni | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/crashpad_buildconfig.gni b/build/crashpad_buildconfig.gni index d919a8b4be..a8bf6cf86e 100644 --- a/build/crashpad_buildconfig.gni +++ b/build/crashpad_buildconfig.gni @@ -38,7 +38,7 @@ if (crashpad_is_in_chromium) { crashpad_is_mac = is_mac crashpad_is_ios = is_ios crashpad_is_win = is_win - crashpad_is_linux = is_linux + crashpad_is_linux = is_linux || is_chromeos crashpad_is_android = is_android crashpad_is_fuchsia = is_fuchsia From 7547d0aa874f550fb3d3e8a5365ec51da96cbd86 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 13 Aug 2020 11:53:06 -0700 Subject: [PATCH 4/9] android: Remove orderfile configs from handler trampoline Upstreams https://chromium-review.googlesource.com/c/chromium/src/+/1505951 Change-Id: Id78d20308cfcbfba9b0f6535a8c69e9cbbf219e1 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2354278 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- handler/BUILD.gn | 2 ++ 1 file changed, 2 insertions(+) diff --git a/handler/BUILD.gn b/handler/BUILD.gn index 4fb7264937..dc481784f4 100644 --- a/handler/BUILD.gn +++ b/handler/BUILD.gn @@ -185,6 +185,8 @@ if (crashpad_is_android) { if (crashpad_is_in_chromium) { no_default_deps = true + remove_configs = + [ "//build/config/android:default_orderfile_instrumentation" ] } } } From b4724081350eff0a732a6be80dd9f6c5143fe259 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 13 Aug 2020 15:08:40 -0700 Subject: [PATCH 5/9] fuchsia: Remove run_tests.py functionality This removes the rotted functionality for running tests on Fuchsia. It had previously been broken by other platform changes. Other tools are from previous SDKs being removed too; this approach is no longer going to work. The preferred way is to connect via SSH to the device, however, that requires using the femu.sh from the SDK, which in turn requires `sudo` to create a network device, so it won't directly work on bots anyway. I started trying to update the to use femu.sh, fserve.sh, & fpublish.sh, but that requires building a .far, which uses GN templates which differ from the in-tree versions, and don't seem (?) to support packaging resources into the package. So, for now because it was confusing people (see linked bug) give up and delete the code for the time being. Bug: fuchsia:54031 Change-Id: Iac7af80094b150d11e71474cba4bd93eb8e80639 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2354160 Commit-Queue: Scott Graham Reviewed-by: Mark Mentovai --- DEPS | 20 ----- build/run_fuchsia_qemu.py | 145 ----------------------------------- build/run_tests.py | 157 -------------------------------------- 3 files changed, 322 deletions(-) delete mode 100755 build/run_fuchsia_qemu.py diff --git a/DEPS b/DEPS index 3640572496..6bd5494bb7 100644 --- a/DEPS +++ b/DEPS @@ -81,26 +81,6 @@ deps = { 'condition': 'checkout_fuchsia and host_os == "linux"', 'dep_type': 'cipd' }, - 'crashpad/third_party/fuchsia/qemu/mac-amd64': { - 'packages': [ - { - 'package': 'fuchsia/qemu/mac-amd64', - 'version': 'latest' - }, - ], - 'condition': 'checkout_fuchsia and host_os == "mac"', - 'dep_type': 'cipd' - }, - 'crashpad/third_party/fuchsia/qemu/linux-amd64': { - 'packages': [ - { - 'package': 'fuchsia/qemu/linux-amd64', - 'version': 'latest' - }, - ], - 'condition': 'checkout_fuchsia and host_os == "linux"', - 'dep_type': 'cipd' - }, 'crashpad/third_party/fuchsia/sdk/mac-amd64': { 'packages': [ { diff --git a/build/run_fuchsia_qemu.py b/build/run_fuchsia_qemu.py deleted file mode 100755 index 67463e63a5..0000000000 --- a/build/run_fuchsia_qemu.py +++ /dev/null @@ -1,145 +0,0 @@ -#!/usr/bin/env python - -# 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. -"""Helper script to [re]start or stop a helper Fuchsia QEMU instance to be used -for running tests without a device. -""" - -from __future__ import print_function - -import getpass -import os -import random -import signal -import subprocess -import sys -import tempfile -import time - -try: - from subprocess import DEVNULL -except ImportError: - DEVNULL = open(os.devnull, 'r+b') - -CRASHPAD_ROOT = os.path.join(os.path.dirname(os.path.abspath(__file__)), - os.pardir) - - -def _Stop(pid_file): - if os.path.isfile(pid_file): - with open(pid_file, 'rb') as f: - pid = int(f.read().strip()) - try: - os.kill(pid, signal.SIGTERM) - except: - print('Unable to kill pid %d, continuing' % pid, file=sys.stderr) - os.unlink(pid_file) - - -def _CheckForTun(): - """Check for networking. TODO(scottmg): Currently, this is Linux-specific. - """ - returncode = subprocess.call( - ['tunctl', '-b', '-u', - getpass.getuser(), '-t', 'qemu'], - stdout=DEVNULL, - stderr=DEVNULL) - if returncode != 0: - print('To use QEMU with networking on Linux, configure TUN/TAP. See:', - file=sys.stderr) - print( - ' https://fuchsia.googlesource.com/zircon/+/HEAD/docs/qemu.md#enabling-networking-under-qemu-x86_64-only', - file=sys.stderr) - return 2 - return 0 - - -def _Start(pid_file): - tun_result = _CheckForTun() - if tun_result != 0: - return tun_result - - arch = 'mac-amd64' if sys.platform == 'darwin' else 'linux-amd64' - fuchsia_dir = os.path.join(CRASHPAD_ROOT, 'third_party', 'fuchsia') - qemu_path = os.path.join(fuchsia_dir, 'qemu', arch, 'bin', - 'qemu-system-x86_64') - kernel_data_dir = os.path.join(fuchsia_dir, 'sdk', arch, 'target', 'x86_64') - kernel_path = os.path.join(kernel_data_dir, 'zircon.bin') - initrd_path = os.path.join(kernel_data_dir, 'bootdata.bin') - - mac_tail = ':'.join('%02x' % random.randint(0, 255) for x in range(3)) - instance_name = ( - 'crashpad_qemu_' + - ''.join(chr(random.randint(ord('A'), ord('Z'))) for x in range(8))) - - # These arguments are from the Fuchsia repo in zircon/scripts/run-zircon. - - # yapf: disable - popen = subprocess.Popen([ - qemu_path, - '-m', '2048', - '-nographic', - '-kernel', kernel_path, - '-initrd', initrd_path, - '-smp', '4', - '-serial', 'stdio', - '-monitor', 'none', - '-machine', 'q35', - '-cpu', 'host,migratable=no,+invtsc', - '-enable-kvm', - '-netdev', 'type=tap,ifname=qemu,script=no,downscript=no,id=net0', - '-device', 'e1000,netdev=net0,mac=52:54:00:' + mac_tail, - '-append', 'TERM=dumb zircon.nodename=' + instance_name, - ], - stdin=DEVNULL, - stdout=DEVNULL, - stderr=DEVNULL) - # yapf: enable - - with open(pid_file, 'wb') as f: - f.write('%d\n' % popen.pid) - - for i in range(10): - netaddr_path = os.path.join(fuchsia_dir, 'sdk', arch, 'tools', - 'netaddr') - if subprocess.call([netaddr_path, '--nowait', instance_name], - stdout=open(os.devnull), - stderr=open(os.devnull)) == 0: - break - time.sleep(.5) - else: - print('instance did not respond after start', file=sys.stderr) - return 1 - - return 0 - - -def main(args): - if len(args) != 1 or args[0] not in ('start', 'stop'): - print('usage: run_fuchsia_qemu.py start|stop', file=sys.stderr) - return 1 - - command = args[0] - - pid_file = os.path.join(tempfile.gettempdir(), 'crashpad_fuchsia_qemu_pid') - _Stop(pid_file) - if command == 'start': - return _Start(pid_file) - - return 0 - - -if __name__ == '__main__': - sys.exit(main(sys.argv[1:])) diff --git a/build/run_tests.py b/build/run_tests.py index 5e70d7b668..f48652ff78 100755 --- a/build/run_tests.py +++ b/build/run_tests.py @@ -313,144 +313,6 @@ def _adb_shell(command_args, env={}): _adb_shell(['rm', '-rf', device_temp_dir]) -def _GetFuchsiaSDKRoot(): - arch = 'mac-amd64' if sys.platform == 'darwin' else 'linux-amd64' - return os.path.join(CRASHPAD_DIR, 'third_party', 'fuchsia', 'sdk', arch) - - -def _GenerateFuchsiaRuntimeDepsFiles(binary_dir, tests): - """Ensures a /.runtime_deps file exists for each test.""" - targets_file = os.path.join(binary_dir, 'targets.txt') - with open(targets_file, 'wb') as f: - f.write('//:' + '\n//:'.join(tests) + '\n') - gn_path = _FindGNFromBinaryDir(binary_dir) - subprocess.check_call([ - gn_path, '--root=' + CRASHPAD_DIR, 'gen', binary_dir, - '--runtime-deps-list-file=' + targets_file - ]) - - # Run again so that --runtime-deps-list-file isn't in the regen rule. See - # https://crbug.com/814816. - subprocess.check_call( - [gn_path, '--root=' + CRASHPAD_DIR, 'gen', binary_dir]) - - -def _HandleOutputFromFuchsiaLogListener(process, done_message): - """Pass through the output from |process| (which should be an instance of - Fuchsia's loglistener) until a special termination |done_message| is - encountered. - - Also attempts to determine if any tests failed by inspecting the log output, - and returns False if there were failures. - """ - success = True - while True: - line = process.stdout.readline().rstrip() - if 'FAILED TEST' in line: - success = False - elif done_message in line and 'echo ' not in line: - break - print(line) - return success - - -def _RunOnFuchsiaTarget(binary_dir, test, device_name, extra_command_line): - """Runs the given Fuchsia |test| executable on the given |device_name|. The - device must already be booted. - - Copies the executable and its runtime dependencies as specified by GN to the - target in /tmp using `netcp`, runs the binary on the target, and logs output - back to stdout on this machine via `loglistener`. - """ - sdk_root = _GetFuchsiaSDKRoot() - - # Run loglistener and filter the output to know when the test is done. - loglistener_process = subprocess.Popen( - [os.path.join(sdk_root, 'tools', 'loglistener'), device_name], - stdout=subprocess.PIPE, - stdin=open(os.devnull), - stderr=open(os.devnull)) - - runtime_deps_file = os.path.join(binary_dir, test + '.runtime_deps') - with open(runtime_deps_file, 'rb') as f: - runtime_deps = f.read().splitlines() - - def netruncmd(*args): - """Runs a list of commands on the target device. Each command is escaped - by using pipes.quote(), and then each command is chained by shell ';'. - """ - netruncmd_path = os.path.join(sdk_root, 'tools', 'netruncmd') - final_args = ' ; '.join( - ' '.join(pipes.quote(x) for x in command) for command in args) - subprocess.check_call([netruncmd_path, device_name, final_args]) - - try: - unique_id = uuid.uuid4().hex - test_root = '/tmp/%s_%s' % (test, unique_id) - tmp_root = test_root + '/tmp' - staging_root = test_root + '/pkg' - - # Make a staging directory tree on the target. - directories_to_create = [ - tmp_root, - '%s/bin' % staging_root, - '%s/assets' % staging_root - ] - netruncmd(['mkdir', '-p'] + directories_to_create) - - def netcp(local_path): - """Uses `netcp` to copy a file or directory to the device. Files - located inside the build dir are stored to /pkg/bin, otherwise to - /pkg/assets. .so files are stored somewhere completely different, - into /boot/lib (!). This is because the loader service does not yet - correctly handle the namespace in which the caller is being run, and - so can only load .so files from a couple hardcoded locations, the - only writable one of which is /boot/lib, so we copy all .so files - there. This bug is filed upstream as ZX-1619. - """ - in_binary_dir = local_path.startswith(binary_dir + '/') - if in_binary_dir: - if local_path.endswith('.so'): - target_path = os.path.join('/boot/lib', - local_path[len(binary_dir) + 1:]) - else: - target_path = os.path.join(staging_root, 'bin', - local_path[len(binary_dir) + 1:]) - else: - relative_path = os.path.relpath(local_path, CRASHPAD_DIR) - target_path = os.path.join(staging_root, 'assets', - relative_path) - netcp_path = os.path.join(sdk_root, 'tools', 'netcp') - subprocess.check_call( - [netcp_path, local_path, device_name + ':' + target_path], - stderr=open(os.devnull)) - - # Copy runtime deps into the staging tree. - for dep in runtime_deps: - local_path = os.path.normpath(os.path.join(binary_dir, dep)) - if os.path.isdir(local_path): - for root, dirs, files in os.walk(local_path): - for f in files: - netcp(os.path.join(root, f)) - else: - netcp(local_path) - - done_message = 'TERMINATED: ' + unique_id - namespace_command = [ - 'namespace', '/pkg=' + staging_root, '/tmp=' + tmp_root, - '/svc=/svc', '--replace-child-argv0=/pkg/bin/' + test, '--', - staging_root + '/bin/' + test - ] + extra_command_line - netruncmd(namespace_command, ['echo', done_message]) - - success = _HandleOutputFromFuchsiaLogListener(loglistener_process, - done_message) - if not success: - raise subprocess.CalledProcessError(1, test) - finally: - netruncmd(['rm', '-rf', test_root]) - - def _RunOnIOSTarget(binary_dir, test, is_xcuitest=False): """Runs the given iOS |test| app on iPhone 8 with the default OS version.""" @@ -544,7 +406,6 @@ def main(args): target_os = _BinaryDirTargetOS(args.binary_dir) is_android = target_os == 'android' - is_fuchsia = target_os == 'fuchsia' is_ios = target_os == 'ios' tests = [ @@ -575,21 +436,6 @@ def main(args): return 2 android_device = devices[0] print('Using autodetected Android device:', android_device) - elif is_fuchsia: - zircon_nodename = os.environ.get('ZIRCON_NODENAME') - if not zircon_nodename: - netls = os.path.join(_GetFuchsiaSDKRoot(), 'tools', 'netls') - popen = subprocess.Popen([netls, '--nowait'], - stdout=subprocess.PIPE) - devices = popen.communicate()[0].splitlines() - if popen.returncode != 0 or len(devices) != 1: - print("Please set ZIRCON_NODENAME to your device's hostname", - file=sys.stderr) - return 2 - zircon_nodename = devices[0].strip().split()[1] - print('Using autodetected Fuchsia device:', zircon_nodename) - _GenerateFuchsiaRuntimeDepsFiles( - args.binary_dir, [t for t in tests if not t.endswith('.py')]) elif is_ios: tests.append('ios_crash_xcuitests') elif IS_WINDOWS_HOST: @@ -618,9 +464,6 @@ def main(args): if is_android: _RunOnAndroidTarget(args.binary_dir, test, android_device, extra_command_line) - elif is_fuchsia: - _RunOnFuchsiaTarget(args.binary_dir, test, zircon_nodename, - extra_command_line) elif is_ios: _RunOnIOSTarget(args.binary_dir, test, From ebab28f30d33235b4245c7b212e8b17ea3503a67 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 18 Aug 2020 09:56:41 -0400 Subject: [PATCH 6/9] Remove a variable-length array* to enable compiling with -Wvla * that hopefully never actually materialized Change-Id: Ic8625c0edf773a2dd5f0c40b7f293ec5492ce101 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2363147 Reviewed-by: Robert Sesek Commit-Queue: Mark Mentovai --- snapshot/mac/process_types.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/snapshot/mac/process_types.cc b/snapshot/mac/process_types.cc index 0a1b7f9504..92bc722b5d 100644 --- a/snapshot/mac/process_types.cc +++ b/snapshot/mac/process_types.cc @@ -219,7 +219,7 @@ inline void Assign(UInt64Array4* destination, size_t count, \ struct_name* specific) { \ return process_reader->Memory()->Read( \ - address, sizeof(struct_name[count]), specific); \ + address, count * sizeof(struct_name), specific); \ } \ \ } /* namespace internal */ \ From 1509aadd63643e53430d7d6d09d7727ae1f010f5 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 18 Aug 2020 16:56:02 -0400 Subject: [PATCH 7/9] Remove a variable-length array to enable compiling with -Wvla Change-Id: I721f1dccc7e1188631a57f435c5c097ff83853b8 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2363768 Reviewed-by: Joshua Peraza Commit-Queue: Mark Mentovai --- snapshot/sanitized/process_snapshot_sanitized_test.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/snapshot/sanitized/process_snapshot_sanitized_test.cc b/snapshot/sanitized/process_snapshot_sanitized_test.cc index 0484778344..26bd8d7702 100644 --- a/snapshot/sanitized/process_snapshot_sanitized_test.cc +++ b/snapshot/sanitized/process_snapshot_sanitized_test.cc @@ -14,8 +14,11 @@ #include "snapshot/sanitized/process_snapshot_sanitized.h" +#include + #include "base/macros.h" #include "base/notreached.h" +#include "base/stl_util.h" #include "build/build_config.h" #include "gtest/gtest.h" #include "test/multiprocess_exec.h" @@ -98,7 +101,7 @@ void ChildTestFunction() { static StringAnnotation<32> non_allowed_annotation(kNonAllowedAnnotationName); non_allowed_annotation.Set(kNonAllowedAnnotationValue); - char string_data[strlen(kSensitiveStackData) + 1]; + char string_data[base::size(kSensitiveStackData)]; strcpy(string_data, kSensitiveStackData); void (*code_pointer)(void) = ChildTestFunction; From 45ca490687af63766240e5a113f231b0e14473d7 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 18 Aug 2020 17:57:08 -0400 Subject: [PATCH 8/9] Build with -Wvla, forbidding stack-allocated variable-length arrays Update mini_chromium to f3cfec80ca521881c97629adf6fcdf21158d635d 4b41c7657578 Add DISABLE_CFI_ICALL and NO_SANITIZE macros f3cfec80ca52 Build with -Wvla, forbidding stack-allocated variable- length arrays Change-Id: Ic6342ac9fee49061f9086ffb79a061b104168eef Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2363827 Reviewed-by: Joshua Peraza Commit-Queue: Mark Mentovai --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 6bd5494bb7..66c24df3f9 100644 --- a/DEPS +++ b/DEPS @@ -42,7 +42,7 @@ deps = { '7bde79cc274d06451bf65ae82c012a5d3e476b5a', 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '891e31d0b649ec9fa98ca5745f9dcd14a34de34c', + 'f3cfec80ca521881c97629adf6fcdf21158d635d', 'crashpad/third_party/libfuzzer/src': Var('chromium_git') + '/chromium/llvm-project/compiler-rt/lib/fuzzer.git@' + 'fda403cf93ecb8792cb1d061564d89a6553ca020', From 59e8120e7ada3608f68f42cbbab423bcc163a326 Mon Sep 17 00:00:00 2001 From: Shai Barack Date: Sun, 9 Aug 2020 09:51:01 -0700 Subject: [PATCH 9/9] [Wconversion] Suppress warnings on Fuchsia Bug: fuchsia:56258 Change-Id: I6bdc0b81a0294040e4dceb18576ab38c45a430e4 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2345384 Reviewed-by: Scott Graham Commit-Queue: Scott Graham --- client/BUILD.gn | 1 + compat/BUILD.gn | 4 ++++ snapshot/BUILD.gn | 8 ++++++++ util/BUILD.gn | 4 ++++ 4 files changed, 17 insertions(+) diff --git a/client/BUILD.gn b/client/BUILD.gn index f448d35e36..10b994f8e5 100644 --- a/client/BUILD.gn +++ b/client/BUILD.gn @@ -116,6 +116,7 @@ static_library("client") { deps += [ "../third_party/fuchsia" ] if (crashpad_is_in_fuchsia) { deps += [ "//sdk/lib/fdio" ] + configs += [ "//build/config:Wno-conversion" ] } } } diff --git a/compat/BUILD.gn b/compat/BUILD.gn index f2ffcf5f10..d3045ebf55 100644 --- a/compat/BUILD.gn +++ b/compat/BUILD.gn @@ -171,4 +171,8 @@ compat_target("compat") { if (!crashpad_is_linux && !crashpad_is_android && !crashpad_is_fuchsia) { deps += [ "../third_party/glibc" ] } + + if (crashpad_is_in_fuchsia) { + configs = [ "//build/config:Wno-conversion" ] + } } diff --git a/snapshot/BUILD.gn b/snapshot/BUILD.gn index 8615c99ab3..8da697d87d 100644 --- a/snapshot/BUILD.gn +++ b/snapshot/BUILD.gn @@ -261,6 +261,10 @@ static_library("snapshot") { } configs += [ "..:disable_ubsan" ] + + if (crashpad_is_in_fuchsia) { + configs += [ "//build/config:Wno-conversion" ] + } } # :context is the only part of snapshot that minidump may depend on. @@ -281,6 +285,10 @@ static_library("context") { if (crashpad_is_win) { cflags = [ "/wd4201" ] # nonstandard extension used : nameless struct/union } + + if (crashpad_is_in_fuchsia) { + configs += [ "//build/config:Wno-conversion" ] + } } if (crashpad_is_linux) { diff --git a/util/BUILD.gn b/util/BUILD.gn index 2e1bbee310..4654fabb3d 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -612,6 +612,10 @@ static_library("util") { } configs += [ "..:disable_ubsan" ] + + if (crashpad_is_in_fuchsia) { + configs += [ "//build/config:Wno-conversion" ] + } } if (!crashpad_is_android && !crashpad_is_ios) {