Analyzing why GCC failed to build LLVM recently, one root cause lies
in definition of ArrayRef:
// ArrayRef.h:
ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {}
Here address of const reference is taken and stored to an object. It
is believed that live range of const reference is only at the function
call site, escaping of its address to an object with a longer live
range is invalid. Referring to the case and discussion here:
https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html
So I would suggest to fix ArrayRef. Adding a non-const version of
constructor should work, but it still leaves the vulnerability in
const version, which I'd expect on people in the community to work out
a solution.
ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {}
Thanks,
Joey
I seem to recall discussing this before - is there an existing llvm bug filed, another email thread or something (or perhaps it was just an IRC conversation)? It would be good to keep all the discussion together or at least reference the prior (llvm community) discussion. Have you tried applying your suggested patch? I assume you meant to suggest replacing/modifying the existing ctor (to take non-const ref) rather than adding another? I'd assume that causes a lot of build failures as we probably rely on binding temporaries to ArrayRef's in many places correctly (most ArrayRef's are temporaries, not local variables). I think in the previous discussion I suggested we might just want to make a practice of treating named/local (not parameter) ArrayRef's with greater suspicion, the same way we do for twine, but I'm not sure. We could move this ctor into a factory function (and/or just make the CTor private and friend the makeArrayRef helper for this case) to make it more obvious/easier to find bad call sites. But it would be helpful to have the context of the prior discussion to continue from there. On Aug 19, 2014 11:18 PM, "Joey Ye" <joey.ye.cc at gmail.com> wrote:> Analyzing why GCC failed to build LLVM recently, one root cause lies > in definition of ArrayRef: > // ArrayRef.h: > ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {} > > Here address of const reference is taken and stored to an object. It > is believed that live range of const reference is only at the function > call site, escaping of its address to an object with a longer live > range is invalid. Referring to the case and discussion here: > https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html > > So I would suggest to fix ArrayRef. Adding a non-const version of > constructor should work, but it still leaves the vulnerability in > const version, which I'd expect on people in the community to work out > a solution. > ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {} > > > Thanks, > Joey > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140820/55b6ad6e/attachment.html>
Just applied and it didn't work. I'm fresh to LLVM so not sure if it is built correct. Anyway, if the issue is known I'll leave people from this community to solve it. Thanks, Joey On Wed, Aug 20, 2014 at 11:28 PM, David Blaikie <dblaikie at gmail.com> wrote:> I seem to recall discussing this before - is there an existing llvm bug > filed, another email thread or something (or perhaps it was just an IRC > conversation)? It would be good to keep all the discussion together or at > least reference the prior (llvm community) discussion. > > Have you tried applying your suggested patch? I assume you meant to suggest > replacing/modifying the existing ctor (to take non-const ref) rather than > adding another? > > I'd assume that causes a lot of build failures as we probably rely on > binding temporaries to ArrayRef's in many places correctly (most ArrayRef's > are temporaries, not local variables). > > I think in the previous discussion I suggested we might just want to make a > practice of treating named/local (not parameter) ArrayRef's with greater > suspicion, the same way we do for twine, but I'm not sure. > > We could move this ctor into a factory function (and/or just make the CTor > private and friend the makeArrayRef helper for this case) to make it more > obvious/easier to find bad call sites. But it would be helpful to have the > context of the prior discussion to continue from there. > > On Aug 19, 2014 11:18 PM, "Joey Ye" <joey.ye.cc at gmail.com> wrote: >> >> Analyzing why GCC failed to build LLVM recently, one root cause lies >> in definition of ArrayRef: >> // ArrayRef.h: >> ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {} >> >> Here address of const reference is taken and stored to an object. It >> is believed that live range of const reference is only at the function >> call site, escaping of its address to an object with a longer live >> range is invalid. Referring to the case and discussion here: >> https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html >> >> So I would suggest to fix ArrayRef. Adding a non-const version of >> constructor should work, but it still leaves the vulnerability in >> const version, which I'd expect on people in the community to work out >> a solution. >> ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {} >> >> >> Thanks, >> Joey >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
David Blaikie <dblaikie at gmail.com> writes:> I seem to recall discussing this before - is there an existing llvm > bug filed, another email thread or something (or perhaps it was just > an IRC conversation)? It would be good to keep all the discussion > together or at least reference the prior (llvm community) discussion.I'm not sure if it's been discussed before, but it's led to issues as recently as a couple of weeks ago: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140804/229557.html I certainly think it's worthwhile to make this API safer, or at least to document the caveats in how it can safely be used.> Have you tried applying your suggested patch? I assume you meant to suggest > replacing/modifying the existing ctor (to take non-const ref) rather than > adding another? > > I'd assume that causes a lot of build failures as we probably rely on binding > temporaries to ArrayRef's in many places correctly (most ArrayRef's are > temporaries, not local variables). > > I think in the previous discussion I suggested we might just want to make a > practice of treating named/local (not parameter) ArrayRef's with greater > suspicion, the same way we do for twine, but I'm not sure. > > We could move this ctor into a factory function (and/or just make the CTor > private and friend the makeArrayRef helper for this case) to make it more > obvious/easier to find bad call sites. But it would be helpful to have the > context of the prior discussion to continue from there. > > On Aug 19, 2014 11:18 PM, "Joey Ye" <joey.ye.cc at gmail.com> wrote: > > Analyzing why GCC failed to build LLVM recently, one root cause lies > in definition of ArrayRef: > // ArrayRef.h: > ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {} > > Here address of const reference is taken and stored to an object. It > is believed that live range of const reference is only at the function > call site, escaping of its address to an object with a longer live > range is invalid. Referring to the case and discussion here: > https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html > > So I would suggest to fix ArrayRef. Adding a non-const version of > constructor should work, but it still leaves the vulnerability in > const version, which I'd expect on people in the community to work out > a solution. > ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {} > > Thanks, > Joey > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvmdev