On Mon, Oct 3, 2016 at 2:59 PM, Peter Collingbourne <pe...@pcc.me.uk> wrote:
> pcc added a reviewer: rsmith. > pcc added a comment. > > It seems to me that this sanitizer would break the semantics of otherwise > well-defined programs. For example: > > int *x = nullptr; > delete x; > if (x != nullptr) { > // normally unreachable > } > > 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? This sort of thing happens quite a lot. For instance, code like this is not uncommon: delete p; map.erase(p); 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. However, the patch as-is is incorrect in several ways if we interpret it as a (partial) sanitizer for invalid pointer values: 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) 2) it applies even if the pointer value was originally null; null pointer values are not rendered invalid by a delete-expression 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 As a particularly instructive case of #3, consider: template<typename T> struct IntrusivePtrBase { T *complete_object; int refcnt; void decref() { if (--refcnt == 0) delete complete_object; } }; struct X : IntrusivePtrBase<X> { X() : IntrusivePtrBase{this, 1} {} }; X *p = new X; p->decref(); 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. 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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits