Skip to content

Commit 80d4e24

Browse files
[analyzer] Improve handling of placement new in PointerArith (#155855)
This pull improves the handling of placement new in`PointerArith`, fixing one family of false positives, and one of negatives: ### False Positives ```cpp Buffer buffer; int* array = new (&buffer) int[10]; ++array; // there should be no warning ``` The code above should flag the memory region `buffer` as reinterpreted, very much as `reinterpret_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 as `buffer`. This is no-op if the placement new is opaque. ### False Negatives ```cpp Buffer buffer; int* array = new (&buffer) int; ++array; // there should be a warning ``` 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 to `void*` 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 of `unsigned 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 test `checkPlacementNewSlices`. CPP-6868
1 parent 9b24a8a commit 80d4e24

File tree

3 files changed

+138
-2
lines changed

3 files changed

+138
-2
lines changed

clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,22 @@ class PointerArithChecker
7474

7575
REGISTER_MAP_WITH_PROGRAMSTATE(RegionState, const MemRegion *, AllocKind)
7676

77+
static bool isArrayPlacementNew(const CXXNewExpr *NE) {
78+
return NE->isArray() && NE->getNumPlacementArgs() > 0;
79+
}
80+
81+
static ProgramStateRef markSuperRegionReinterpreted(ProgramStateRef State,
82+
const MemRegion *Region) {
83+
while (const auto *BaseRegion = dyn_cast<CXXBaseObjectRegion>(Region)) {
84+
Region = BaseRegion->getSuperRegion();
85+
}
86+
if (const auto *ElemRegion = dyn_cast<ElementRegion>(Region)) {
87+
State = State->set<RegionState>(ElemRegion->getSuperRegion(),
88+
AllocKind::Reinterpreted);
89+
}
90+
return State;
91+
}
92+
7793
void PointerArithChecker::checkDeadSymbols(SymbolReaper &SR,
7894
CheckerContext &C) const {
7995
// TODO: intentional leak. Some information is garbage collected too early,
@@ -244,13 +260,23 @@ void PointerArithChecker::checkPostStmt(const CXXNewExpr *NE,
244260
const MemRegion *Region = AllocedVal.getAsRegion();
245261
if (!Region)
246262
return;
263+
264+
// For array placement-new, mark the original region as reinterpreted
265+
if (isArrayPlacementNew(NE)) {
266+
State = markSuperRegionReinterpreted(State, Region);
267+
}
268+
247269
State = State->set<RegionState>(Region, Kind);
248270
C.addTransition(State);
249271
}
250272

251273
void PointerArithChecker::checkPostStmt(const CastExpr *CE,
252274
CheckerContext &C) const {
253-
if (CE->getCastKind() != CastKind::CK_BitCast)
275+
// Casts to `void*` happen, for instance, on placement new calls.
276+
// We consider `void*` not to erase the type information about the underlying
277+
// region.
278+
if (CE->getCastKind() != CastKind::CK_BitCast ||
279+
CE->getType()->isVoidPointerType())
254280
return;
255281

256282
const Expr *CastedExpr = CE->getSubExpr();

clang/test/Analysis/PR24184.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ void fn1_1(void *p1, const void *p2) { p1 != p2; }
5656
void fn2_1(uint32_t *p1, unsigned char *p2, uint32_t p3) {
5757
unsigned i = 0;
5858
for (0; i < p3; i++)
59-
fn1_1(p1 + i, p2 + i * 0);
59+
fn1_1(p1 + i, p2 + i * 0); // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
6060
}
6161

6262
struct A_1 {

clang/test/Analysis/ptr-arith.cpp

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,3 +165,113 @@ void LValueToRValueBitCast_dumps(void *p, char (*array)[8]) {
165165
unsigned long ptr_arithmetic(void *p) {
166166
return __builtin_bit_cast(unsigned long, p) + 1; // no-crash
167167
}
168+
169+
struct AllocOpaqueFlag {};
170+
171+
void *operator new(unsigned long, void *ptr) noexcept { return ptr; }
172+
void *operator new(unsigned long, void *ptr, AllocOpaqueFlag const &) noexcept;
173+
174+
void *operator new[](unsigned long, void *ptr) noexcept { return ptr; }
175+
void *operator new[](unsigned long, void *ptr,
176+
AllocOpaqueFlag const &) noexcept;
177+
178+
struct Buffer {
179+
char buf[100];
180+
int padding;
181+
};
182+
183+
void checkPlacementNewArryInObject() {
184+
Buffer buffer;
185+
int *array = new (&buffer) int[10];
186+
++array; // no warning
187+
(void)*array;
188+
}
189+
190+
void checkPlacementNewArrayInObjectOpaque() {
191+
Buffer buffer;
192+
int *array = new (&buffer, AllocOpaqueFlag{}) int[10];
193+
++array; // no warning
194+
(void)*array;
195+
}
196+
197+
void checkPlacementNewArrayInArray() {
198+
char buffer[100];
199+
int *array = new (buffer) int[10];
200+
++array; // no warning
201+
(void)*array;
202+
}
203+
204+
void checkPlacementNewArrayInArrayOpaque() {
205+
char buffer[100];
206+
int *array = new (buffer, AllocOpaqueFlag{}) int;
207+
++array; // no warning
208+
(void)*array;
209+
}
210+
211+
void checkPlacementNewObjectInObject() {
212+
Buffer buffer;
213+
int *array = new (&buffer) int;
214+
++array; // expected-warning{{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous}}
215+
(void)*array;
216+
}
217+
218+
void checkPlacementNewObjectInObjectOpaque() {
219+
Buffer buffer;
220+
int *array = new (&buffer, AllocOpaqueFlag{}) int;
221+
++array; // no warning (allocator is opaque)
222+
(void)*array;
223+
}
224+
225+
void checkPlacementNewObjectInArray() {
226+
char buffer[sizeof(int)];
227+
int *array = new (buffer) int;
228+
++array; // no warning (FN)
229+
(void)*array;
230+
}
231+
232+
void checkPlacementNewObjectInArrayOpaque() {
233+
char buffer[sizeof(int)];
234+
int *array = new (buffer, AllocOpaqueFlag{}) int;
235+
++array; // no warning (FN)
236+
(void)*array;
237+
}
238+
239+
void checkPlacementNewSlices() {
240+
const int N = 10;
241+
char buffer[sizeof(int) * N] = {0};
242+
int *start = new (buffer) int{0};
243+
for (int i = 1; i < N; i++) {
244+
auto *ptr = new int(buffer[i * sizeof(int)]);
245+
*ptr = i;
246+
}
247+
++start; // no warning
248+
(void *)start;
249+
}
250+
251+
class BumpAlloc {
252+
char *buffer;
253+
char *offset;
254+
255+
public:
256+
BumpAlloc(int n) : buffer(new char[n]), offset{buffer} {}
257+
~BumpAlloc() { delete[] buffer; }
258+
259+
void *alloc(unsigned long size) {
260+
auto *ptr = offset;
261+
offset += size;
262+
return ptr;
263+
}
264+
};
265+
266+
void *operator new(unsigned long size, BumpAlloc &ba) { return ba.alloc(size); }
267+
268+
void checkPlacementSlab() {
269+
BumpAlloc bump{10};
270+
271+
int *ptr = new (bump) int{0};
272+
++ptr; // no warning
273+
(void)*ptr;
274+
275+
BumpAlloc *why = &bump;
276+
++why; // expected-warning {{Pointer arithmetic on non-array variables relies on memory layout, which is dangerous [alpha.core.PointerArithm]}}
277+
}

0 commit comments

Comments
 (0)