From 594eb43b589dd07b2461987ee2704171056ee768 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 7 Feb 2017 13:57:09 -0500 Subject: [PATCH] mac: Make 64-bit handler able to read 32-bit module lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 32-bit process_types definition of dyld_all_image_infos winds up with four extra bytes of tail padding when built into a 64-bit crashpad_handler compared to a 32-bit one, and compared to the structure’s native size. This prevents a 64-bit crashpad_handler from being able to create a module snapshot of a 32-bit process for dyld_all_image_infos versions 14 (since 10.9) and 15 (since 10.12). Work around this by placing a zero-length “end” marker and using offsetof(dyld_all_image_infos, end) in preference to sizeof(dyld_all_image_infos). BUG=crashpad:156,crashpad:148,crashpad:120 TEST=crashpad_snapshot_test ProcessTypes.DyldImagesSelf, run_with_crashpad --handler=crashpad_handler{,32} builtin_trap{,32} Change-Id: I406ad53851b5bd29ec802b7ad3073836ebe8c34c Reviewed-on: https://chromium-review.googlesource.com/437924 Reviewed-by: Robert Sesek --- snapshot/mac/process_types.h | 16 ++++- snapshot/mac/process_types/custom.cc | 65 ++++++++----------- .../mac/process_types/dyld_images.proctype | 7 ++ snapshot/mac/process_types/traits.h | 4 +- snapshot/mac/process_types_test.cc | 37 +++++++++++ 5 files changed, 86 insertions(+), 43 deletions(-) diff --git a/snapshot/mac/process_types.h b/snapshot/mac/process_types.h index 664b37b6..7274515f 100644 --- a/snapshot/mac/process_types.h +++ b/snapshot/mac/process_types.h @@ -27,13 +27,21 @@ namespace crashpad { namespace process_types { namespace internal { +// Some structure definitions have tail padding when built in a 64-bit +// environment, but will have less (or no) tail padding in a 32-bit environment. +// These structure’s apparent sizes will differ between these two environments, +// which is incorrect. Use this “Nothing” type to place an “end” marker after +// all of the real members of such structures, and use offsetof(type, end) to +// compute their actual sizes. +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 structures more precisely. -using Reserved32_64Only32 = char[0]; +using Reserved32_64Only32 = Nothing; using Reserved32_64Only64 = uint32_t; -using Reserved64_64Only32 = char[0]; +using Reserved64_64Only32 = Nothing; using Reserved64_64Only64 = uint64_t; } // namespace internal @@ -65,6 +73,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using UIntPtr = internal::TraitsGeneric::UIntPtr; \ using Reserved32_64Only = internal::TraitsGeneric::Reserved32_64Only; \ using Reserved64_64Only = internal::TraitsGeneric::Reserved64_64Only; \ + using Nothing = internal::TraitsGeneric::Nothing; \ \ /* Initializes an object with data read from |process_reader| at \ * |address|, properly genericized. */ \ @@ -88,7 +97,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) /* Similar to Size(), but computes the expected size of a structure based \ * on the process’ bitness. This can be used prior to reading any data \ * from a process. */ \ - static size_t ExpectedSize(ProcessReader* process_reader); \ + static size_t ExpectedSize(ProcessReader* process_reader); #define PROCESS_TYPE_STRUCT_MEMBER(member_type, member_name, ...) \ member_type member_name __VA_ARGS__; @@ -155,6 +164,7 @@ DECLARE_PROCESS_TYPE_TRAITS_CLASS(Generic, 64) using UIntPtr = typename Traits::UIntPtr; \ using Reserved32_64Only = typename Traits::Reserved32_64Only; \ using Reserved64_64Only = typename Traits::Reserved64_64Only; \ + using Nothing = typename Traits::Nothing; \ \ /* Read(), ReadArrayInto(), and Size() are as in the generic user-visible \ * struct above. */ \ diff --git a/snapshot/mac/process_types/custom.cc b/snapshot/mac/process_types/custom.cc index 5fe92288..33e3a7ec 100644 --- a/snapshot/mac/process_types/custom.cc +++ b/snapshot/mac/process_types/custom.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/logging.h" #include "snapshot/mac/process_types/internal.h" #include "util/mach/task_memory.h" @@ -62,46 +64,31 @@ bool ReadIntoVersioned(ProcessReader* process_reader, template size_t dyld_all_image_infos::ExpectedSizeForVersion( decltype(dyld_all_image_infos::version) version) { - if (version >= 14) { - return sizeof(dyld_all_image_infos); + const size_t kSizeForVersion[] = { + offsetof(dyld_all_image_infos, infoArrayCount), // 0 + offsetof(dyld_all_image_infos, libSystemInitialized), // 1 + offsetof(dyld_all_image_infos, jitInfo), // 2 + offsetof(dyld_all_image_infos, dyldVersion), // 3 + offsetof(dyld_all_image_infos, dyldVersion), // 4 + offsetof(dyld_all_image_infos, coreSymbolicationShmPage), // 5 + offsetof(dyld_all_image_infos, systemOrderFlag), // 6 + offsetof(dyld_all_image_infos, uuidArrayCount), // 7 + offsetof(dyld_all_image_infos, dyldAllImageInfosAddress), // 8 + offsetof(dyld_all_image_infos, initialImageCount), // 9 + offsetof(dyld_all_image_infos, errorKind), // 10 + 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 + }; + + if (version >= arraysize(kSizeForVersion)) { + return kSizeForVersion[arraysize(kSizeForVersion) - 1]; } - if (version >= 13) { - return offsetof(dyld_all_image_infos, infoArrayChangeTimestamp); - } - if (version >= 12) { - return offsetof(dyld_all_image_infos, sharedCacheUUID); - } - if (version >= 11) { - return offsetof(dyld_all_image_infos, sharedCacheSlide); - } - if (version >= 10) { - return offsetof(dyld_all_image_infos, errorKind); - } - if (version >= 9) { - return offsetof(dyld_all_image_infos, initialImageCount); - } - if (version >= 8) { - return offsetof(dyld_all_image_infos, dyldAllImageInfosAddress); - } - if (version >= 7) { - return offsetof(dyld_all_image_infos, uuidArrayCount); - } - if (version >= 6) { - return offsetof(dyld_all_image_infos, systemOrderFlag); - } - if (version >= 5) { - return offsetof(dyld_all_image_infos, coreSymbolicationShmPage); - } - if (version >= 3) { - return offsetof(dyld_all_image_infos, dyldVersion); - } - if (version >= 2) { - return offsetof(dyld_all_image_infos, jitInfo); - } - if (version >= 1) { - return offsetof(dyld_all_image_infos, libSystemInitialized); - } - return offsetof(dyld_all_image_infos, infoArrayCount); + + static_assert(std::is_unsigned::value, + "version must be unsigned"); + return kSizeForVersion[version]; } // static diff --git a/snapshot/mac/process_types/dyld_images.proctype b/snapshot/mac/process_types/dyld_images.proctype index eb8410aa..3470ee83 100644 --- a/snapshot/mac/process_types/dyld_images.proctype +++ b/snapshot/mac/process_types/dyld_images.proctype @@ -123,6 +123,13 @@ PROCESS_TYPE_STRUCT_BEGIN(dyld_all_image_infos) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_6) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_7) PROCESS_TYPE_STRUCT_MEMBER(Reserved64_64Only, reserved_8) + + // 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) 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 fec234d0..396799be 100644 --- a/snapshot/mac/process_types/traits.h +++ b/snapshot/mac/process_types/traits.h @@ -22,7 +22,8 @@ // DECLARE_PROCESS_TYPE_TRAITS_CLASS before #including this file again and after // the last #include of this file. // -// |Reserved*| are used for padding fields that may be zero-length, and thus +// |Reserved*| is used for padding fields that may be zero-length, and |Nothing| +// is always zero-length and is used solely as an anchor to compute offsets. // __VA_ARGS__, which is intended to set the alignment of the 64-bit types, is // not used for those type aliases. #define DECLARE_PROCESS_TYPE_TRAITS_CLASS(traits_name, lp_bits, ...) \ @@ -37,6 +38,7 @@ using UIntPtr = uint##lp_bits##_t __VA_ARGS__; \ 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 ce092fe1..8525c14e 100644 --- a/snapshot/mac/process_types_test.cc +++ b/snapshot/mac/process_types_test.cc @@ -24,6 +24,7 @@ #include "base/strings/stringprintf.h" #include "build/build_config.h" #include "gtest/gtest.h" +#include "snapshot/mac/process_types/internal.h" #include "test/mac/dyld.h" #include "util/mac/mac_util.h" #include "util/misc/implicit_cast.h" @@ -116,6 +117,42 @@ TEST(ProcessTypes, DyldImagesSelf) { process_types::dyld_all_image_infos::ExpectedSizeForVersion( &process_reader, kDyldAllImageInfosVersionInSDK)); + // Make sure that the computed sizes of various versions of this structure are + // correct at different bitnessses. + const struct { + uint32_t version; + size_t size_32; + size_t size_64; + } kVersionsAndSizes[] = { + {1, 17, 25}, + {2, 24, 40}, + {3, 28, 48}, + {5, 40, 72}, + {6, 44, 80}, + {7, 48, 88}, + {8, 56, 104}, + {9, 60, 112}, + {10, 64, 120}, + {11, 80, 152}, + {12, 84, 160}, + {13, 104, 184}, + {14, 164, 304}, + {15, 164, 304}, + }; + for (size_t index = 0; index < arraysize(kVersionsAndSizes); ++index) { + uint32_t version = kVersionsAndSizes[index].version; + SCOPED_TRACE(base::StringPrintf("index %zu, version %u", index, version)); + + EXPECT_EQ(kVersionsAndSizes[index].size_32, + process_types::internal::dyld_all_image_infos< + process_types::internal::Traits32>:: + ExpectedSizeForVersion(version)); + EXPECT_EQ(kVersionsAndSizes[index].size_64, + process_types::internal::dyld_all_image_infos< + process_types::internal::Traits64>:: + ExpectedSizeForVersion(version)); + } + process_types::dyld_all_image_infos proctype_image_infos; ASSERT_TRUE(proctype_image_infos.Read(&process_reader, dyld_info.all_image_info_addr));