Pete Cooper via llvm-dev
2016-Mar-23 00:30 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
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 Thanks, Pete -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/619fe5dc/attachment.html>
David Blaikie via llvm-dev
2016-Mar-23 00:35 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
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. Can check for wording if that's helpful/desired.> > Thanks, > Pete > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/094dcf31/attachment.html>
Pete Cooper via llvm-dev
2016-Mar-23 00:39 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
> 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 <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.> 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-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/9da90609/attachment.html>
On Tue, Mar 22, 2016 at 5:30 PM, Pete Cooper via llvm-dev <llvm-dev at lists.llvm.org> 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.Why is noalias not valid here? I thought LLVM's notion of noalias was based on data dependence -- ptr_a does not alias ptr_b if a store to ptr_a does not have a data dependence with a load from ptr_b and vice versa (and for a zero-sized allocation there can be no data dependence since there cannot legally be such a load/store). Is the C++ notion of noalias different?> > 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 1A fourth option would be return a non-null pointer (maybe the address of some static char) for allocations of size 0; unless I'm mistaken about the noalias bit above. -- Sanjoy
Pete Cooper via llvm-dev
2016-Mar-23 01:00 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
> On Mar 22, 2016, at 5:57 PM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote: > > On Tue, Mar 22, 2016 at 5:30 PM, Pete Cooper via llvm-dev > <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> 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. > > Why is noalias not valid here? > > I thought LLVM's notion of noalias was based on data dependence -- > ptr_a does not alias ptr_b if a store to ptr_a does not have a data > dependence with a load from ptr_b and vice versa (and for a zero-sized > allocation there can be no data dependence since there cannot legally > be such a load/store). Is the C++ notion of noalias different?That was an assumption on my part that “allocate(0) != allocate(0)’. That is, I didn’t think you needed to dereference a pointer for no alias, just compare it. I could be wrong as I’m just assuming thats the behavior without any formal knowledge on the subject.> >> >> 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 > > A fourth option would be return a non-null pointer (maybe the address > of some static char) for allocations of size 0; unless I'm mistaken > about the noalias bit above.Interesting. I didn’t think of that, but there’s no reason that wouldn’t work, assuming the noalias is ok as you say. Cheers, Pete> > -- Sanjoy-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/9b8df212/attachment.html>