<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 3, 2016 at 2:59 PM, Peter Collingbourne <span dir="ltr"><<a href="mailto:peter@pcc.me.uk" target="_blank">peter@pcc.me.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">pcc added a reviewer: rsmith.<br>
pcc added a comment.<br>
<br>
It seems to me that this sanitizer would break the semantics of otherwise well-defined programs. For example:<br>
<br>
  int *x = nullptr;<br>
  delete x;<br>
  if (x != nullptr) {<br>
    // normally unreachable<br>
  }<br>
<br>
It may be that a null comparison would be enough to avoid the semantics break, but I am not certain of this. Richard, what do you think?</blockquote><div><br></div><div>This sort of thing happens quite a lot. For instance, code like this is not uncommon:</div><div><br></div><div>  delete p;</div><div>  map.erase(p); </div><div><br></div><div>The C++ rule is that deallocating an object renders all pointers to it "invalid pointer values", but leaves it implementation-defined what you can do with an invalid pointer value. We don't have proper documentation of our implementation-defined behavior, but if we did, we would document that uses like the above are valid. The purpose of this being implementation-defined is to support architectures where loading a pointer to unmapped memory into some kind of pointer register can trap, so you could view this patch as adding a sanitizer for a certain kind of (theoretically) non-portable behavior.</div><div><br></div><div>However, the patch as-is is incorrect in several ways if we interpret it as a (partial) sanitizer for invalid pointer values: </div><div>1) it applies to all delete-expressions, even ones which call a class-specific operator delete (such operator delete functions do not necessarily deallocate the storage, so do not necessarily render the pointer invalid)</div><div>2) it applies even if the pointer value was originally null; null pointer values are not rendered invalid by a delete-expression</div><div>3) it blindly stores through any lvalue used as an operand to delete, potentially storing into read-only memory, creating data races, trampling over memory that was already cleared by the invoked destructor or memory that the operator delete function just released, and so on</div><div><br></div><div>As a particularly instructive case of #3, consider:</div><div><br></div><div>template<typename T></div><div>struct IntrusivePtrBase {</div><div>  T *complete_object;</div><div>  int refcnt;</div><div>  void decref() {</div><div>    if (--refcnt == 0)</div><div>      delete complete_object;</div><div>  }</div><div>};</div><div>struct X : IntrusivePtrBase<X> {</div><div>  X() : IntrusivePtrBase{this, 1} {}</div><div>};</div><div>X *p = new X;</div><div>p->decref();</div><div><br></div><div>Note that this new sanitizer will invent a store of garbage through complete_object after the containing object has been deallocated, probably corrupting the heap. I'm really not convinced this is a good idea.</div><div><br></div><div>Perhaps if this only applied to non-null, non-const, non-volatile local variables of pointer type, passed to a usual deallocation function, it might be OK. But I'd be concerned that it wouldn't provide enough value over ASan to justify its existence at that point.</div></div></div></div>