[Patch] Use uintptr_t instead of ptr comparisons in BumpPtrAllocator (Was: Re: [llvm] r215841 - BumpPtrAllocator: remove 'no slabs allocated yet' check)

[Chandler Carruth]

On Tue, Sep 2, 2014 at 10:44 AM, Hans Wennborg <hans at chromium.org> wrote:

> I'm attaching a patch that should make this UB-proof. Let me know what
> you think.
>

The reason I didn't suggest this originally is that it doesn't actually
make anything UB-proof. You're still potentially walking a pointer far past
the end of the allocated array and comparing it with a pointer one past the
end of the array. You need to check every single time prior to adding that
there is space within the allocated array.

As an example of how I think this could fail, imagine that your system maps
pointers to uintptr_t such that 'End' becomes
std::numeric_limits<uintptr_t>::max() and either the aligning or the size
addition wrap.

If you want to go this route, at least use intptr_t so we get a UBSan
failure if this state ever arises. Better would be to just reformulate the
entire thing in terms that are guaranteed to only use pointers within the
allocated array.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/7ab5d138/attachment.html>