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

MSan use-after-destroy does not poison objects with implicit destructors #596

Open
nmusgrave opened this issue Sep 3, 2015 · 7 comments

Comments

@nmusgrave
Copy link

During code generation, no destructor exists to emit a destructor body for, so no members are poisoned. In order to do, a destructor must be generated for this object.

The creation of implicit destructors occurs during semantic analysis (clang/sema/). However, this step has no access to command line options.

Either semantic analysis must have access to the command line options, to check for msan and emit the required destructor, or another approach must be taken.

@eugenis
Copy link
Contributor

eugenis commented Sep 3, 2015

You probably meant with trivial implicit destructors.

@kcc
Copy link
Contributor

kcc commented Sep 3, 2015

Is there a test case?
Please add one to the test suite (it will fail now) and mention it here.

@eugenis
Copy link
Contributor

eugenis commented Sep 3, 2015

Another possible approach would be to track the place(s) in CodeGen where a destructor call for an object is be emitted, and, if the object has no destructor, call the poisoning function directly.

@nmusgrave
Copy link
Author

Test case in clang/test/CodeGenCXX/sanitize-dtor-generated.cpp fails currently.

llvm-mirror pushed a commit to llvm-mirror/clang that referenced this issue Sep 8, 2015
Summary:
If class or struct has not declared a destructor,
no destructor is emitted, and members are not poisoned
after destruction. This case highlights bug in current
implementation of use-after-dtor poisoning (detailed
in google/sanitizers#596).

Reviewers: eugenis, kcc

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D12616

Only check simplest object for existence of sanitizing callback.

Rename test.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@247025 91177308-0d34-0410-b5e6-96231b3b80d8
spurious pushed a commit to spurious/clang-mirror that referenced this issue Sep 8, 2015
Summary:
If class or struct has not declared a destructor,
no destructor is emitted, and members are not poisoned
after destruction. This case highlights bug in current
implementation of use-after-dtor poisoning (detailed
in google/sanitizers#596).

Reviewers: eugenis, kcc

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D12616

Only check simplest object for existence of sanitizing callback.

Rename test.

git-svn-id: http://llvm.org/svn/llvm-project/cfe/trunk@247025 91177308-0d34-0410-b5e6-96231b3b80d8
3bueckle pushed a commit to 3bueckle/CS3041 that referenced this issue Oct 12, 2015
Summary:
If class or struct has not declared a destructor,
no destructor is emitted, and members are not poisoned
after destruction. This case highlights bug in current
implementation of use-after-dtor poisoning (detailed
in google/sanitizers#596).

Reviewers: eugenis, kcc

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D12616

Only check simplest object for existence of sanitizing callback.

Rename test.
@morehouse
Copy link
Contributor

Do we really want to poison trivially-destructable objects? AFAIK, they are the preferred (safe) way to avoid destruction-order issues among globals.

Among other things, use-after-dtor of trivially-destructable globals is allowed by Google's style guide.

@morehouse morehouse self-assigned this Jun 7, 2018
@anforowicz
Copy link

Do we really want to poison trivially-destructable objects?

According to a memory-safety-dev@ discussion accessing a trivially-destructed field (e.g. a raw pointer, while the pointee is still alive) will be Undefined Behavior in C++20. But even before C++20 it seems undesirable that changing a raw pointer Foo* to a smart pointer RawPtr<Foo> can lead to trouble (and therefore it would be great if the sanitizers could detect such situation).

@eugenis
Copy link
Contributor

eugenis commented Dec 16, 2020

Interesting. It seems pretty easy to implement, too: this loop needs to poison trivially destructible fields in order instead of doing that in bulk before the loop. It can be a bit tricky to merge poisoning calls for adjacent members.

For pre-c++20 that would have be under a flag. @vitalybuka FYI

@morehouse morehouse assigned eugenis and unassigned morehouse Jul 1, 2022
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

6 participants