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

[Hans Wennborg]

On Wed, Sep 3, 2014 at 7:20 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2014 Sep 2, at 21:34, Chandler Carruth <chandlerc at google.com> wrote:
>>
>>> 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.
>
> I think the attached/committed patch is an incremental improvement.
> The pointer isn't walking anywhere; all addition and comparison is
> done as `uintptr_t`.  So it removed the "common case" reliance on UB.
>
>> 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.
>
> Overflowing `uintptr_t` is the only UB left.  Is this a practical
> concern?  I assumed not, but maybe that was naive?
>
>> 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.
>
> It should be sufficient to check that the final `uintptr_t` is `>=`
> the unmodified `Ptr` before converting back to `char*`.
>
> Reformulating might be better anyway, though.  I wouldn't mind
> something like this:
>
>     uintptr_t Adjustment = getAlignmentAdjustment(Ptr, Alignment);
>     assert(Adjustment + Size >= Size && "overflow");
>     if (Adjustment + Size <= uintptr_t(End - Ptr)) {
>       Ptr += Adjustment;
>
>       // allocate in current slab.
>       return;
>     }
>     // allocate in new slab.

I like this idea. How about the attached patch?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: allocator.patch
Type: application/octet-stream
Size: 3156 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140903/f16ab3e5/attachment.obj>