-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[analyzer] Improve handling of placement new in PointerArith
#155855
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
[analyzer] Improve handling of placement new in PointerArith
#155855
Conversation
Flag the region as if it was `reinterpret_cast`-ed
@llvm/pr-subscribers-clang Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) ChangesThis pull improves the handling of placement new in False Positives Buffer buffer;
int* array = new (&buffer) int[10];
++array; // there should be no warning The code above should flag the memory region This is no-op if the placement new is opaque. False Negatives Buffer buffer;
int* array = new (&buffer) int;
++array; // there should be a warning In this case, there is an implicit cast to There are still some limitations, of course. For starters, if a single CPP-6868 Full diff: https://github.com/llvm/llvm-project/pull/155855.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
index 30e01e73eb18f..c4a968950e8bc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
@@ -74,6 +74,26 @@ class PointerArithChecker
REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, const MemRegion *, AllocKind)
+namespace {
+
+bool isArrayPlacementNew(const CXXNewExpr *NE) {
+ return NE->isArray() && NE->getNumPlacementArgs() > 0;
+}
+
+ProgramStateRef markSuperRegionReinterpreted(ProgramStateRef State,
+ const MemRegion *Region) {
+ while (const auto *BaseRegion = dyn_cast<CXXBaseObjectRegion>(Region)) {
+ Region = BaseRegion->getSuperRegion();
+ }
+ if (const auto *ElemRegion = dyn_cast<ElementRegion>(Region)) {
+ State = State->set<RegionState>(ElemRegion->getSuperRegion(),
+ AllocKind::Reinterpreted);
+ }
+ return State;
+}
+
+} // namespace
+
void PointerArithChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
// TODO: intentional leak. Some information is garbage collected too early,
@@ -244,13 +264,23 @@ void PointerArithChecker::checkPostStmt(const CXXNewExpr *NE,
const MemRegion *Region = AllocedVal.getAsRegion();
if (!Region)
return;
+
+ // For array placement-new, mark the original region as reinterpreted
+ if (isArrayPlacementNew(NE)) {
+ State = markSuperRegionReinterpreted(State, Region);
+ }
+
State = State->set<RegionState>(Region, Kind);
C.addTransition(State);
}
void PointerArithChecker::checkPostStmt(const CastExpr *CE,
CheckerContext &C) const {
- if (CE->getCastKind() != CastKind::CK_BitCast)
+ // Casts to `void*` happen, for instance, on placement new calls.
+ // We consider `void*` not to erase the type information about the underlying
+ // region.
+ if (CE->getCastKind() != CastKind::CK_BitCast ||
+ CE->getType()->isVoidPointerType())
return;
const Expr *CastedExpr = CE->getSubExpr();
diff --git a/clang/test/Analysis/PR24184.cpp b/clang/test/Analysis/PR24184.cpp
index 172d3e7e33864..c73271b87de98 100644
--- a/clang/test/Analysis/PR24184.cpp
+++ b/clang/test/Analysis/PR24184.cpp
@@ -56,7 +56,7 @@ void fn1_1(void *p1, const void *p2) { p1 != p2; }
void fn2_1(uint32_t *p1, unsigned char *p2, uint32_t p3) {
unsigned i = 0;
for (0; i < p3; i++)
- fn1_1(p1 + i, p2 + i * 0);
+ fn1_1(p1 + i, p2 + i * 0); // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
}
struct A_1 {
diff --git a/clang/test/Analysis/ptr-arith.cpp b/clang/test/Analysis/ptr-arith.cpp
index ec1c75c0c4063..39bb239f51ca7 100644
--- a/clang/test/Analysis/ptr-arith.cpp
+++ b/clang/test/Analysis/ptr-arith.cpp
@@ -165,3 +165,124 @@ void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
unsigned long ptr_arithmetic(void *p) {
return __builtin_bit_cast(unsigned long, p) + 1; // no-crash
}
+
+
+void escape(int*);
+
+struct AllocOpaqueFlag {};
+
+void* operator new(unsigned long, void *ptr) noexcept { return ptr; }
+void* operator new(unsigned long, void *ptr, AllocOpaqueFlag const&) noexcept { return ptr; }
+
+void* operator new[](unsigned long, void* ptr) noexcept { return ptr; }
+void* operator new[](unsigned long, void* ptr, AllocOpaqueFlag const&);
+
+struct Buffer {
+ char buf[100];
+ int padding;
+};
+
+void checkPlacementNewArryInObject() {
+ Buffer buffer;
+ int* array = new (&buffer) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInObjectOpaque() {
+ Buffer buffer;
+ int* array = new (&buffer, AllocOpaqueFlag{}) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInArray() {
+ char buffer[100];
+ int* array = new (buffer) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInArrayOpaque() {
+ char buffer[100];
+ int* array = new (buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewObjectInObject() {
+ Buffer buffer;
+ int* array = new (&buffer) int;
+ escape(array);
+ ++array; // expected-warning{{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
+ (void)*array;
+}
+
+void checkPlacementNewObjectInObjectOpaque() {
+ Buffer buffer;
+ int* array = new (&buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // expected-warning{{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
+ (void)*array;
+}
+
+void checkPlacementNewObjectInArray() {
+ char buffer[sizeof(int)];
+ int* array = new (buffer) int;
+ escape(array);
+ ++array; // no warning (FN)
+ (void)*array;
+}
+
+void checkPlacementNewObjectInArrayOpaque() {
+ char buffer[sizeof(int)];
+ int* array = new (buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // no warning (FN)
+ (void)*array;
+}
+
+void checkPlacementNewSlices() {
+ const int N = 10;
+ char buffer[sizeof(int) * N] = {0};
+ int *start = new (buffer) int{0};
+ for (int i = 1; i < N; i++) {
+ auto *ptr = new int(buffer[i * sizeof(int)]);
+ *ptr = i;
+ }
+ ++start; // no warning
+ (void*)start;
+}
+
+class BumpAlloc {
+ char *buffer;
+ char *offset;
+public:
+ BumpAlloc(int n): buffer(new char[n]), offset{buffer} {}
+ ~BumpAlloc() {delete [] buffer;}
+
+ void* alloc(unsigned long size) {
+ auto* ptr = offset;
+ offset += size;
+ return ptr;
+ }
+};
+
+void* operator new(unsigned long size, BumpAlloc &ba) {
+ return ba.alloc(size);
+}
+
+void checkPlacementSlab() {
+ BumpAlloc bump{10};
+
+ int *ptr = new (bump) int{0};
+ ++ptr; // no warning
+ (void)*ptr;
+
+ BumpAlloc *why = ≎
+ ++why; // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous [alpha.core.PointerArithm]}}
+}
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource) ChangesThis pull improves the handling of placement new in False Positives Buffer buffer;
int* array = new (&buffer) int[10];
++array; // there should be no warning The code above should flag the memory region This is no-op if the placement new is opaque. False Negatives Buffer buffer;
int* array = new (&buffer) int;
++array; // there should be a warning In this case, there is an implicit cast to There are still some limitations, of course. For starters, if a single CPP-6868 Full diff: https://github.com/llvm/llvm-project/pull/155855.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
index 30e01e73eb18f..c4a968950e8bc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
@@ -74,6 +74,26 @@ class PointerArithChecker
REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, const MemRegion *, AllocKind)
+namespace {
+
+bool isArrayPlacementNew(const CXXNewExpr *NE) {
+ return NE->isArray() && NE->getNumPlacementArgs() > 0;
+}
+
+ProgramStateRef markSuperRegionReinterpreted(ProgramStateRef State,
+ const MemRegion *Region) {
+ while (const auto *BaseRegion = dyn_cast<CXXBaseObjectRegion>(Region)) {
+ Region = BaseRegion->getSuperRegion();
+ }
+ if (const auto *ElemRegion = dyn_cast<ElementRegion>(Region)) {
+ State = State->set<RegionState>(ElemRegion->getSuperRegion(),
+ AllocKind::Reinterpreted);
+ }
+ return State;
+}
+
+} // namespace
+
void PointerArithChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
// TODO: intentional leak. Some information is garbage collected too early,
@@ -244,13 +264,23 @@ void PointerArithChecker::checkPostStmt(const CXXNewExpr *NE,
const MemRegion *Region = AllocedVal.getAsRegion();
if (!Region)
return;
+
+ // For array placement-new, mark the original region as reinterpreted
+ if (isArrayPlacementNew(NE)) {
+ State = markSuperRegionReinterpreted(State, Region);
+ }
+
State = State->set<RegionState>(Region, Kind);
C.addTransition(State);
}
void PointerArithChecker::checkPostStmt(const CastExpr *CE,
CheckerContext &C) const {
- if (CE->getCastKind() != CastKind::CK_BitCast)
+ // Casts to `void*` happen, for instance, on placement new calls.
+ // We consider `void*` not to erase the type information about the underlying
+ // region.
+ if (CE->getCastKind() != CastKind::CK_BitCast ||
+ CE->getType()->isVoidPointerType())
return;
const Expr *CastedExpr = CE->getSubExpr();
diff --git a/clang/test/Analysis/PR24184.cpp b/clang/test/Analysis/PR24184.cpp
index 172d3e7e33864..c73271b87de98 100644
--- a/clang/test/Analysis/PR24184.cpp
+++ b/clang/test/Analysis/PR24184.cpp
@@ -56,7 +56,7 @@ void fn1_1(void *p1, const void *p2) { p1 != p2; }
void fn2_1(uint32_t *p1, unsigned char *p2, uint32_t p3) {
unsigned i = 0;
for (0; i < p3; i++)
- fn1_1(p1 + i, p2 + i * 0);
+ fn1_1(p1 + i, p2 + i * 0); // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
}
struct A_1 {
diff --git a/clang/test/Analysis/ptr-arith.cpp b/clang/test/Analysis/ptr-arith.cpp
index ec1c75c0c4063..39bb239f51ca7 100644
--- a/clang/test/Analysis/ptr-arith.cpp
+++ b/clang/test/Analysis/ptr-arith.cpp
@@ -165,3 +165,124 @@ void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
unsigned long ptr_arithmetic(void *p) {
return __builtin_bit_cast(unsigned long, p) + 1; // no-crash
}
+
+
+void escape(int*);
+
+struct AllocOpaqueFlag {};
+
+void* operator new(unsigned long, void *ptr) noexcept { return ptr; }
+void* operator new(unsigned long, void *ptr, AllocOpaqueFlag const&) noexcept { return ptr; }
+
+void* operator new[](unsigned long, void* ptr) noexcept { return ptr; }
+void* operator new[](unsigned long, void* ptr, AllocOpaqueFlag const&);
+
+struct Buffer {
+ char buf[100];
+ int padding;
+};
+
+void checkPlacementNewArryInObject() {
+ Buffer buffer;
+ int* array = new (&buffer) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInObjectOpaque() {
+ Buffer buffer;
+ int* array = new (&buffer, AllocOpaqueFlag{}) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInArray() {
+ char buffer[100];
+ int* array = new (buffer) int[10];
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewArrayInArrayOpaque() {
+ char buffer[100];
+ int* array = new (buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // no warning
+ (void)*array;
+}
+
+void checkPlacementNewObjectInObject() {
+ Buffer buffer;
+ int* array = new (&buffer) int;
+ escape(array);
+ ++array; // expected-warning{{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
+ (void)*array;
+}
+
+void checkPlacementNewObjectInObjectOpaque() {
+ Buffer buffer;
+ int* array = new (&buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // expected-warning{{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
+ (void)*array;
+}
+
+void checkPlacementNewObjectInArray() {
+ char buffer[sizeof(int)];
+ int* array = new (buffer) int;
+ escape(array);
+ ++array; // no warning (FN)
+ (void)*array;
+}
+
+void checkPlacementNewObjectInArrayOpaque() {
+ char buffer[sizeof(int)];
+ int* array = new (buffer, AllocOpaqueFlag{}) int;
+ escape(array);
+ ++array; // no warning (FN)
+ (void)*array;
+}
+
+void checkPlacementNewSlices() {
+ const int N = 10;
+ char buffer[sizeof(int) * N] = {0};
+ int *start = new (buffer) int{0};
+ for (int i = 1; i < N; i++) {
+ auto *ptr = new int(buffer[i * sizeof(int)]);
+ *ptr = i;
+ }
+ ++start; // no warning
+ (void*)start;
+}
+
+class BumpAlloc {
+ char *buffer;
+ char *offset;
+public:
+ BumpAlloc(int n): buffer(new char[n]), offset{buffer} {}
+ ~BumpAlloc() {delete [] buffer;}
+
+ void* alloc(unsigned long size) {
+ auto* ptr = offset;
+ offset += size;
+ return ptr;
+ }
+};
+
+void* operator new(unsigned long size, BumpAlloc &ba) {
+ return ba.alloc(size);
+}
+
+void checkPlacementSlab() {
+ BumpAlloc bump{10};
+
+ int *ptr = new (bump) int{0};
+ ++ptr; // no warning
+ (void)*ptr;
+
+ BumpAlloc *why = ≎
+ ++why; // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous [alpha.core.PointerArithm]}}
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sweet. I did the first round, I'll let others to finish the rest while I'm on vacation
clang/test/Analysis/ptr-arith.cpp
Outdated
|
||
void checkPlacementNewArryInObject() { | ||
Buffer buffer; | ||
int* array = new (&buffer) int[10]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I guess, alignment requirements are not checked. Thats sad, but We could extend this checker later.
This is a common mistake I think.
This change itself seems to be correct, but I have doubts about the overall viability of the implementation of this In particular I don't like that it maintains its independent model about things that are modeled more accurately by other checkers. For example it recognizes a few allocation functions instead of relying on Based on this impression I was planning to discard the existing implementation and reimplement the functionality of this @alejandro-alvarez-sonarsource @steakhal What are your high-level plans for this checker? Is this patch just an ad-hoc bugfix, or part of a more systemic cleanup? |
This is a great point. I don't have the context if this they could be merged in a meaningful way, but we should think about it.
What you say makes sense. Ideally, |
Sorry for the delay in answering, I was waiting for @steakhal to be back to double check with him, but he already answered with what I had anyways in mind. |
As this commit is already written, I don't have any reason against merging it. (I didn't do a through review yet, but I can happily do so if you want.) My concerns about the long-term future of this alpha checker are only relevant for additional improvements: before investing additional work into ad-hoc improvements, we should discuss the long-term viability of this checker because it would be unfortunate to waste work in a dead end checker. (I'm not sure that this is a dead end; I just have suspicions about it.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have approved this fix downstream
This pull improves the handling of placement new in
PointerArith
, fixing one family of false positives, and one of negatives:False Positives
The code above should flag the memory region
buffer
as reinterpreted, very much asreinterpret_cast
would do. Note that in this particular case the placement new is inlined so the engine can track that*array
points to the same region asbuffer
.This is no-op if the placement new is opaque.
False Negatives
In this case, there is an implicit cast to
void*
when calling placement new. The memory region was marked as reinterpreted, and therefore later pointer arithmetic will not raise. I have added a condition to not consider a cast tovoid*
as a reinterpretation, as an array of voids does not make much sense.There are still some limitations, of course. For starters, if a single
int
is created in place of an array ofunsigned char
of exactly the same size, it will still be considered as an array. A convoluted example to make the point that I think it makes sense not to raise in this situation is in the testcheckPlacementNewSlices
.CPP-6868