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
