<div dir="ltr">mailed <a href="http://reviews.llvm.org/D4170">http://reviews.llvm.org/D4170</a><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 12, 2014 at 11:11 AM, 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">FTR, I did not forget about this issue. <div>While trying to make a patch and tests I discovered what looks like a bug in std::vector, </div>
<div>please take a look: <a href="http://llvm.org/bugs/show_bug.cgi?id=20009" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=20009</a></div>
<div><br></div><div>--kcc </div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 14, 2014 at 8:45 AM, Marshall Clow <span dir="ltr"><<a href="mailto:mclow.lists@gmail.com" target="_blank">mclow.lists@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On May 13, 2014, at 6:48 AM, Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><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></blockquote><div><br></div></div>I think that we want to make these exception-aware; I thought that the existing test cases would have caught this.</div><div>
That tells me I need more tests for vector (in any case).</div><div><br></div><div>In general, I’m a fan of scope-related objects (things that clean up in the destructor).</div><div><br></div><div>I am at C++Now all this week; and I would really like to deal with this next week, if at all possible.</div>
<div>[ If this is causing people trouble, I would prefer to revert it and re-commit later. ]</div><span><font color="#888888"><div><br></div><div>— Marshall</div></font></span><div><div><div><br>
<br><blockquote type="cite"><div dir="ltr"><div></div></div><div class="gmail_extra"><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>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>
<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><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>
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>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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><span><font color="#888888">
- Stephan<br>
<br>
</font></span></blockquote></div></div><br></div></div>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>