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>
David Blaikie via llvm-dev
2016-Mar-23 00:42 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
On Tue, Mar 22, 2016 at 5:39 PM, Pete Cooper <peter_cooper at apple.com> 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. > > 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. >Sounds plausible to me> > Thanks, > Pete > > > >> >> Thanks, >> Pete >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/2336ee18/attachment-0001.html>
Mehdi Amini via llvm-dev
2016-Mar-23 04:18 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
> 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 <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>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160322/a58f6c13/attachment.html>
Chandler Carruth via llvm-dev
2016-Mar-28 22:12 UTC
[llvm-dev] UBSan, StringRef and Allocator.h
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>