<div dir="ltr">This reminds me <a href="https://code.google.com/p/address-sanitizer/issues/detail?id=258" target="_blank">https://code.google.com/p/address-sanitizer/issues/detail?id=258</a><div>(and also reminds me that I hate exceptions :)</div>
<div><br></div><div>The situation you describe (and my test shows) may lead only to false negatives (missed bugs), </div><div>and not to false positives or crashes (the checks in __sanitizer_annotate_contiguous_container are disabled due to the above bug).</div>
<div>At least I don't see how these may happen. </div><div><br></div><div>Of course, __sanitizer_verify_contiguous_container will fail on such test, but that's a test-only routine anyway. </div><div>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. <br>
</div><div><br></div><div>Marshall, your thoughts? </div><div><br></div><div>--kcc </div><div><br></div><div>
<br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, May 12, 2014 at 8:35 PM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div class="">On Mon, May 12, 2014 at 7:04 PM, Stephan Tolksdorf <span dir="ltr"><<a href="mailto:st@quanttec.com" target="_blank">st@quanttec.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 14-05-12 16:27, Kostya Serebryany wrote:<br>
> And an unrelated issue: the documentation for<br>
> __sanitizer_annotate_<u></u>contiguous_container states that the<br>
> complete buffer should be unpoisened before it is deallocated.<br>
> This doesn't seem to be happening in the destructor or in the<br>
> deallocate function.<br>
><br>
> Here we rely on the fact that vector is using the default allocator,<br>
> which immediately calls delete, which itself unpoisons the memory<br>
> chunk.<br>
<br></div>
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?<div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Reproduced. (we have very little code with exceptions so this has<br>
avoided our testing efforts :)<br>
Any suggestion for the fix?<br>
</blockquote>
<br></div>
Using a scope guard object for annotating operations that increase the size would be one possibility, as e.g. in the following pseudo code:<br></blockquote><div><br></div></div><div>OMG. Yes, I can see how it works, although it looks so heavy. Let me (and others) think a bit. </div>
<div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
struct AnnotatedAppendScope {<br>
Vector& v;<br>
const size_t newLength;<br>
#if USE_ASAN<br>
bool committed;<br>
#endif<br>
<br>
/// \pre vector.length() < newLength <= vector.capacity()<br>
AnnotatedAppendScope(Vector& vector, size_t newLength)<br>
: v(vector), newLength(newLength)<br>
#if USE_ASAN<br>
, committed(false)<br>
#endif<br>
{<br>
#if USE_ASAN<br>
__sanitizer_annotate_<u></u>contiguous_container(<br>
v.begin(), v.begin() + v.capacity(),<br>
v.begin() + v.length(), v.begin() + newLength;<br>
);<br>
#endif<br>
}<br>
<br>
#if USE_ASAN<br>
~AnnotatedAppendScope() {<br>
if (!committed) {<br>
__sanitizer_annotate_<u></u>contiguous_container(<br>
v.begin(), v.begin() + v.capacity(),<br>
v.begin() + newLength, v.begin() + v.length();<br>
);<br>
}<br>
}<br>
#endif<br>
<br>
void commitLength() {<br>
v.setLength(newLength);<br>
#if USE_ASAN<br>
committed = true;<br>
#endif<br>
}<br>
};<br>
<br>
void Vector::push_back(const T& value) {<br>
reserve(length() + 1);<br>
AnnotatedAppendScope scope(*this, length() + 1);<br>
new (end()) T(value);<br>
scope.commitLength();<br>
}<br>
<br>
There's another comment below.<div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
#include <vector><br>
#include <assert.h><br>
#include <iostream><br>
#include <sanitizer/asan_interface.h><br>
using namespace std;<br>
<br>
class X {<br>
public:<br>
X(const X &x) {<br>
if (x.a == 42) throw 0;<br>
a = x.a;<br>
}<br>
X(char arg): a(arg){}<br>
char get() const { return a; }<br>
private:<br>
char a;<br>
};<br>
<br>
int main() {<br>
vector<X> v;<br>
v.reserve(2);<br>
v.push_back(X(2));<br>
assert(v.size() == 1);<br>
try {<br>
v.push_back(X(42));<br>
assert(0);<br>
} catch (int e) {<br>
assert(v.size() == 1);<br>
}<br>
assert(v.size() == 1);<br>
assert(v[1].get() != 777);<br>
</blockquote>
<br></div></div>
What does the above line test?<br>
It would always fail if the index is assert checked, which currently is the case when _LIBCPP_DEBUG is defined.</blockquote><div><br></div></div></div><div>I just wanted to verify that asan does not catch the bug. This line is indeed redundant. </div>
<div class="">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
char *p = (char*)v.data();<br>
assert(!__asan_address_is_<u></u>poisoned(p));<br>
assert(__asan_address_is_<u></u>poisoned(p+1));<br>
}<br>
<br>
</blockquote>
<br></div></div><span><font color="#888888">
- Stephan<br>
<br>
</font></span></blockquote></div></div><br></div></div>
</blockquote></div><br></div>