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
