[PATCH] D25199: [ubsan] Sanitize deleted pointers

[Richard Smith via cfe-commits]

On Mon, Oct 3, 2016 at 2:59 PM, Peter Collingbourne <peter at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161004/6803686b/attachment.html>