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

[Chandler Carruth]

As long as 'alignAddr' can move a pointer more than one past the end of the
allocation, it doesn't really work IMO. But this is all such a
technicality, I'm not sure how much continued hacking is worth here.

You need to calculate both the alignment addition and the requested size,
entirely as an integer delta without moving the pointer in order to avoid
moving the pointer more thane one past the end I think...


On Wed, Sep 3, 2014 at 9:36 AM, Hans Wennborg <hans at chromium.org> wrote:

> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140903/d58931df/attachment.html>