mailed http://reviews.llvm.org/D4170
On Thu, Jun 12, 2014 at 11:11 AM, Kostya Serebryany <k...@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.li...@gmail.com> > wrote: > >> >> On May 13, 2014, at 6:48 AM, Kostya Serebryany <k...@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 <k...@google.com> >> wrote: >> >>> >>> >>> >>> On Mon, May 12, 2014 at 7:04 PM, Stephan Tolksdorf <s...@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 >>>> >>>> >>> >> >> >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
