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

[Kostya Serebryany]

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?

--kcc





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/602b2b76/attachment.html>