Fix crashpad tests under UBSan

These are slightly frustrating. First, when a struct is packed, some of
its fields may be underaligned. This is fine for direct access
(foo.bar), but if one takes the address if the field, this creates an
unaligned pointer. Dereferencing that pointer is then UB. (I'm not sure
if creating that pointer is UB.)

Crashpad seemingly doesn't do this, but it uses EXPECT_EQ from GTest.
EXPECT_EQ seems to internally take pointers to its arguments. I'm
guessing it binds them by const reference. This then trips UBSan. To
avoid this, we can copy the value into a temporary before passing to
EXPECT_EQ.

Second, the test to divide by 0 to trigger SIGFPE is undefined behavior.
The compiler is not actually obligated to trip SIGFPE. UBSan prints one
of its errors instead. Instead, since this file is only built on POSIX
anyway, use GCC inline assembly to do the division. That one is
well-defined.

Finally, casting a string to uint32_t* is undefined both by alignment
and by strict aliasing (although Chromium doesn't enable the latter).
Instead, type-punning should be done with memcpy.

Bug: chromium:1394755
Change-Id: I79108773a04ac26f5189e7b88a0acbf62eb4401d
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4985905
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
This commit is contained in:
David Benjamin 2023-10-27 21:26:50 -04:00 committed by Crashpad LUCI CQ
parent 4a93d7f4c4
commit 41f6ad560f
4 changed files with 18 additions and 8 deletions

View File

@ -253,7 +253,10 @@ class ExceptionHandlerServerTest : public testing::TestWithParam<bool> {
pid_t last_client; pid_t last_client;
ASSERT_TRUE(server_test_->Delegate()->WaitForException( ASSERT_TRUE(server_test_->Delegate()->WaitForException(
5.0, &last_client, &last_address)); 5.0, &last_client, &last_address));
EXPECT_EQ(last_address, info.exception_information_address); // `exception_information_address` is underaligned and `EXPECT_EQ`
// internally takes arguments by reference. Copy it into a temporary
// before comparing to avoid undefined behavior.
EXPECT_EQ(last_address, VMAddress{info.exception_information_address});
EXPECT_EQ(last_client, ChildPID()); EXPECT_EQ(last_client, ChildPID());
} else { } else {
CheckedReadFileAtEOF(ReadPipeHandle()); CheckedReadFileAtEOF(ReadPipeHandle());

View File

@ -178,8 +178,11 @@ void ExpectMiscInfoEqual<MINIDUMP_MISC_INFO_5>(
expected_misc_info.XStateData.SizeOfInfo); expected_misc_info.XStateData.SizeOfInfo);
EXPECT_EQ(observed_misc_info.XStateData.ContextSize, EXPECT_EQ(observed_misc_info.XStateData.ContextSize,
expected_misc_info.XStateData.ContextSize); expected_misc_info.XStateData.ContextSize);
EXPECT_EQ(observed_misc_info.XStateData.EnabledFeatures, // `EnabledFeatures` is underaligned and `EXPECT_EQ` internally takes
expected_misc_info.XStateData.EnabledFeatures); // arguments by reference. Copy it into a temporary before comparing to avoid
// undefined behavior.
EXPECT_EQ(uint64_t{observed_misc_info.XStateData.EnabledFeatures},
uint64_t{expected_misc_info.XStateData.EnabledFeatures});
for (size_t feature_index = 0; for (size_t feature_index = 0;
feature_index < std::size(observed_misc_info.XStateData.Features); feature_index < std::size(observed_misc_info.XStateData.Features);
++feature_index) { ++feature_index) {

View File

@ -868,8 +868,9 @@ TEST(ProcessSnapshotMinidump, ThreadsWithNames) {
} }
TEST(ProcessSnapshotMinidump, System) { TEST(ProcessSnapshotMinidump, System) {
const char* cpu_info = "GenuineIntel"; const char cpu_info[] = "GenuineIntel";
const uint32_t* cpu_info_bytes = reinterpret_cast<const uint32_t*>(cpu_info); uint32_t cpu_info_bytes[3];
memcpy(cpu_info_bytes, cpu_info, sizeof(cpu_info_bytes));
StringFile string_file; StringFile string_file;
MINIDUMP_HEADER header = {}; MINIDUMP_HEADER header = {};

View File

@ -164,9 +164,12 @@ void CauseSignal(int sig, int code) {
* Arm architecture. * Arm architecture.
*/ */
#if defined(ARCH_CPU_X86_FAMILY) #if defined(ARCH_CPU_X86_FAMILY)
[[maybe_unused]] volatile int a = 42; // Dividing by zero is undefined in C, so the compiler is permitted to
volatile int b = 0; // optimize out the division. Instead, divide using inline assembly. As
a = a / b; // this instruction will trap anyway, we skip declaring any clobbers or
// output registers.
int a = 42, b = 0;
asm volatile("idivl %2" : : "a"(0), "d"(a), "r"(b));
#endif #endif
break; break;
} }