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
I think David is referring this thread: http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/074909.html I also tried building with the suggested patch (replacing/modifying the existing ctor (to take non-const ref) ) and this indeed results in plenty of build errors. I tried fixing them for a few hours but haven't seen light at the end of the tunnel. Best regards, Andy 2014-08-21 9:38 GMT+02:00 Justin Bogner <mail at justinbogner.com>:> 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 > > _______________________________________________ > 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/20140821/5f56cec7/attachment.html>
Yeah - I suspect there are just too many cases where we use this ctor correctly: where the ArrayRef is only a temporary to a function call. Perhaps this is a problem for which the best solution is a clang-tidy checker - though I'm not sure if we have good integration there yet. (& yes, Andreas, that looks like the previous thread - thanks!) On Thu, Aug 21, 2014 at 5:09 AM, Andreas Weber <andreas.c.weber at gmail.com> wrote:> I think David is referring this thread: > http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-July/074909.html > > I also tried building with the suggested patch (replacing/modifying the > existing ctor (to take non-const ref) ) and this indeed results in plenty of > build errors. I tried fixing them for a few hours but haven't seen light at > the end of the tunnel. > > Best regards, > Andy > > > 2014-08-21 9:38 GMT+02:00 Justin Bogner <mail at justinbogner.com>: > >> 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 >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >