From 2f6cffa676795d3ac66e50d5fde4a286edfc9377 Mon Sep 17 00:00:00 2001 From: Leonard Grey Date: Fri, 13 Oct 2023 12:42:13 -0400 Subject: [PATCH] Mac: don't consider module order in process reader tests This is a follow-up to 0fc1b6ae780e7ba854652bd5581f936abf824a5e. The change in macOS 14's dyld to insert new modules in the front of `dyld_all_image_infos` means that if any images are loaded after the executable and its direct dependencies, it's no longer possible to rotate the list to match the order used by the `dyld_get_image...` APIs. This forces us to dispense with checking the order at all except to ensure that the executable is first, and dyld itself is last. Additionally fixes an unreachable return introduced in 0fc1b6ae780e7ba854652bd5581f936abf824a5e. Bug: chromium:1452203 Change-Id: If0b09b9110d8f60d29cca79ea6a59050b0293c5e Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4935952 Commit-Queue: Leonard Grey Reviewed-by: Mark Mentovai --- snapshot/mac/mach_o_image_segment_reader.cc | 4 +- snapshot/mac/process_reader_mac_test.cc | 152 +++++++++++--------- 2 files changed, 90 insertions(+), 66 deletions(-) diff --git a/snapshot/mac/mach_o_image_segment_reader.cc b/snapshot/mac/mach_o_image_segment_reader.cc index efa7cc68..75f229ef 100644 --- a/snapshot/mac/mach_o_image_segment_reader.cc +++ b/snapshot/mac/mach_o_image_segment_reader.cc @@ -56,9 +56,9 @@ bool IsMalformedCLKernelsModule(uint32_t mach_o_file_type, 0, strlen(kCvmsObjectPathPrefix), kCvmsObjectPathPrefix) == 0 && (__MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_14 || MacOSVersionNumber() >= 10'14'00); -#endif // ARCH_CPU_X86_FAMILY - +#else return false; +#endif // ARCH_CPU_X86_FAMILY } MachOImageSegmentReader::MachOImageSegmentReader() diff --git a/snapshot/mac/process_reader_mac_test.cc b/snapshot/mac/process_reader_mac_test.cc index a6e009e9..ed699237 100644 --- a/snapshot/mac/process_reader_mac_test.cc +++ b/snapshot/mac/process_reader_mac_test.cc @@ -28,6 +28,7 @@ #include #include +#include #include #include "base/apple/mach_logging.h" @@ -54,6 +55,15 @@ namespace crashpad { namespace test { namespace { +using ModulePathAndAddress = std::pair; +struct PathAndAddressHash { + std::size_t operator()(const ModulePathAndAddress& pair) const { + return std::hash()(pair.first) ^ + std::hash()(pair.second); + } +}; +using ModuleSet = std::unordered_set; + constexpr char kDyldPath[] = "/usr/lib/dyld"; TEST(ProcessReaderMac, SelfBasic) { @@ -833,54 +843,57 @@ TEST(ProcessReaderMac, SelfModules) { ASSERT_TRUE(process_reader.Initialize(mach_task_self())); uint32_t dyld_image_count = _dyld_image_count(); - const std::vector& modules = - process_reader.Modules(); - // There needs to be at least an entry for the main executable, for a dylib, - // and for dyld. - ASSERT_GE(modules.size(), 3u); - - // dyld_image_count doesn’t include an entry for dyld itself, but |modules| - // does. - ASSERT_EQ(modules.size(), dyld_image_count + 1); - - bool found_cl_kernels = false; - for (uint32_t index = 0; index < dyld_image_count; ++index) { - SCOPED_TRACE(base::StringPrintf( - "index %u, name %s", index, modules[index].name.c_str())); - - const char* dyld_image_name = _dyld_get_image_name(index); - EXPECT_EQ(modules[index].name, dyld_image_name); - ASSERT_TRUE(modules[index].reader); - EXPECT_EQ( - modules[index].reader->Address(), - FromPointerCast(_dyld_get_image_header(index))); - - if (index == 0 && MacOSVersionNumber() < 12'00'00) { - // Pre-dyld4, dyld didn’t set the main executable's timestamp, and it was - // reported as 0. - EXPECT_EQ(modules[index].timestamp, 0); - } else if (IsMalformedCLKernelsModule(modules[index].reader->FileType(), - modules[index].name)) { - found_cl_kernels = true; + std::set cl_kernel_names; + auto modules = process_reader.Modules(); + ModuleSet actual_modules; + for (size_t i = 0; i < modules.size(); ++i) { + auto& module = modules[i]; + ASSERT_TRUE(module.reader); + if (i == modules.size() - 1) { + EXPECT_EQ(module.name, kDyldPath); + const dyld_all_image_infos* dyld_image_infos = DyldGetAllImageInfos(); + if (dyld_image_infos->version >= 2) { + EXPECT_EQ(module.reader->Address(), + FromPointerCast( + dyld_image_infos->dyldImageLoadAddress)); + } + // Don't include dyld, since dyld image APIs will not have an entry for + // dyld itself. + continue; + } + // Ensure executable is first, and that there's only one. + uint32_t file_type = module.reader->FileType(); + if (i == 0) { + EXPECT_EQ(file_type, static_cast(MH_EXECUTE)); } else { - // Hope that the module didn’t change on disk. + EXPECT_NE(file_type, static_cast(MH_EXECUTE)); + } + if (IsMalformedCLKernelsModule(module.reader->FileType(), module.name)) { + cl_kernel_names.insert(module.name); + } + actual_modules.insert( + std::make_pair(module.name, module.reader->Address())); + } + EXPECT_EQ(cl_kernel_names.size() > 0, + ExpectCLKernels() && ensure_cl_kernels.success()); + + // There needs to be at least an entry for the main executable and a dylib. + ASSERT_GE(actual_modules.size(), 2u); + ASSERT_EQ(actual_modules.size(), dyld_image_count); + + ModuleSet expect_modules; + for (uint32_t index = 0; index < dyld_image_count; ++index) { + const char* dyld_image_name = _dyld_get_image_name(index); + mach_vm_address_t dyld_image_address = + FromPointerCast(_dyld_get_image_header(index)); + expect_modules.insert( + std::make_pair(std::string(dyld_image_name), dyld_image_address)); + if (cl_kernel_names.find(dyld_image_name) == cl_kernel_names.end()) { VerifyImageExistence(dyld_image_name); } } - - EXPECT_EQ(found_cl_kernels, ExpectCLKernels() && ensure_cl_kernels.success()); - - size_t index = modules.size() - 1; - EXPECT_EQ(modules[index].name, kDyldPath); - - 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(), - FromPointerCast( - dyld_image_infos->dyldImageLoadAddress)); - } + EXPECT_EQ(actual_modules, expect_modules); } class ProcessReaderModulesChild final : public MachMultiprocess { @@ -899,27 +912,45 @@ class ProcessReaderModulesChild final : public MachMultiprocess { void MachMultiprocessParent() override { ProcessReaderMac process_reader; ASSERT_TRUE(process_reader.Initialize(ChildTask())); - const std::vector& modules = process_reader.Modules(); + ModuleSet actual_modules; + std::set cl_kernel_names; + for (size_t i = 0; i < modules.size(); ++i) { + auto& module = modules[i]; + ASSERT_TRUE(module.reader); + uint32_t file_type = module.reader->FileType(); + if (i == 0) { + EXPECT_EQ(file_type, static_cast(MH_EXECUTE)); + } else if (i == modules.size() - 1) { + EXPECT_EQ(file_type, static_cast(MH_DYLINKER)); + + } else { + EXPECT_NE(file_type, static_cast(MH_EXECUTE)); + EXPECT_NE(file_type, static_cast(MH_DYLINKER)); + } + if (IsMalformedCLKernelsModule(module.reader->FileType(), module.name)) { + cl_kernel_names.insert(module.name); + } + actual_modules.insert( + std::make_pair(module.name, module.reader->Address())); + } + // There needs to be at least an entry for the main executable, for a dylib, // and for dyld. - ASSERT_GE(modules.size(), 3u); + ASSERT_GE(actual_modules.size(), 3u); FileHandle read_handle = ReadPipeHandle(); - uint32_t expect_modules; + uint32_t expect_modules_size; CheckedReadFileExactly( - read_handle, &expect_modules, sizeof(expect_modules)); + read_handle, &expect_modules_size, sizeof(expect_modules_size)); - ASSERT_EQ(modules.size(), expect_modules); - - bool found_cl_kernels = false; - for (size_t index = 0; index < modules.size(); ++index) { - SCOPED_TRACE(base::StringPrintf( - "index %zu, name %s", index, modules[index].name.c_str())); + ASSERT_EQ(actual_modules.size(), expect_modules_size); + ModuleSet expect_modules; + for (size_t index = 0; index < expect_modules_size; ++index) { uint32_t expect_name_length; CheckedReadFileExactly( read_handle, &expect_name_length, sizeof(expect_name_length)); @@ -927,25 +958,18 @@ class ProcessReaderModulesChild final : public MachMultiprocess { // The NUL terminator is not read. std::string expect_name(expect_name_length, '\0'); CheckedReadFileExactly(read_handle, &expect_name[0], expect_name_length); - EXPECT_EQ(modules[index].name, expect_name); mach_vm_address_t expect_address; CheckedReadFileExactly( read_handle, &expect_address, sizeof(expect_address)); - ASSERT_TRUE(modules[index].reader); - EXPECT_EQ(modules[index].reader->Address(), expect_address); - - if (IsMalformedCLKernelsModule(modules[index].reader->FileType(), - modules[index].name)) { - found_cl_kernels = true; - } else { - // Hope that the module didn’t change on disk. + expect_modules.insert(std::make_pair(expect_name, expect_address)); + if (cl_kernel_names.find(expect_name) == cl_kernel_names.end()) { VerifyImageExistence(expect_name.c_str()); } } - - EXPECT_EQ(found_cl_kernels, + EXPECT_EQ(cl_kernel_names.size() > 0, ExpectCLKernels() && ensure_cl_kernels_success_); + EXPECT_EQ(expect_modules, actual_modules); } void MachMultiprocessChild() override {