[libcxx] r208319 - Add Address Sanitizer support to std::vector
Kostya Serebryany
kcc at google.com
Tue Jun 17 01:39:27 PDT 2014
mailed http://reviews.llvm.org/D4170
On Thu, Jun 12, 2014 at 11:11 AM, Kostya Serebryany <kcc at google.com> wrote:
> 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/20140617/8332dd40/attachment.html>
More information about the cfe-commits
mailing list