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

[Marshall Clow]

On May 13, 2014, at 6:48 AM, Kostya Serebryany <kcc at google.com> wrote:

> This reminds me https://code.google.com/p/address-sanitizer/issues/detail?id=258
> (and also reminds me that I hate exceptions :)
> 
> The situation you describe (and my test shows) may lead only to false negatives (missed bugs), 
> and not to false positives or crashes (the checks in __sanitizer_annotate_contiguous_container are disabled due to the above bug).
> At least I don't see how these may happen. 
> 
> Of course, __sanitizer_verify_contiguous_container will fail on such test, but that's a test-only routine anyway. 
> We may eventually fix the problem with RAII, as you suggested, but maybe it's ok to keep the code as is for a while. 
> 
> Marshall, your thoughts? 

I think that we want to make these exception-aware; I thought that the existing test cases would have caught this.
That tells me I need more tests for vector (in any case).

In general, I’m a fan of scope-related objects (things that clean up in the destructor).

I am at C++Now all this week; and I would really like to deal with this next week, if at all possible.
[ If this is causing people trouble, I would prefer to revert it and re-commit later. ]

— Marshall


> 
> On Mon, May 12, 2014 at 8:35 PM, Kostya Serebryany <kcc at google.com> wrote:
> 
> 
> 
> 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/20140513/53656aba/attachment.html>