[cfe-dev] annotating libc++ to catch buffer overflows in vector/string with asan

Kostya Serebryany kcc at google.com
Wed Jan 15 01:53:45 PST 2014


Marshall,
Any chance to start the review this month?
Slightly updated patch attached.
FYI: we applied the same approach to libstdc++, ran various tests and found
dozens of bugs.
These annotations also help to achieve more precise leak detection with
LeakSanitizer.
https://code.google.com/p/address-sanitizer/wiki/ContainerOverflow

If this patch has any chance to get committed eventually, I'd like to
understand your preference regarding the testing.
One approach is to test it with asan: then all you need is to run all
existing vector tests built with
clang -fsanitize=address -D_LIBCPP_ADDRESS_SANITIZER_ANNOTATIONS
But this will tie the testing process to clang, I am not sure if that is
acceptable for you (totally ok for me).
Another approach is to provide a fake standalone implementation of
__sanitizer_annotate_contiguous_container;
it will be a slightly less accurate testing, but will not depend on clang.

Thanks,

--kcc



On Tue, Dec 10, 2013 at 6:07 PM, Kostya Serebryany <kcc at google.com> wrote:

> Here is another version; this time I've been able to build and run
> Chromium with libc++ and these annotations.
>
> --kcc
>
>
> On Mon, Dec 9, 2013 at 6:58 PM, Kostya Serebryany <kcc at google.com> wrote:
>
>> Marshall,
>> Here is a more complete patch for asan vector annotations.
>> It passes all tests in libcxx/test/containers/sequences/vector, but
>> doesn't yet work on larger programs (I tried Chrome).
>> Two questions:
>>    1. Does it look like something potentially committable?
>>    2. Would you suggest some bigger test suite specifically targeted to
>> test std::vector?
>>
>> Thanks,
>>
>> --kcc
>>
>>
>>
>> On Wed, Nov 20, 2013 at 6:35 PM, Kostya Serebryany <kcc at google.com>wrote:
>>
>>> Attached is an intermediate patch that passes my tests (also attached).
>>> Few questions:
>>>
>>> The patch gets a bit too verbose and it has a few repetitions, e.g. we
>>> have this snippet repeated multiple times:
>>> +            __annotate_contiguous_container(data(), data() + capacity(),
>>> +                                            data() + size(), data() +
>>> size() + 1);
>>>
>>> WDYT about adding a private method to vector like this (and a couple
>>> more)?
>>>
>>> void __annotate_size_increase(size_t __n) {
>>>     __annotate_contiguous_container(data(), data() + capacity(),
>>>                                             data() + size(), data() +
>>> size() + __n);
>>> }
>>>
>>> In order to test the annotation properly I need to do something like
>>> this as often as possible:
>>> template <class T> void TestVector(std::vector<T> &v) {
>>>
>>>   if (v.capacity() == 0) return;
>>>
>>>   char *beg = (char*)&v.front();
>>>
>>>   char *end = beg + v.capacity() * sizeof(T);
>>>
>>>   char *mid = beg + v.size() * sizeof(T);
>>>
>>>   for (char *p = beg; p < mid; p++)
>>>
>>>     assert(!__asan_address_is_poisoned(p));
>>>
>>>   for (char *p = mid ; p < end; p++)
>>>
>>>     assert(__asan_address_is_poisoned(p));
>>>
>>> }
>>>
>>> Is it possible to add code like this as another private method of vector
>>> (under some ifdef)?
>>>
>>> --kcc
>>>
>>>
>>> On Tue, Nov 19, 2013 at 1:57 PM, Kostya Serebryany <kcc at google.com>wrote:
>>>
>>>>
>>>>> > Do you like the idea?
>>>>>
>>>>> Yes - very much so.
>>>>>
>>>>> > Any comments about the prototype patch?
>>>>>
>>>>> Yes.
>>>>>
>>>>> 1) I would implement it with inline functions other than macros.
>>>>>
>>>>> Like this:
>>>>>
>>>>>
>>>>> extern "C" void __sanitizer_annotate_contiguous_container(void *, void
>>>>> *, void *, void *);
>>>>> void inline __annotate_contiguous container
>>>>>         ( const void *__beg, const void *__end, const void *__old_mid,
>>>>> const void *new_mid )
>>>>> #if __has_feature(address_sanitizer)
>>>>>         {  __sanitizer_annotate_contiguous_container(beg, end,
>>>>> old_mid, new_mid); }
>>>>> #else
>>>>>         {}
>>>>> #endif
>>>>>
>>>>
>>>> Makes sense, see below.
>>>> We''l probably want to move __annotate_contiguous container into a
>>>> separate file so that both string and vector use it.
>>>> But let's do the whole thing with vector before moving to string.
>>>>
>>>>
>>>>
>>>>>
>>>>> 2) I'd like more information about what
>>>>> __sanitizer_annotate_contiguous_container does ;-)
>>>>>
>>>>
>>>> In short, it [un]poisons the parts of shadow memory that correspond to
>>>> the container's memory.
>>>> More details in compiler-rt files:
>>>>  include/sanitizer/common_interface_defs.h and lib/asan/asan_poisoning.cc
>>>>
>>>>
>>>>
>>>>>
>>>>> > Any suggestion how to test these annotations in libc++ (we need some
>>>>> kind of DEATH tests)?
>>>>>
>>>>> We already have failing tests in libc++.
>>>>> they are named XXXX.fail.cpp.
>>>>>
>>>>
>>>> These are tests that fail to compile.
>>>> for asan annotations we need tests that build and run, and then either
>>>> fail at run-time or check the state of shadow.
>>>> Like these:
>>>> compiler-rt/lib/asan/lit_tests/TestCases/contiguous_container.cc
>>>> compiler-rt/lib/asan/lit_tests/TestCases/contiguous_container_crash.cc
>>>>
>>>> Suggestions?
>>>>
>>>> --kcc
>>>>
>>>> --- include/vector      (revision 195116)
>>>> +++ include/vector      (working copy)
>>>> @@ -288,6 +288,16 @@
>>>>
>>>>  _LIBCPP_BEGIN_NAMESPACE_STD
>>>>
>>>> +extern "C" void __sanitizer_annotate_contiguous_container(
>>>> +  const void *, const void *, const void *, const void *);
>>>> +void inline __annotate_contiguous_container
>>>> +    (const void *__beg, const void *__end, const void *__old_mid,
>>>> const void *__new_mid)
>>>> +#if __has_feature(address_sanitizer)
>>>> +    {  __sanitizer_annotate_contiguous_container(__beg, __end,
>>>> __old_mid, __new_mid); }
>>>> +#else
>>>> +    {}
>>>> +#endif
>>>> +
>>>>  template <bool>
>>>>  class __vector_base_common
>>>>  {
>>>> @@ -1525,7 +1535,12 @@
>>>>      // __v.push_back(_VSTD::forward<_Up>(__x));
>>>>      __alloc_traits::construct(__a,
>>>> _VSTD::__to_raw_pointer(__v.__end_), _VSTD::forward<_Up>(__x));
>>>>      __v.__end_++;
>>>> +    if (data())
>>>> +      __annotate_contiguous_container(
>>>>  +          data(), data() + capacity(), data() + size(), data() +
>>>> capacity());
>>>>      __swap_out_circular_buffer(__v);
>>>> +    __annotate_contiguous_container(data(), data() + capacity(),
>>>> +                                          data() + capacity(), data()
>>>> + size());
>>>>  }
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140115/dedca5e0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vector_asan.patch
Type: text/x-patch
Size: 7648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140115/dedca5e0/attachment.bin>


More information about the cfe-dev mailing list