Skip to content

UBSan: Fix unaligned load/stores found by Undefined Behaviour Sanitizer. #2522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2019
Merged

UBSan: Fix unaligned load/stores found by Undefined Behaviour Sanitizer. #2522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions CoreFoundation/Base.subproj/CFInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,5 +1044,79 @@ CF_PRIVATE const CFStringRef __kCFLocaleCollatorID;

CF_EXTERN_C_END


// Load 16,32,64 bit values from unaligned memory addresses. These need to be done bytewise otherwise
// it is undefined behaviour in C. On some architectures, eg x86, unaligned loads are allowed by the
// processor and the compiler will convert these byte accesses into the appropiate DWORD/QWORD memory
// access.

CF_INLINE uint32_t unaligned_load32(const void *ptr) {
uint8_t *bytes = (uint8_t *)ptr;
#if __LITTLE_ENDIAN__
uint32_t result = (uint32_t)bytes[0];
result |= ((uint32_t)bytes[1] << 8);
result |= ((uint32_t)bytes[2] << 16);
result |= ((uint32_t)bytes[3] << 24);
#else
uint32_t result = (uint32_t)bytes[0] << 24;
result |= ((uint32_t)bytes[1] << 16);
result |= ((uint32_t)bytes[2] << 8);
result |= (uint32_t)bytes[3];
#endif
return result;
}


CF_INLINE void unaligned_store32(void *ptr, uint32_t value) {
uint8_t *bytes = (uint8_t *)ptr;
#if __LITTLE_ENDIAN__
bytes[0] = (uint8_t)(value & 0xff);
bytes[1] = (uint8_t)((value >> 8) & 0xff);
bytes[2] = (uint8_t)((value >> 16) & 0xff);
bytes[3] = (uint8_t)((value >> 24) & 0xff);
#else
bytes[0] = (uint8_t)((value >> 24) & 0xff);
bytes[1] = (uint8_t)((value >> 16) & 0xff);
bytes[2] = (uint8_t)((value >> 8) & 0xff);
bytes[3] = (uint8_t)((value >> 0) & 0xff);
#endif
}


// Load values stored in Big Endian order in memory.
CF_INLINE uint16_t unaligned_load16be(const void *ptr) {
uint8_t *bytes = (uint8_t *)ptr;
uint16_t result = (uint16_t)bytes[0] << 8;
result |= (uint16_t)bytes[1];

return result;
}


CF_INLINE uint32_t unaligned_load32be(const void *ptr) {
uint8_t *bytes = (uint8_t *)ptr;
uint32_t result = (uint32_t)bytes[0] << 24;
result |= ((uint32_t)bytes[1] << 16);
result |= ((uint32_t)bytes[2] << 8);
result |= (uint32_t)bytes[3];

return result;
}


CF_INLINE uint64_t unaligned_load64be(const void *ptr) {
uint8_t *bytes = (uint8_t *)ptr;
uint64_t result = (uint64_t)bytes[0] << 56;
result |= ((uint64_t)bytes[1] << 48);
result |= ((uint64_t)bytes[2] << 40);
result |= ((uint64_t)bytes[3] << 32);
result |= ((uint64_t)bytes[4] << 24);
result |= ((uint64_t)bytes[5] << 16);
result |= ((uint64_t)bytes[6] << 8);
result |= (uint64_t)bytes[7];

return result;
}

#endif /* ! __COREFOUNDATION_CFINTERNAL__ */

