llvm dev - Sep 2020 - [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

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>

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>