Skip to content

Commit 2efc09c

Browse files
author
Kostya Kortchinsky
committed
[scudo][standalone] Fix tests under ASan/UBSan
Fix a potential UB in `appendSignedDecimal` (with -INT64_MIN) by making it a special case. Fix the terrible test cases for `isOwned`: I was pretty sloppy on those and used some stack & static variables, but since `isOwned` accesses memory prior to the pointer to check for the validity of the Scudo header, it ended up being detected as some global and stack buffer out of bounds accesses. So not I am using buffers with enough room so that the test will not access memory prior to the variables. With those fixes, the tests pass on the ASan+UBSan Fuchsia build. Thanks to Roland for pointing those out! Differential Revision: https://reviews.llvm.org/D88170
1 parent c96d0cc commit 2efc09c

File tree

2 files changed

+21
-13
lines changed

2 files changed

+21
-13
lines changed

compiler-rt/lib/scudo/standalone/string_utils.cpp

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,11 @@ static int appendUnsigned(char **Buffer, const char *BufferEnd, u64 Num,
7878
static int appendSignedDecimal(char **Buffer, const char *BufferEnd, s64 Num,
7979
u8 MinNumberLength, bool PadWithZero) {
8080
const bool Negative = (Num < 0);
81-
return appendNumber(Buffer, BufferEnd,
82-
static_cast<u64>(Negative ? -Num : Num), 10,
83-
MinNumberLength, PadWithZero, Negative,
84-
/*Upper=*/false);
81+
const u64 UnsignedNum = (Num == INT64_MIN)
82+
? static_cast<u64>(INT64_MAX) + 1
83+
: static_cast<u64>(Negative ? -Num : Num);
84+
return appendNumber(Buffer, BufferEnd, UnsignedNum, 10, MinNumberLength,
85+
PadWithZero, Negative, /*Upper=*/false);
8586
}
8687

8788
// Use the fact that explicitly requesting 0 Width (%0s) results in UB and
@@ -158,16 +159,18 @@ int formatString(char *Buffer, uptr BufferLength, const char *Format,
158159
CHECK(!((Precision >= 0 || LeftJustified) && *Cur != 's'));
159160
switch (*Cur) {
160161
case 'd': {
161-
DVal = HaveLL ? va_arg(Args, s64)
162-
: HaveZ ? va_arg(Args, sptr) : va_arg(Args, int);
162+
DVal = HaveLL ? va_arg(Args, s64)
163+
: HaveZ ? va_arg(Args, sptr)
164+
: va_arg(Args, int);
163165
Res += appendSignedDecimal(&Buffer, BufferEnd, DVal, Width, PadWithZero);
164166
break;
165167
}
166168
case 'u':
167169
case 'x':
168170
case 'X': {
169-
UVal = HaveLL ? va_arg(Args, u64)
170-
: HaveZ ? va_arg(Args, uptr) : va_arg(Args, unsigned);
171+
UVal = HaveLL ? va_arg(Args, u64)
172+
: HaveZ ? va_arg(Args, uptr)
173+
: va_arg(Args, unsigned);
171174
const bool Upper = (*Cur == 'X');
172175
Res += appendUnsigned(&Buffer, BufferEnd, UVal, (*Cur == 'u') ? 10 : 16,
173176
Width, PadWithZero, Upper);

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,16 @@ template <class Config> static void testAllocator() {
8282
using AllocatorT = TestAllocator<Config>;
8383
auto Allocator = std::unique_ptr<AllocatorT>(new AllocatorT());
8484

85-
EXPECT_FALSE(Allocator->isOwned(&Mutex));
86-
EXPECT_FALSE(Allocator->isOwned(&Allocator));
87-
scudo::u64 StackVariable = 0x42424242U;
88-
EXPECT_FALSE(Allocator->isOwned(&StackVariable));
89-
EXPECT_EQ(StackVariable, 0x42424242U);
85+
static scudo::u8 StaticBuffer[scudo::Chunk::getHeaderSize() + 1];
86+
EXPECT_FALSE(
87+
Allocator->isOwned(&StaticBuffer[scudo::Chunk::getHeaderSize()]));
88+
89+
scudo::u8 StackBuffer[scudo::Chunk::getHeaderSize() + 1];
90+
for (scudo::uptr I = 0; I < sizeof(StackBuffer); I++)
91+
StackBuffer[I] = 0x42U;
92+
EXPECT_FALSE(Allocator->isOwned(&StackBuffer[scudo::Chunk::getHeaderSize()]));
93+
for (scudo::uptr I = 0; I < sizeof(StackBuffer); I++)
94+
EXPECT_EQ(StackBuffer[I], 0x42U);
9095

9196
constexpr scudo::uptr MinAlignLog = FIRST_32_SECOND_64(3U, 4U);
9297

0 commit comments

Comments
 (0)