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

[Kostya Serebryany]

FTR, I did not forget about this issue.
While trying to make a patch and tests I discovered what looks like a bug
in std::vector,
please take a look: http://llvm.org/bugs/show_bug.cgi?id=20009

--kcc


On Wed, May 14, 2014 at 8:45 AM, Marshall Clow <mclow.lists at gmail.com>
wrote:

>
> 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/20140612/ccb08789/attachment.html>