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

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Sep 3 07:20:44 PDT 2014

> 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.
    // allocate in new slab.

More information about the llvm-commits mailing list