23 changes: 10 additions & 13 deletions CoreFoundation/Parsing.subproj/CFBinaryPList.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ static void _flattenPlist(CFPropertyListRef plist, CFMutableArrayRef objlist, CF
CFDictionaryAddValue(objtable, plist, (const void *)(uintptr_t)refnum);
if (_kCFRuntimeIDCFDictionary == type) {
CFIndex count = CFDictionaryGetCount((CFDictionaryRef)plist);
STACK_BUFFER_DECL(CFPropertyListRef, buffer, count <= 128 ? count * 2 : 1);
STACK_BUFFER_DECL(CFPropertyListRef, buffer, (count > 0 && count <= 128) ? count * 2 : 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this same check need to be repeated on the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be needed because if count == 0 then the stack allocation is used instead of the malloc()'d buffer. 1 byte is wasted on the stack but this is also the case if count > 128 and the heap is used instead of the stack.

CFPropertyListRef *list = (count <= 128) ? buffer : (CFPropertyListRef *)CFAllocatorAllocate(kCFAllocatorSystemDefault, 2 * count * sizeof(CFTypeRef), 0);
CFDictionaryGetKeysAndValues((CFDictionaryRef)plist, list, list + count);
for (CFIndex idx = 0; idx < 2 * count; idx++) {
Expand All @@ -510,7 +510,7 @@ static void _flattenPlist(CFPropertyListRef plist, CFMutableArrayRef objlist, CF
if (list != buffer) CFAllocatorDeallocate(kCFAllocatorSystemDefault, list);
} else if (_kCFRuntimeIDCFArray == type) {
CFIndex count = CFArrayGetCount((CFArrayRef)plist);
STACK_BUFFER_DECL(CFPropertyListRef, buffer, count <= 256 ? count : 1);
STACK_BUFFER_DECL(CFPropertyListRef, buffer, (count > 0 && count <= 256) ? count : 1);
CFPropertyListRef *list = (count <= 256) ? buffer : (CFPropertyListRef *)CFAllocatorAllocate(kCFAllocatorSystemDefault, count * sizeof(CFTypeRef), 0);
CFArrayGetValues((CFArrayRef)plist, CFRangeMake(0, count), list);
for (CFIndex idx = 0; idx < count; idx++) {
Expand Down Expand Up @@ -713,22 +713,19 @@ CF_PRIVATE bool __CFBinaryPlistCreateObjectFiltered(const uint8_t *databytes, ui
/* Grab a valSize-bytes integer out of the buffer pointed at by data and return it.
*/
CF_INLINE uint64_t _getSizedInt(const uint8_t *data, uint8_t valSize) {
#if defined(__i386__) || defined(__x86_64__)

if (valSize == 1) {
return (uint64_t)*data;
} else if (valSize == 2) {
uint16_t val = *(uint16_t *)data;
return (uint64_t)CFSwapInt16BigToHost(val);
return (uint64_t)unaligned_load16be(data);
} else if (valSize == 4) {
uint32_t val = *(uint32_t *)data;
return (uint64_t)CFSwapInt32BigToHost(val);
return (uint64_t)unaligned_load32be(data);
} else if (valSize == 8) {
uint64_t val = *(uint64_t *)data;
return CFSwapInt64BigToHost(val);
return unaligned_load64be(data);
}
#endif
// Compatibility with existing archives, including anything with a non-power-of-2
// size and 16-byte values, and architectures that don't support unaligned access

// Compatibility with existing archives, including anything with a non-power-of-2
// size and 16-byte values
uint64_t res = 0;
for (CFIndex idx = 0; idx < valSize; idx++) {
res = (res << 8) + data[idx];
Expand Down Expand Up @@ -1429,7 +1426,7 @@ CF_PRIVATE bool __CFBinaryPlistCreateObjectFiltered(const uint8_t *databytes, ui
if (databytes + objectsRangeEnd < extent) FAIL_FALSE;
byte_cnt = check_size_t_mul(arrayCount, sizeof(CFPropertyListRef), &err);
if (CF_NO_ERROR != err) FAIL_FALSE;
STACK_BUFFER_DECL(CFPropertyListRef, buffer, arrayCount <= 256 ? arrayCount : 1);
STACK_BUFFER_DECL(CFPropertyListRef, buffer, (arrayCount > 0 && arrayCount <= 256) ? arrayCount : 1);
list = (arrayCount <= 256) ? buffer : (CFPropertyListRef *)CFAllocatorAllocate(kCFAllocatorSystemDefault, byte_cnt, 0);
if (!list) FAIL_FALSE;
Boolean madeSet = false;
Expand Down
12 changes: 12 additions & 0 deletions CoreFoundation/String.subproj/CFString.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,18 @@ CFStringEncoding __CFStringComputeEightBitStringEncoding(void) {
/* Returns whether the provided bytes can be stored in ASCII
*/
CF_INLINE Boolean __CFBytesInASCII(const uint8_t *bytes, CFIndex len) {
#if TARGET_RT_64_BIT
uint64_t align_mask = 7;
#else
uint32_t align_mask = 3;
#endif

/* Read bytes until the buffer is aligned. */
while (((uintptr_t)bytes & align_mask) && len > 0) {
if (*bytes++ & 0x80) return false;
len--;
}

#if TARGET_RT_64_BIT
/* A bit of unrolling; go by 32s, 16s, and 8s first */
while (len >= 32) {
Expand Down
35 changes: 22 additions & 13 deletions CoreFoundation/StringEncodings.subproj/CFUniChar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,8 +632,8 @@ CF_PRIVATE uint8_t CFUniCharGetBitmapForPlane(uint32_t charset, uint32_t plane,
numBytes /= 4; // for 32bit

while (numBytes-- > 0) {
*((uint32_t *)bitmap) = value;
#if defined (__cplusplus)
unaligned_store32(bitmap, value);
#if defined (__cplusplus)
bitmap = (uint8_t *)bitmap + sizeof(uint32_t);
#else
bitmap += sizeof(uint32_t);
Expand Down Expand Up @@ -744,7 +744,8 @@ CF_PRIVATE const void *CFUniCharGetMappingData(uint32_t type) {
headerSize = *((uint8_t *)bytes); bytes = (uint8_t *)bytes + sizeof(uint32_t);
#else
bytes += 4; // Skip Unicode version
headerSize = *((uint32_t *)bytes); bytes += sizeof(uint32_t);
headerSize = unaligned_load32(bytes);
bytes += sizeof(uint32_t);
#endif
headerSize -= (sizeof(uint32_t) * 2);
bodyBase = (char *)bytes + headerSize;
Expand All @@ -757,7 +758,8 @@ CF_PRIVATE const void *CFUniCharGetMappingData(uint32_t type) {
#if defined (__cplusplus)
__CFUniCharMappingTables[idx] = (char *)bodyBase + *((uint32_t *)bytes); bytes = (uint8_t *)bytes + sizeof(uint32_t);
#else
__CFUniCharMappingTables[idx] = (char *)bodyBase + *((uint32_t *)bytes); bytes += sizeof(uint32_t);
__CFUniCharMappingTables[idx] = (char *)bodyBase + unaligned_load32(bytes);
bytes += sizeof(uint32_t);
#endif
}
}
Expand All @@ -783,18 +785,24 @@ typedef struct {
static uint32_t __CFUniCharGetMappedCase(const __CFUniCharCaseMappings *theTable, uint32_t numElem, UTF32Char character) {
const __CFUniCharCaseMappings *p, *q, *divider;

if ((character < theTable[0]._key) || (character > theTable[numElem-1]._key)) {
#define READ_KEY(x) unaligned_load32(((uint8_t *)x) + offsetof(__CFUniCharCaseMappings, _key))
#define READ_VALUE(x) unaligned_load32(((uint8_t *)x) + offsetof(__CFUniCharCaseMappings, _value))

if ((character < READ_KEY(&theTable[0])) || (character > READ_KEY(&theTable[numElem-1]))) {
return 0;
}
p = theTable;
q = p + (numElem-1);
while (p <= q) {
divider = p + ((q - p) >> 1); /* divide by 2 */
if (character < divider->_key) { q = divider - 1; }
else if (character > divider->_key) { p = divider + 1; }
else { return divider->_value; }
if (character < READ_KEY(divider)) { q = divider - 1; }
else if (character > READ_KEY(divider)) { p = divider + 1; }
else { return READ_VALUE(divider); }
}
return 0;

#undef READ_KEY
#undef READ_VALUE
}

#define NUM_CASE_MAP_DATA (kCFUniCharCaseFold + 1)
Expand All @@ -818,9 +826,9 @@ static bool __CFUniCharLoadCaseMappingTable(void) {
__CFUniCharCaseMappingExtraTable = (const uint32_t **)__CFUniCharCaseMappingTable + NUM_CASE_MAP_DATA;

for (idx = 0;idx < NUM_CASE_MAP_DATA;idx++) {
countArray[idx] = *((uint32_t *)__CFUniCharMappingTables[idx]) / (sizeof(uint32_t) * 2);
countArray[idx] = unaligned_load32(__CFUniCharMappingTables[idx]) / (sizeof(uint32_t) * 2);
__CFUniCharCaseMappingTable[idx] = ((uint32_t *)__CFUniCharMappingTables[idx]) + 1;
__CFUniCharCaseMappingExtraTable[idx] = (const uint32_t *)((char *)__CFUniCharCaseMappingTable[idx] + *((uint32_t *)__CFUniCharMappingTables[idx]));
__CFUniCharCaseMappingExtraTable[idx] = (const uint32_t *)((char *)__CFUniCharCaseMappingTable[idx] + unaligned_load32(__CFUniCharMappingTables[idx]));
}

__CFUniCharCaseMappingTableCounts = countArray;
Expand Down Expand Up @@ -1034,7 +1042,7 @@ CFIndex CFUniCharMapCaseTo(UTF32Char theChar, UTF16Char *convertedChar, CFIndex
} else {
CFIndex idx;

for (idx = 0;idx < count;idx++) *(convertedChar++) = (UTF16Char)*(extraMapping++);
for (idx = 0;idx < count;idx++) *(convertedChar++) = (UTF16Char)unaligned_load32(extraMapping++);
return count;
}
}
Expand Down Expand Up @@ -1241,7 +1249,8 @@ const void *CFUniCharGetUnicodePropertyDataForPlane(uint32_t propertyType, uint3
headerSize = CFSwapInt32BigToHost(*((uint32_t *)bytes)); bytes = (uint8_t *)bytes + sizeof(uint32_t);
#else
bytes += 4; // Skip Unicode version
headerSize = CFSwapInt32BigToHost(*((uint32_t *)bytes)); bytes += sizeof(uint32_t);
headerSize = unaligned_load32be(bytes);
bytes += sizeof(uint32_t);
#endif

headerSize -= (sizeof(uint32_t) * 2);
Expand Down Expand Up @@ -1275,7 +1284,7 @@ const void *CFUniCharGetUnicodePropertyDataForPlane(uint32_t propertyType, uint3
bodyBase = (const uint8_t *)bodyBase + (CFSwapInt32BigToHost(*(uint32_t *)bytes));
((uint32_t *&)bytes) ++;
#else
bodyBase += (CFSwapInt32BigToHost(*((uint32_t *)bytes++)));
bodyBase += unaligned_load32be(bytes++);
#endif
}

Expand Down
18 changes: 12 additions & 6 deletions CoreFoundation/StringEncodings.subproj/CFUnicodeDecomposition.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static void __CFUniCharLoadDecompositionTable(void) {
return;
}

__CFUniCharDecompositionTableLength = *(bytes++);
__CFUniCharDecompositionTableLength = unaligned_load32(bytes++);
__CFUniCharDecompositionTable = (UTF32Char *)bytes;
__CFUniCharMultipleDecompositionTable = (UTF32Char *)((intptr_t)bytes + __CFUniCharDecompositionTableLength);

Expand Down Expand Up @@ -102,18 +102,24 @@ typedef struct {
static uint32_t __CFUniCharGetMappedValue(const __CFUniCharDecomposeMappings *theTable, uint32_t numElem, UTF32Char character) {
const __CFUniCharDecomposeMappings *p, *q, *divider;

if ((character < theTable[0]._key) || (character > theTable[numElem-1]._key)) {
#define READ_KEY(x) unaligned_load32(((uint8_t *)x) + offsetof(__CFUniCharDecomposeMappings, _key))
#define READ_VALUE(x) unaligned_load32(((uint8_t *)x) + offsetof(__CFUniCharDecomposeMappings, _value))

if ((character < READ_KEY(&theTable[0])) || (character > READ_KEY(&theTable[numElem-1]))) {
return 0;
}
p = theTable;
q = p + (numElem-1);
while (p <= q) {
divider = p + ((q - p) >> 1); /* divide by 2 */
if (character < divider->_key) { q = divider - 1; }
else if (character > divider->_key) { p = divider + 1; }
else { return divider->_value; }
if (character < READ_KEY(divider)) { q = divider - 1; }
else if (character > READ_KEY(divider)) { p = divider + 1; }
else { return READ_VALUE(divider); }
}
return 0;

#undef READ_KEY
#undef READ_VALUE
}

static void __CFUniCharPrioritySort(UTF32Char *characters, CFIndex length) {
Expand Down Expand Up @@ -162,7 +168,7 @@ static CFIndex __CFUniCharRecursivelyDecomposeCharacter(UTF32Char character, UTF

usedLength += length;

while (length--) *(convertedChars++) = *(mappings++);
while (length--) *(convertedChars++) = unaligned_load32(mappings++);

return usedLength;
}
Expand Down