Detect Use after destruction (but before free)

[ramosian-glider]

Originally reported on Google Code with ID 73

We may want to poison memory on a call to a destructor so that 
a use of destructed (but not yet deleted) object is detected. 
The tricky part here is how to handle placement new followed by an explicit 
call to a DTOR. 

Reported by konstantin.s.serebryany on 2012-05-22 13:08:44

[ramosian-glider]

test case: 

% cat UAD/uad_test.cc 
#include <stdio.h>
#include "asan_interface.h"

volatile double z;

struct A {
  double x;
  void TouchMe() {
    z = x;
  }
  ~A() {
    printf("A::~A\n");
    ASAN_POISON_MEMORY_REGION(this, sizeof(*this));
  }
};

struct B {
  void SetA(A *a) { a_ = a; }
  ~B() {
    printf("B::~B\n");
    a_->TouchMe();
  }
  A *a_;
};
struct Outer {
  B b;
  A a;
  Outer() {
    b.SetA(&a);
  }
};

int main() {
  Outer o;
}
% clang++ UAD/uad_test.cc -I. -faddress-sanitizer  &&  ./a.out 

=================================================================
==678== ERROR: AddressSanitizer use-after-poison on address 0x7fff2e07c508 at pc 0x4055a0
bp 0x7fff2e07bf70 sp 0x7fff2e07bf68
READ of size 8 at 0x7fff2e07c508 thread T0
...

It would be nice to be able to insert ASAN_POISON_MEMORY_REGION automatically, but
see my next comment

Reported by konstantin.s.serebryany on 2012-05-22 13:43:28

[ramosian-glider]

and here is the counter-example. 
Can we distinguish between this and the previous one? 

% cat UAD/uad_placement_new.cc 
#include <stdio.h>
#include <new>
#include "asan_interface.h"

struct A {
  double x;
  ~A() {
    printf("A::~A\n");
    ASAN_POISON_MEMORY_REGION(this, sizeof(*this));
  }
};

volatile double z;

int main() {
  double obj;
  A *a = new (&obj) A;
  a->~A();
  obj = z;
  return obj >= 0.0;
}

% clang++ UAD/uad_placement_new.cc -I. -faddress-sanitizer  &&  ./a.out 
==13102== ERROR: AddressSanitizer use-after-poison on address 0x7ffff0802e00 at pc
0x404b78 bp 0x7ffff0802d10 sp 0x7ffff0802d08
...

Reported by konstantin.s.serebryany on 2012-05-22 13:50:52

[ramosian-glider]

This actually sounds more like the use case for uninitialized value tracking tool (msan).

Reported by konstantin.s.serebryany on 2012-05-22 14:18:20

[ramosian-glider]

Can such a bug be detected by a simpler msan implementation that just checks that the
client C++ classes are initialized? Perhaps this will almost always be done in the
instrumented code, not in the libraries.

Reported by ramosian.glider on 2012-05-22 14:22:45

[ramosian-glider]

>> Can such a bug be detected by a simpler msan implementation
I doubt so. 
Once a memory is poisoned, we need to un-poison it on stores, which may happen inside
a library. 

Reported by konstantin.s.serebryany on 2012-05-22 14:58:35

[ramosian-glider]

another test case

#include <cstring>

class A
{
public:
    A(int x):_x(x){}
    int _x;
};

//#define CLEAR_FOR_CRASH

class B
{
public:
    B(const A& a):_a(a){}
    ~B()
    {
#if defined(CLEAR_FOR_CRASH)
      ::memset(this,0xff,sizeof(B)); // invalidates B
#endif
    }
    const A& _a; // want a copy but forget to remove & on refactor
};

class C
{
public:
    C(const B& b):_b(b){}
    const B& _b;
};

int main()
{
   const A a(10);
   const C c(a); // temporary B is created and destroyed
   int x2 = c._b._a._x; // crashes on defined CLEAR_FOR_CRASH - else not

   return 0;
}

Reported by dennis.luehring on 2013-10-31 07:15:27

[ramosian-glider]

After various discussion we agreed that detecting such bugs is more appropriate 
for MemorySanitizer (https://code.google.com/p/memory-sanitizer/) 
than in AddressSanitizer. But since most of the work is still in Clang,
and an optional feature in AddressSanitizer still makes sense, let's keep 
this feature request here. 

Reported by konstantin.s.serebryany on 2013-12-23 14:59:46

[ramosian-glider]

Re: c#2 - is it a correct C++ code?
Doesn't it break some aliasing rules?

Reported by timurrrr@google.com on 2014-02-05 01:29:28

[ramosian-glider]

Attached is a small draft patch that detects bugs in c#1 and c#6.

It should be fairly easy to have a more strict poisoning of holes and tails in classes
with inheritance.
Also, needs lit tests :)

