[libcxx] r208319 - Add Address Sanitizer support to std::vector

[Kostya Serebryany]

On Mon, May 12, 2014 at 7:04 PM, Stephan Tolksdorf <st at quanttec.com> wrote:

> On 14-05-12 16:27, Kostya Serebryany wrote:
> >         And an unrelated issue: the documentation for
> >         __sanitizer_annotate_contiguous_container states that the
> >         complete buffer should be unpoisened before it is deallocated.
> >         This doesn't seem to be happening in the destructor or in the
> >         deallocate function.
> >
> >     Here we rely on the fact that vector is using the default allocator,
> >     which immediately calls delete, which itself unpoisons the memory
> >     chunk.
>
> Do I understand you correctly, that the global ::operator delete doesn't
> actually require the complete buffer to be unpoisened? I assume this is
> also true for ::free then?
>
>
>  Reproduced. (we have very little code with exceptions so this has
>> avoided our testing efforts :)
>> Any suggestion for the fix?
>>
>
> Using a scope guard object for annotating operations that increase the
> size would be one possibility, as e.g. in the following pseudo code:
>

OMG. Yes, I can see how it works, although it looks so heavy. Let me (and
others) think a bit.


>
> struct AnnotatedAppendScope {
>   Vector& v;
>   const size_t newLength;
> #if USE_ASAN
>   bool committed;
> #endif
>
>   /// \pre vector.length() < newLength <= vector.capacity()
>   AnnotatedAppendScope(Vector& vector, size_t newLength)
>   : v(vector), newLength(newLength)
>   #if USE_ASAN
>     , committed(false)
>   #endif
>   {
>   #if USE_ASAN
>     __sanitizer_annotate_contiguous_container(
>       v.begin(), v.begin() + v.capacity(),
>       v.begin() + v.length(), v.begin() + newLength;
>     );
>   #endif
>   }
>
> #if USE_ASAN
>   ~AnnotatedAppendScope() {
>     if (!committed) {
>       __sanitizer_annotate_contiguous_container(
>         v.begin(), v.begin() + v.capacity(),
>         v.begin() + newLength, v.begin() + v.length();
>       );
>     }
>   }
> #endif
>
>   void commitLength() {
>     v.setLength(newLength);
>   #if USE_ASAN
>     committed = true;
>   #endif
>   }
> };
>
> void Vector::push_back(const T& value) {
>   reserve(length() + 1);
>   AnnotatedAppendScope scope(*this, length() + 1);
>   new (end()) T(value);
>   scope.commitLength();
> }
>
> There's another comment below.
>
>
>
>> #include <vector>
>> #include <assert.h>
>> #include <iostream>
>> #include <sanitizer/asan_interface.h>
>> using namespace std;
>>
>> class X {
>>   public:
>>    X(const X &x) {
>>      if (x.a == 42) throw 0;
>>      a = x.a;
>>    }
>>    X(char arg): a(arg){}
>>    char get() const { return a; }
>>   private:
>>    char a;
>> };
>>
>> int main() {
>>    vector<X> v;
>>    v.reserve(2);
>>    v.push_back(X(2));
>>    assert(v.size() == 1);
>>    try {
>>      v.push_back(X(42));
>>      assert(0);
>>    } catch (int e) {
>>      assert(v.size() == 1);
>>    }
>>    assert(v.size() == 1);
>>    assert(v[1].get() != 777);
>>
>
> What does the above line test?
> It would always fail if the index is assert checked, which currently is
> the case when _LIBCPP_DEBUG is defined.


I just wanted to verify that asan does not catch the bug. This line is
indeed redundant.


>
>
>     char *p = (char*)v.data();
>>    assert(!__asan_address_is_poisoned(p));
>>    assert(__asan_address_is_poisoned(p+1));
>> }
>>
>>
> - Stephan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140512/795f96a8/attachment.html>