[clang] a504ddc - [analyzer] Initialize regions returned by CXXNew to undefined
Kristóf Umann via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 26 08:22:35 PDT 2022
Author: Kristóf Umann
Date: 2022-10-26T17:22:12+02:00
New Revision: a504ddc8bf9d5c406ea88b84b8495d7aae200d4c
URL: https://github.com/llvm/llvm-project/commit/a504ddc8bf9d5c406ea88b84b8495d7aae200d4c
DIFF: https://github.com/llvm/llvm-project/commit/a504ddc8bf9d5c406ea88b84b8495d7aae200d4c.diff
LOG: [analyzer] Initialize regions returned by CXXNew to undefined
Discourse mail:
https://discourse.llvm.org/t/analyzer-why-do-we-suck-at-modeling-c-dynamic-memory/65667
malloc() returns a piece of uninitialized dynamic memory. We were (almost)
always able to model this behaviour. Its C++ counterpart, operator new is a
lot more complex, because it allows for initialization, the most complicated of which is the usage of constructors.
We gradually became better in modeling constructors, but for some reason, most
likely for reasons lost in history, we never actually modeled the case when the
memory returned by operator new was just simply uninitialized. This patch
(attempts) to fix this tiny little error.
Differential Revision: https://reviews.llvm.org/D135375
Added:
Modified:
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/test/Analysis/NewDelete-checker-test.cpp
clang/test/Analysis/cxx-member-initializer-const-field.cpp
clang/test/Analysis/new-ctor-conservative.cpp
clang/test/Analysis/new-ctor-recursive.cpp
clang/test/Analysis/new.cpp
clang/test/Analysis/placement-new.cpp
clang/test/Analysis/reinterpret-cast.cpp
clang/test/Analysis/uninit-const.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 476afc598ac6c..38c8896b10ba7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,15 +10,16 @@
//
//===----------------------------------------------------------------------===//
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/Analysis/ConstructionContext.h"
#include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtCXX.h"
#include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/ConstructionContext.h"
#include "clang/Basic/PrettyStackTrace.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
@@ -953,6 +954,11 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
// skip it for now.
ProgramStateRef State = I->getState();
SVal RetVal = State->getSVal(CNE, LCtx);
+ // [basic.stc.dynamic.allocation] (on the return value of an allocation
+ // function):
+ // "The order, contiguity, and initial value of storage allocated by
+ // successive calls to an allocation function are unspecified."
+ State = State->bindDefaultInitial(RetVal, UndefinedVal{}, LCtx);
// If this allocation function is not declared as non-throwing, failures
// /must/ be signalled by exceptions, and thus the return value will never
diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp
index 5a06c8327f717..1100b49e84d3a 100644
--- a/clang/test/Analysis/NewDelete-checker-test.cpp
+++ b/clang/test/Analysis/NewDelete-checker-test.cpp
@@ -385,7 +385,11 @@ class DerefClass{
public:
int *x;
DerefClass() {}
- ~DerefClass() {*x = 1;}
+ ~DerefClass() {
+ int i = 0;
+ x = &i;
+ *x = 1;
+ }
};
void testDoubleDeleteClassInstance() {
diff --git a/clang/test/Analysis/cxx-member-initializer-const-field.cpp b/clang/test/Analysis/cxx-member-initializer-const-field.cpp
index f0abbddbc4441..98076124d82b7 100644
--- a/clang/test/Analysis/cxx-member-initializer-const-field.cpp
+++ b/clang/test/Analysis/cxx-member-initializer-const-field.cpp
@@ -10,7 +10,7 @@ struct WithConstructor {
WithConstructor(int *x) : ptr(x) {}
static auto compliant() {
- WithConstructor c(new int);
+ WithConstructor c(new int{});
return *(c.ptr); // no warning
}
@@ -28,7 +28,7 @@ struct RegularAggregate {
int *const ptr = nullptr;
static int compliant() {
- RegularAggregate c{new int};
+ RegularAggregate c{new int{}};
return *(c.ptr); // no warning
}
diff --git a/clang/test/Analysis/new-ctor-conservative.cpp b/clang/test/Analysis/new-ctor-conservative.cpp
index bcc33fb8f8970..1fcb6708e43e1 100644
--- a/clang/test/Analysis/new-ctor-conservative.cpp
+++ b/clang/test/Analysis/new-ctor-conservative.cpp
@@ -14,9 +14,12 @@ void checkConstructorInlining() {
clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
}
-void checkNewPOD() {
+void checkNewPODunit() {
int *i = new int;
- clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(*i == 0); // expected-warning{{The left operand of '==' is a garbage value [core.UndefinedBinaryOperatorResult]}}
+}
+
+void checkNewPOD() {
int *j = new int();
clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
int *k = new int(5);
diff --git a/clang/test/Analysis/new-ctor-recursive.cpp b/clang/test/Analysis/new-ctor-recursive.cpp
index f21795d1739aa..4aca1d832bf28 100644
--- a/clang/test/Analysis/new-ctor-recursive.cpp
+++ b/clang/test/Analysis/new-ctor-recursive.cpp
@@ -51,11 +51,9 @@ void *operator new(size_t size, S *place) {
void testThatCharConstructorIndeedYieldsGarbage() {
S *s = new S(ConstructionKind::Garbage);
- clang_analyzer_eval(s->x == 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(s->x == 1); // expected-warning{{UNKNOWN}}
- // FIXME: This should warn, but MallocChecker doesn't default-bind regions
- // returned by standard operator new to garbage.
- s->x += 1; // no-warning
+ clang_analyzer_eval(s->x == 0); // expected-warning{{The left operand of '==' is a garbage value [core.UndefinedBinaryOperatorResult]}}
+ clang_analyzer_eval(s->x == 1);
+ s->x += 1;
delete s;
}
diff --git a/clang/test/Analysis/new.cpp b/clang/test/Analysis/new.cpp
index 4b93baf345642..3542b5c594b50 100644
--- a/clang/test/Analysis/new.cpp
+++ b/clang/test/Analysis/new.cpp
@@ -177,15 +177,10 @@ void testAggregateNew() {
clang_analyzer_eval(p.y == 2); // expected-warning{{TRUE}}
}
-//--------------------------------
-// Incorrectly-modelled behavior
-//--------------------------------
-
int testNoInitialization() {
int *n = new int;
- // Should warn that *n is uninitialized.
- if (*n) { // no-warning
+ if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
delete n;
return 0;
}
@@ -193,6 +188,10 @@ int testNoInitialization() {
return 1;
}
+//===----------------------------------------------------------------------===//
+// Incorrectly-modelled behavior.
+//===----------------------------------------------------------------------===//
+
int testNoInitializationPlacement() {
int n;
new (&n) int;
diff --git a/clang/test/Analysis/placement-new.cpp b/clang/test/Analysis/placement-new.cpp
index f3ecd7ebf2a77..766b11cf84c5f 100644
--- a/clang/test/Analysis/placement-new.cpp
+++ b/clang/test/Analysis/placement-new.cpp
@@ -112,6 +112,10 @@ void f() {
namespace testHeapAllocatedBuffer {
void g2() {
char *buf = new char[2]; // expected-note {{'buf' initialized here}}
+ // FIXME: The message is misleading -- we should
+ // state that a pointer to an uninitialized value
+ // is stored.
+ // expected-note at -4{{Storing uninitialized value}}
long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
(void)lp;
}
diff --git a/clang/test/Analysis/reinterpret-cast.cpp b/clang/test/Analysis/reinterpret-cast.cpp
index 7b8c2f3ee9a48..b71d588505fc8 100644
--- a/clang/test/Analysis/reinterpret-cast.cpp
+++ b/clang/test/Analysis/reinterpret-cast.cpp
@@ -77,15 +77,11 @@ namespace PR15345 {
class Derived : public Base {};
void test() {
- Derived* p;
- *(reinterpret_cast<void**>(&p)) = new C;
- p->f();
-
- // We should still be able to do some reasoning about bindings.
- p->x = 42;
- clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+ Derived* p;
+ *(reinterpret_cast<void**>(&p)) = new C;
+ p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
};
-}
+} // namespace PR15345
int trackpointer_std_addressof() {
int x;
diff --git a/clang/test/Analysis/uninit-const.cpp b/clang/test/Analysis/uninit-const.cpp
index 69c2c007aee4b..3ffcda1294abb 100644
--- a/clang/test/Analysis/uninit-const.cpp
+++ b/clang/test/Analysis/uninit-const.cpp
@@ -21,9 +21,11 @@ void doStuff_uninit(const int *u);
int f10(void) {
int *ptr;
-
- ptr = new int; //
- if(*ptr) {
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+ ptr = new int; // expected-note{{Storing uninitialized value}}
+ if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+ // expected-note at -1 {{Branch condition evaluates to a garbage value}}
doStuff4(*ptr);
}
delete ptr;
@@ -32,10 +34,12 @@ int f10(void) {
int f9(void) {
int *ptr;
-
- ptr = new int; //
-
- doStuff_uninit(ptr); // no warning
+ // FIXME: The message is misleading -- we should state that
+ // a pointer to an uninitialized value is stored.
+ ptr = new int; // expected-note{{Storing uninitialized value}}
+ // expected-note at -1{{Value assigned to 'ptr'}}
+ doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+ // expected-note at -1{{1st function call argument is a pointer to uninitialized value}}
delete ptr;
return 0;
}
More information about the cfe-commits
mailing list