Nathan James via llvm-dev
2020-Sep-07 14:50 UTC
[llvm-dev] [ADT] Adding instrumentation for ASAN to SmallVector
Dear list, I recently tried to add instrumentation to SmallVector for using Address sanitizer to detect cases where references used after they are invalidated. This basic implementation for this is here - https://reviews.llvm.org/D87237 However, in adding/testing this, I did uncover some questionable code. Firstly `SmallString<unsigned>::c_str()` and `Twine::toNullTerminatedStringRef(SmallVectorImpl<char>&)` both use bytes outside the range of the SmallVectors storage. This isn't inherently bad. Secondly calling `SmallVectorImpl<T>::insert(iterator, const T&)` results in a reference invalidation when the element to insert is contained inside the SmallVector and the SmallVector needs to grow for the insert. This has been fixed inside the aforementioned PR. My main point here is how does everyone feel about using ASAN to catch bugs like this not only inside SmallVector but also adding the instrumentation to some other containers used by llvm. If people are happy with this implementation for SmallVector I'd be happy for feedback on the PR. It would likely need some specific asan test cases however I'm not entirely sure how to go about adding those. Thanks for reading, ~Nathan
Mitch Phillips via llvm-dev
2020-Sep-08 17:39 UTC
[llvm-dev] [ADT] Adding instrumentation for ASAN to SmallVector
It sounds like a very reasonable strategy! Have you seen __sanitizer_annotate_contiguous_container (compiler-rt/include/sanitizer/common_interface_defs.h)? In case you missed it - we have container-overflow built into ASan and implemented in std::vector in libcxx, and this is the canonical way of annotating containers like this. Have you tried building LLVM/Clang with an ASan-ified LLVM/Clang? Curious to see whether there's any latent bugs around. On Mon, Sep 7, 2020 at 7:50 AM Nathan James via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Dear list, > > I recently tried to add instrumentation to SmallVector for using > Address sanitizer to detect cases where references used after they are > invalidated. This basic implementation for this is here - > https://reviews.llvm.org/D87237 > > However, in adding/testing this, I did uncover some questionable code. > Firstly `SmallString<unsigned>::c_str()` and > `Twine::toNullTerminatedStringRef(SmallVectorImpl<char>&)` both use > bytes outside the range of the SmallVectors storage. This isn't > inherently bad. > Secondly calling `SmallVectorImpl<T>::insert(iterator, const T&)` > results in a reference invalidation when the element to insert is > contained inside the SmallVector and the SmallVector needs to grow for > the insert. This has been fixed inside the aforementioned PR. > > My main point here is how does everyone feel about using ASAN to catch > bugs like this not only inside SmallVector but also adding the > instrumentation to some other containers used by llvm. If people are > happy with this implementation for SmallVector I'd be happy for > feedback on the PR. It would likely need some specific asan test cases > however I'm not entirely sure how to go about adding those. > > Thanks for reading, > > ~Nathan > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://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/20200908/3cace368/attachment.html>
Evgenii Stepanov via llvm-dev
2020-Sep-08 17:49 UTC
[llvm-dev] [ADT] Adding instrumentation for ASAN to SmallVector
My main concern with container poisoning is that it is all-or-nothing - you need to build all translation units that include the container definition with ASan, or risk false positive due to a missing shadow update. This is not a property of ASan in general, and can result in confusing error reports. I'm fine with it as long as it can be easily disabled, see below. On Tue, Sep 8, 2020 at 10:39 AM Mitch Phillips via llvm-dev < llvm-dev at lists.llvm.org> wrote:> It sounds like a very reasonable strategy! > > Have you seen __sanitizer_annotate_contiguous_container > (compiler-rt/include/sanitizer/common_interface_defs.h)? In case you missed > it - we have container-overflow built into ASan and implemented in > std::vector in libcxx, and this is the canonical way of annotating > containers like this. >Yes, this will produce better error reports, and it can be disabled with a runtime flag.> > Have you tried building LLVM/Clang with an ASan-ified LLVM/Clang? Curious > to see whether there's any latent bugs around. > > On Mon, Sep 7, 2020 at 7:50 AM Nathan James via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Dear list, >> >> I recently tried to add instrumentation to SmallVector for using >> Address sanitizer to detect cases where references used after they are >> invalidated. This basic implementation for this is here - >> https://reviews.llvm.org/D87237 >> >> However, in adding/testing this, I did uncover some questionable code. >> Firstly `SmallString<unsigned>::c_str()` and >> `Twine::toNullTerminatedStringRef(SmallVectorImpl<char>&)` both use >> bytes outside the range of the SmallVectors storage. This isn't >> inherently bad. >> Secondly calling `SmallVectorImpl<T>::insert(iterator, const T&)` >> results in a reference invalidation when the element to insert is >> contained inside the SmallVector and the SmallVector needs to grow for >> the insert. This has been fixed inside the aforementioned PR. >> >> My main point here is how does everyone feel about using ASAN to catch >> bugs like this not only inside SmallVector but also adding the >> instrumentation to some other containers used by llvm. If people are >> happy with this implementation for SmallVector I'd be happy for >> feedback on the PR. It would likely need some specific asan test cases >> however I'm not entirely sure how to go about adding those. >> >> Thanks for reading, >> >> ~Nathan >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://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/20200908/b00d6876/attachment.html>
Apparently Analagous Threads
- [LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h
- Crash in SmallVector with latest MSVC 2019 (debug mode)
- [LLVMdev] llvm/include/Support/FileSystem.h
- [LLVMdev] is the SmallVector implementation standard c++?
- [LLVMdev] [RFC] llvm/include/Support/FileOutputBuffer.h