<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 2, 2014 at 2:51 PM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@hanshq.net" target="_blank">hans@hanshq.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: hans<br>
Date: Tue Sep 2 16:51:35 2014<br>
New Revision: 216973<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=216973&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=216973&view=rev</a><br>
Log:<br>
BumpPtrAllocator: use uintptr_t when aligning addresses to avoid undefined behaviour<br>
<br>
In theory, alignPtr() could push a pointer beyond the end of the current slab, making<br>
comparisons with that pointer undefined behaviour. Use an integer type to avoid this.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/Allocator.h<br>
llvm/trunk/include/llvm/Support/MathExtras.h<br>
llvm/trunk/unittests/Support/AllocatorTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/Allocator.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Allocator.h?rev=216973&r1=216972&r2=216973&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Allocator.h?rev=216973&r1=216972&r2=216973&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/Allocator.h (original)<br>
+++ llvm/trunk/include/llvm/Support/Allocator.h Tue Sep 2 16:51:35 2014<br>
@@ -210,16 +210,17 @@ public:<br>
BytesAllocated += Size;<br>
<br>
// Allocate the aligned space, going forwards from CurPtr.<br>
- char *Ptr = alignPtr(CurPtr, Alignment);<br>
+ uintptr_t AlignedAddr = alignAddr(CurPtr, Alignment);<br>
<br>
// Check if we can hold it.<br>
- if (Ptr + Size <= End) {<br>
- CurPtr = Ptr + Size;<br>
+ if (AlignedAddr + Size <= (uintptr_t)End) {<br></blockquote><div><br></div><div>I would somehow assert that the alignment doesn't wrap End such that it is smaller than CurPtr.</div><div><br></div><div>I would also change this to be:</div>
<div>if ((uintptr_t)End - AlignedAddr >= Size)</div><div><br></div><div>Consider AlignedAddr + Size s.t. it wraps *past* End and results in being really small.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ char *AlignedPtr = (char*)AlignedAddr;<br>
+ CurPtr = AlignedPtr + Size;<br>
// Update the allocation point of this memory block in MemorySanitizer.<br>
// Without this, MemorySanitizer messages for values originated from here<br>
// will point to the allocation of the entire slab.<br>
- __msan_allocated_memory(Ptr, Size);<br>
- return Ptr;<br>
+ __msan_allocated_memory(AlignedPtr, Size);<br>
+ return AlignedPtr;<br>
}<br>
<br>
// If Size is really big, allocate a separate slab for it.<br>
@@ -228,19 +229,22 @@ public:<br>
void *NewSlab = Allocator.Allocate(PaddedSize, 0);<br>
CustomSizedSlabs.push_back(std::make_pair(NewSlab, PaddedSize));<br>
<br>
- Ptr = alignPtr((char *)NewSlab, Alignment);<br>
- assert((uintptr_t)Ptr + Size <= (uintptr_t)NewSlab + PaddedSize);<br>
- __msan_allocated_memory(Ptr, Size);<br>
- return Ptr;<br>
+ uintptr_t AlignedAddr = alignAddr(NewSlab, Alignment);<br>
+ assert(AlignedAddr + Size <= (uintptr_t)NewSlab + PaddedSize);<br>
+ char *AlignedPtr = (char*)AlignedAddr;<br>
+ __msan_allocated_memory(AlignedPtr, Size);<br>
+ return AlignedPtr;<br>
}<br>
<br>
// Otherwise, start a new slab and try again.<br>
StartNewSlab();<br>
- Ptr = alignPtr(CurPtr, Alignment);<br>
- CurPtr = Ptr + Size;<br>
- assert(CurPtr <= End && "Unable to allocate memory!");<br>
- __msan_allocated_memory(Ptr, Size);<br>
- return Ptr;<br>
+ AlignedAddr = alignAddr(CurPtr, Alignment);<br>
+ assert(AlignedAddr + Size <= (uintptr_t)End &&<br>
+ "Unable to allocate memory!");<br>
+ char *AlignedPtr = (char*)AlignedAddr;<br>
+ CurPtr = AlignedPtr + Size;<br>
+ __msan_allocated_memory(AlignedPtr, Size);<br>
+ return AlignedPtr;<br>
}<br>
<br>
// Pull in base class overloads.<br>
@@ -373,7 +377,7 @@ public:<br>
/// all memory allocated so far.<br>
void DestroyAll() {<br>
auto DestroyElements = [](char *Begin, char *End) {<br>
- assert(Begin == alignPtr(Begin, alignOf<T>()));<br>
+ assert(Begin == (char*)alignAddr(Begin, alignOf<T>()));<br>
for (char *Ptr = Begin; Ptr + sizeof(T) <= End; Ptr += sizeof(T))<br>
reinterpret_cast<T *>(Ptr)->~T();<br>
};<br>
@@ -382,7 +386,7 @@ public:<br>
++I) {<br>
size_t AllocatedSlabSize = BumpPtrAllocator::computeSlabSize(<br>
std::distance(Allocator.Slabs.begin(), I));<br>
- char *Begin = alignPtr((char *)*I, alignOf<T>());<br>
+ char *Begin = (char*)alignAddr(*I, alignOf<T>());<br>
char *End = *I == Allocator.Slabs.back() ? Allocator.CurPtr<br>
: (char *)*I + AllocatedSlabSize;<br>
<br>
@@ -392,7 +396,7 @@ public:<br>
for (auto &PtrAndSize : Allocator.CustomSizedSlabs) {<br>
void *Ptr = PtrAndSize.first;<br>
size_t Size = PtrAndSize.second;<br>
- DestroyElements(alignPtr((char *)Ptr, alignOf<T>()), (char *)Ptr + Size);<br>
+ DestroyElements((char*)alignAddr(Ptr, alignOf<T>()), (char *)Ptr + Size);<br>
}<br>
<br>
Allocator.Reset();<br>
<br>
Modified: llvm/trunk/include/llvm/Support/MathExtras.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=216973&r1=216972&r2=216973&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/MathExtras.h?rev=216973&r1=216972&r2=216973&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/MathExtras.h (original)<br>
+++ llvm/trunk/include/llvm/Support/MathExtras.h Tue Sep 2 16:51:35 2014<br>
@@ -550,16 +550,15 @@ inline uint64_t MinAlign(uint64_t A, uin<br>
return (A | B) & (1 + ~(A | B));<br>
}<br>
<br>
-/// \brief Aligns \c Ptr to \c Alignment bytes, rounding up.<br>
+/// \brief Aligns \c Addr to \c Alignment bytes, rounding up.<br>
///<br>
/// Alignment should be a power of two. This method rounds up, so<br>
-/// AlignPtr(7, 4) == 8 and AlignPtr(8, 4) == 8.<br>
-inline char *alignPtr(char *Ptr, size_t Alignment) {<br>
+/// alignAddr(7, 4) == 8 and alignAddr(8, 4) == 8.<br>
+inline uintptr_t alignAddr(void *Addr, size_t Alignment) {<br>
assert(Alignment && isPowerOf2_64((uint64_t)Alignment) &&<br>
"Alignment is not a power of two!");<br>
<br>
- return (char *)(((uintptr_t)Ptr + Alignment - 1) &<br>
- ~(uintptr_t)(Alignment - 1));<br>
+ return (((uintptr_t)Addr + Alignment - 1) & ~(uintptr_t)(Alignment - 1));<br>
}<br>
<br>
/// NextPowerOf2 - Returns the next power of two (in 64-bits)<br>
<br>
Modified: llvm/trunk/unittests/Support/AllocatorTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/AllocatorTest.cpp?rev=216973&r1=216972&r2=216973&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/AllocatorTest.cpp?rev=216973&r1=216972&r2=216973&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/AllocatorTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/AllocatorTest.cpp Tue Sep 2 16:51:35 2014<br>
@@ -130,7 +130,7 @@ public:<br>
void *MemBase = malloc(Size + Alignment - 1 + sizeof(void*));<br>
<br>
// Find the slab start.<br>
- void *Slab = alignPtr((char *)MemBase + sizeof(void *), Alignment);<br>
+ void *Slab = (void *)alignAddr((char*)MemBase + sizeof(void *), Alignment);<br>
<br>
// Hold a pointer to the base so we can free the whole malloced block.<br>
((void**)Slab)[-1] = MemBase;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>