[llvm-dev] UBSan, StringRef and Allocator.h

[Chandler Carruth via llvm-dev]

FWIW, I agree with Mehdi that we should just assert that our types don't
get called with size zero.

That said, I don't think we can be terribly cavalier with what we expect
from standard allocator types, operator new, or malloc. And so I would
expect LLVM_ATTRIBUTE_RETURNS_NOALIAS to not imply NONNULL, and while it
seems reasonable to put NONNULL on *our* allocate function because of the
assert and the fact that we assume the underlying allocation routine never
produces a null, it doesn't seem reasonable for any old function with
NOALIAS to have NONNULL inferred.

-Chandler

On Tue, Mar 22, 2016 at 9:18 PM Mehdi Amini via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> On Mar 22, 2016, at 5:39 PM, Pete Cooper via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
> On Mar 22, 2016, at 5:35 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Tue, Mar 22, 2016 at 5:30 PM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>> Hi all
>>
>> (No idea if I have the correct audience.  Please CC more people as
>> needed).
>>
>> I have an UBSan failure in BumpPtrAllocatorImpl.Allocate.
>>
>> The problem is that lld requests that we StringRef::copy an empty
>> string.  This passes a length of 0 to a BumpPtrAllocator.  The
>> BumpPtrAllocator happened to not have anything allocated yet so the CurPtr
>> is nullptr, but given that we need 0 space we think we have enough space
>> and return an allocation of size 0 at address nullptr.  This therefore
>> returns nullptr from Allocate, but that method is marked
>> with LLVM_ATTRIBUTE_RETURNS_NONNULL and LLVM_ATTRIBUTE_RETURNS_NOALIAS,
>> both of which aren’t true in this case.
>>
>> To put this in code, if I have
>>
>> BumpPtrAllocator allocator;
>> StringRef s;
>> s.copy(allocator);
>>
>>
>> then i’m going to allocate 0 bytes in the allocator and get a
>> StringRef(nullptr, 0).  Its a valid StringRef, but an UBSan failures in the
>> allocator.
>>
>> Lang and I looked up malloc behaviour online as this is fairly
>> analogous.  The answer there is that you are allowed to return nullptr, or
>> not, its implementation defined.  So no help there.
>>
>> So the question is, how do we want this to behave in our code?
>>
>> Some options:
>> - Assert that Allocate never gets a size 0 allocation.  So fix
>> StringRef::copy to see this case
>> - Remove the attributes from Allocate but continue to return nullptr (or
>> base of the allocator?) in this case
>> - Keep the attributes on Allocate and treat size 0 allocations as size 1
>>
>
> I believe the last is closer to 'new's behavior - which I think returns a
> unique non-null address (well, unique amongst current allocations - can be
> recycled once deleted) if I recall correctly.
>
> That’s what I would have expected too.  Its like sizeof(struct {}) which
> can be a 1 depending on factors we don’t need to get in to here.
>
>
> Well except that if sizeof(struct{}) is 1, the allocator is never called
> with a 0.
>
> I would consider forbidding zero sized allocation in the allocator
> (assert()) by design (hey we're controlling every possible uses!), unless
> there is a real use-case for that.
>
> This would also be in line with the C++ standard requirement for allocator
> which specifies that the result of "a.allocate(0)" is unspecified (ref:
> C++14 Table 28 — Allocator requirements).
>
> --
> Mehdi
>
>
> Can check for wording if that's helpful/desired.
>
> No need for my benefit :)  I’m in agreement that this is a good behavior
> to go for, but will leave it to others to say if they’d like the extra
> detail.
>
> One thing I did forget to say is that I’d like to fix StringRef::copy in
> all of the above cases.  I think that this method should always avoid the
> allocator and return StringRef(nullptr, 0) when length is 0.  I’ll get a
> patch up on llvm-commits if there’s no objections there.
>
> Thanks,
> Pete
>
>
>
>>
>> Thanks,
>> Pete
>>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160328/6f0f38aa/attachment.html>