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

[Hans Wennborg]

On Tue, Sep 2, 2014 at 12:38 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2014 Sep 2, at 13:44, Hans Wennborg <hans at chromium.org> wrote:
>>
>> On Mon, Sep 1, 2014 at 2:04 PM, Duncan P. N. Exon Smith
>> <dexonsmith at apple.com> wrote:
>>>
>>>> On 2014 Sep 1, at 00:29, Hans Wennborg <hans at hanshq.net> wrote:
>>>>
>>>> On 08/31/2014 08:36 PM, Chandler Carruth wrote:
>>>>>
>>>>> On Sun, Aug 31, 2014 at 8:14 PM, Duncan P. N. Exon Smith
>>>>> <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>>>>
>>>>>> On 2014 Aug 17, at 14:31, Hans Wennborg <hans at hanshq.net
>>>>>   <mailto:hans at hanshq.net>> wrote:
>>>>>>
>>>>>> Author: hans
>>>>>> Date: Sun Aug 17 13:31:18 2014
>>>>>> New Revision: 215841
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=215841&view=rev
>>>>>> Log:
>>>>>> BumpPtrAllocator: remove 'no slabs allocated yet' check
>>>>>>
>>>>>> We already handle the no-slabs case when checking whether the
>>>>>   current slab
>>>>>> is large enough: if no slabs have been allocated, CurPtr and End
>>>>>   are both 0.
>>>>>> alignPtr(0), will still be 0, and so "if (Ptr + Size <= End)" fails.
>>>>>
>>>>>   I don't think this commit introduces the problem, but isn't the result
>>>>>   of `Ptr + Size` undefined when `Size` is too big?
>>>>>
>>>>>   Should this be changed to `if (Size <= End - Ptr)`?
>>>>
>>>> Ptr, which we got from calling alignPtr() can point beyond the current slab, so the End - Ptr operation could be undefined.
>>>
>>> I assumed that slab sizes are always powers of 2 (so End will always be
>>> well-aligned for reasonable values of `Alignment`), but I don't see any
>>> asserts about that in the code now that I've looked.  If we can add a
>>> (static?) assertion to that effect then `alignPtr()` should be safe.
>>>
>>> Or is there a use case for unaligned slab end pointers?
>>
>> I don't think it's a problem in practice, but if Alignment is greater
>> than the size (not likely), or base alignment of the slab (which we
>> get from malloc), then we could align past End.
>>
>>>>> Should check the pre-commit review here, I think I brought this up and
>>>>> there was some reason why it didn't make sense to fix it here....
>>>>
>>>> I think the reasoning was that it wouldn't cause problems in practice. I'd be happy to fix it, but I think that would mean doing the alignment and bounds check with intptr_t.
>>>
>>> I think this would be good to fix.  Never know when a compiler might
>>> take advantage of UB.
>>
>> I'm attaching a patch that should make this UB-proof. Let me know what
>> you think.
>
> LGTM, thanks!

Committed r216973.

 - Hans