Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect Use after destruction (but before free) #73

Closed
ramosian-glider opened this issue Aug 31, 2015 · 24 comments
Closed

Detect Use after destruction (but before free) #73

ramosian-glider opened this issue Aug 31, 2015 · 24 comments

Comments

@ramosian-glider
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

>> 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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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](https://storage.googleapis.com/google-code-attachments/address-sanitizer/issue-73/comment-9/uad.patch)_

@ramosian-glider
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

>>  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
Copy link
Member Author

> 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
Copy link
Member Author

>> 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
Copy link
Member Author

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](https://storage.googleapis.com/google-code-attachments/address-sanitizer/issue-73/comment-16/uad.patch)_ - _Attachment: [uad_test.cc](https://storage.googleapis.com/google-code-attachments/address-sanitizer/issue-73/comment-16/uad_test.cc)_

@ramosian-glider
Copy link
Member Author

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
Copy link
Member Author

>> (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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

>> 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
Copy link
Member Author

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

  • Labels added: ProjectAddressSanitizer

@ramosian-glider
Copy link
Member Author

Adding Project:AddressSanitizer as part of GitHub migration.

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

@kcc
Copy link
Contributor

kcc commented Dec 1, 2015

This is implemented in msan as -fsanitize-memory-use-after-dtor.
Closing this bug.
Making this flag on by default is another story.

@kcc kcc closed this as completed Dec 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants