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

[Pete Cooper via llvm-dev]

> On Mar 28, 2016, at 3:12 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
> FWIW, I agree with Mehdi that we should just assert that our types don't get called with size zero.
Yeah, I agree.  I’ve tried running the tests with the assert in place and there’s about 1000 failures across llvm/clang.  I’ll see what I can fix as I would like to get these to behave.  There may be actual logic errors in some of these cases.
> 
> 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.
I agree.  I don’t actually know if NOALIAS and NONNULL were put on this function independently, or if the committer assumed one implied the other.  But I do agree that they should be independent in general, even if they happen to both apply here.

Cheers,
Pete
> 
> -Chandler
> 
> On Tue, Mar 22, 2016 at 9:18 PM Mehdi Amini via llvm-dev <llvm-dev at lists.llvm.org <mailto: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 <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>>> 
>>> On Mar 22, 2016, at 5:35 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>> 
>>> 
>>> 
>>> On Tue, Mar 22, 2016 at 5:30 PM, Pete Cooper <peter_cooper at apple.com <mailto: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 <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <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/ae22a95c/attachment.html>