[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326245: [analyzer] Dont crash when dynamic type of a
variable is set via placement new. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43659?vs=135572=136137#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43659
Files:
cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
cfe/trunk/test/Analysis/new-dynamic-types.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -587,7 +587,15 @@
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty,
Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
if (!ThisVal.isUnknown())
Index: cfe/trunk/test/Analysis/new-dynamic-types.cpp
===
--- cfe/trunk/test/Analysis/new-dynamic-types.cpp
+++ cfe/trunk/test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test_ub() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
+
+void test_non_ub() {
+ char c[sizeof(D)]; // Should be enough storage.
+ new (c) D;
+ ((B *)c)->foo(); // no-crash
+}
Index: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -587,7 +587,15 @@
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
if (!ThisVal.isUnknown())
Index: cfe/trunk/test/Analysis/new-dynamic-types.cpp
===
--- cfe/trunk/test/Analysis/new-dynamic-types.cpp
+++ cfe/trunk/test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test_ub() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
+
+void test_non_ub() {
+ char c[sizeof(D)]; // Should be enough storage.
+ new (c) D;
+ ((B *)c)->foo(); // no-crash
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
NoQ added a comment. > In this case, could we emit a warning? If not from CallEvent, then from where? (1) This might result in a buffer overflow, so i home that `alpha.ArrayBound` may eventually catch it. (2) It might be a good idea to make a fairly generic checker for the strict aliasing rule. https://reviews.llvm.org/D43659 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:593 +// Fall back to a generic pointer cast for this-value. +const CXXMethodDecl *StaticMD = cast(getDecl()); +const CXXRecordDecl *StaticClass = StaticMD->getParent(); george.karpenkov wrote: > nit: auto? Dunno, i somehow like the symmetry with the next line. "Take method decl, take record decl". Maybe it's just me. https://reviews.llvm.org/D43659 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
NoQ updated this revision to Diff 135572.
NoQ added a comment.
- Add a definitely-not-UB example (`char` buffers are special).
- Bring back an accidentally deleted blank line.
https://reviews.llvm.org/D43659
Files:
lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/new-dynamic-types.cpp
Index: test/Analysis/new-dynamic-types.cpp
===
--- /dev/null
+++ test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test_ub() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
+
+void test_non_ub() {
+ char c[sizeof(D)]; // Should be enough storage.
+ new (c) D;
+ ((B *)c)->foo(); // no-crash
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -587,7 +587,15 @@
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty,
Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
if (!ThisVal.isUnknown())
Index: test/Analysis/new-dynamic-types.cpp
===
--- /dev/null
+++ test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test_ub() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
+
+void test_non_ub() {
+ char c[sizeof(D)]; // Should be enough storage.
+ new (c) D;
+ ((B *)c)->foo(); // no-crash
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -587,7 +587,15 @@
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
if (!ThisVal.isUnknown())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. LGTM, though in general I still think we should do something on soft-failure-modes in order to aid future debugging... Is the failure mode in this case always a UB? In this case, could we emit a warning? If not from CallEvent, then from where? Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:593 +// Fall back to a generic pointer cast for this-value. +const CXXMethodDecl *StaticMD = cast(getDecl()); +const CXXRecordDecl *StaticClass = StaticMD->getParent(); nit: auto? Repository: rC Clang https://reviews.llvm.org/D43659 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43659: [analyzer] Don't crash when dynamic type of a concrete region is hard-set with placement new.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
This is assertion removal that i find valid. With placement new (which isn't
even need to be inlined, we used to model it conservatively well enough),
anything of any type can have any dynamic type. Even if we have a concrete
region of a variable, its dynamic type and its static type can be completely
unrelated. This might be UB due to strict aliasing rules, but we shouldn't
crash. This patch introduces a relatively sane behavior in this scenario that
consists of `evalCast()`ing this-region to the assumed dynamic type during
virtual calls.
In the common case when the dynamic type is a sub-class of the static type,
this is worse than calling `attemptDownCast()` because it adds element region
instead of removing base regions (which is not incorrect but produces a
non-canonical representation of the SVal). But when the common-case approach is
known to have failed, there doesn't seem to be a better option.
A lot of these crashes have suddenly shown up when i was testing temporaries.
They have nothing to do with temporaries though, but with a weird
implementation detail of `std::function` that suddenly got some if its methods
inlined.
Repository:
rC Clang
https://reviews.llvm.org/D43659
Files:
lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/new-dynamic-types.cpp
Index: test/Analysis/new-dynamic-types.cpp
===
--- /dev/null
+++ test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -583,11 +583,18 @@
ASTContext = SVB.getContext();
const CXXRecordDecl *Class = MD->getParent();
QualType Ty = Ctx.getPointerType(Ctx.getRecordType(Class));
-
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty,
Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
if (!ThisVal.isUnknown())
Index: test/Analysis/new-dynamic-types.cpp
===
--- /dev/null
+++ test/Analysis/new-dynamic-types.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -std=c++11 -verify %s
+
+// expected-no-diagnostics
+
+typedef __typeof(sizeof(int)) size_t;
+
+void *operator new(size_t size, void *ptr);
+
+struct B {
+ virtual void foo();
+};
+
+struct D : public B {
+ virtual void foo() override {}
+};
+
+void test() {
+ // FIXME: Potentially warn because this code is pretty weird.
+ B b;
+ new () D;
+ b.foo(); // no-crash
+}
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -583,11 +583,18 @@
ASTContext = SVB.getContext();
const CXXRecordDecl *Class = MD->getParent();
QualType Ty = Ctx.getPointerType(Ctx.getRecordType(Class));
-
// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
bool Failed;
ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
- assert(!Failed && "Calling an incorrectly devirtualized method");
+ if (Failed) {
+// We might have suffered some sort of placement new earlier, so
+// we're constructing in a completely unexpected storage.
+// Fall back to a generic pointer cast for this-value.
+const CXXMethodDecl *StaticMD = cast(getDecl());
+const CXXRecordDecl *StaticClass = StaticMD->getParent();
+QualType StaticTy = Ctx.getPointerType(Ctx.getRecordType(StaticClass));
+ThisVal = SVB.evalCast(ThisVal, Ty, StaticTy);
+ }
}