Reported by timurrrr@google.com on 2014-02-05 01:37:22

---

• Attachment: uad.patch

[ramosian-glider]

TODO: Also, need to un-poison the memory in the constructors

Reported by timurrrr@google.com on 2014-02-05 01:46:59

[ramosian-glider]

Re: constructors.
__asan_unpoison_memory_region doesn't seem to check the "This memory must be previously
allocated by the user program." condition, so it's not directly usable in constructors.

We should probably have something like __asan_unpoison_memory_region_if_addressable,
WDYT?

Reported by timurrrr@google.com on 2014-02-05 02:20:15

[ramosian-glider]

IMO c#2 is valid C++.  I think a more elaborate case would look like:

struct A {
  A() : a(42) {}
  ~A() {} // poisons a
  int a;
};
int main() {
  char buffer[sizeof(A)];
  A *a = new (buffer) A();
  a->~A();
  // buffer is now free.  We should be able to reuse that memory to construct new objects,
but it is poisoned, and we can't monitor stores and loads differently in asan.
  a = new (buffer) A(); // BOOM
  a->A();
}

Reported by rnk@google.com on 2014-02-05 08:01:57

[ramosian-glider]

>>  and we can't monitor stores and loads differently in asan.
Exactly. remember, we want to make this feature in msan, not in asan, at least at first.

Reported by konstantin.s.serebryany on 2014-02-05 08:07:24

[ramosian-glider]

> IMO c#2 is valid C++.
I've talked to Richard about that today and he said it's a gray area of the standard.
Basically, there's no guarantee this will be allowed by a generic compiler.

Re: doing a placement new after calling destructor - that's what I'm talking about
in c#11. 

Reported by timurrrr@google.com on 2014-02-05 08:22:42

[ramosian-glider]

>> TODO: Also, need to un-poison the memory in the constructors
This is useless, the memory after DTOR may be legally used w/o constructing a new 
C++ object. 

Reported by konstantin.s.serebryany on 2014-02-06 07:54:45

[ramosian-glider]

Comments on the patch: 

>> if (SanOpts->Address
This should be (SanOpts->Address || SanOpts->Memory || SanOpts->Thread)

>> DtorType == Dtor_Complete
This is never true even on a simple test (attached). Please double-check. 

>> "__asan_poison_memory_region"
This should be "__sanitizer_dtor_exit_callback",
every tool will implement this callback in its own way. 
For msan a simple implementation (w/o origins) would be 
to call __msan_poison(addr, size);

I've made these changes and modified the original test case and got this nice report
from msan:
==19562== WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f55690fd2a9 in A::TouchMe() uad_test.cc:6
    #1 0x7f55690fd123 in B::~B() uad_test.cc:15
    #2 0x7f55690fcfab in Outer::~Outer() uad_test.cc:19
    #3 0x7f55690fcef1 in main uad_test.cc:24

Reported by konstantin.s.serebryany on 2014-02-06 08:24:48

---

• Attachment: uad.patch
• _Attachment: uad_test.cc_

[ramosian-glider]

Should probably call the function __sanitizer_something and have it defined in msan
and asan runtimes (asan under a flag, disabled by default).

Reported by eugenis@google.com on 2014-02-06 08:25:12

[ramosian-glider]

>> (asan under a flag, disabled by default).
Yep. This flag should be common for all tools (e.g. detect_use_after_dtor)
and true by default only in msan.

Reported by konstantin.s.serebryany on 2014-02-06 08:31:47

[ramosian-glider]

per offline discussion, the current plan is to only do this for MSan at first and only
in destructors.
The currently-unhandled interesting case is to poison the fields owned by a class when
we leave its destructor.

Reported by timurrrr@google.com on 2014-02-12 14:40:14

[ramosian-glider]

Dmitry suggested to extend this to constructors, ex. even when an object is constructed
in an initialized buffer, we should first poison the entire object, and then unpoison
parts of it in sub-object constructors.

Reported by eugenis@google.com on 2014-08-08 14:27:06

[ramosian-glider]

>> IMO c#2 is valid C++.
> I've talked to Richard about that today and he said it's a gray area of the standard.
> Basically, there's no guarantee this will be allowed by a generic compiler.

New versions of GCC seem to have enabled-by-default flag -flifetime-dse for this (https://gcc.gnu.org/ml/gcc-patches/2015-04/msg00782.html).

Reported by tetra2005x on 2015-06-15 08:58:32

[ramosian-glider]

Reported by ramosian.glider on 2015-07-30 09:05:30

• Labels added: ProjectAddressSanitizer

[ramosian-glider]

Adding Project:AddressSanitizer as part of GitHub migration.

Reported by ramosian.glider on 2015-07-30 09:06:54