Hi!
Preface
Observe the following code snippets:
$ cat dynamic-uninit.c
#include <stdlib.h>
void f() {
int *i = malloc(sizeof(int));
int j = *i;
(void)j;
free(i);
}
We forgot to initialize *i before reading it, and the analyzer catches this error with ease:
$ clang --analyze -c dynamic-uninit.c -Xclang -analyzer-output=text
dynamic-uninit.c:4:3: warning: Assigned value is garbage or undefined [core.uninitialized.Assign]
int j = *i;
^ ~~
dynamic-uninit.c:3:12: note: Storing uninitialized value
int *i = malloc(sizeof(int));
^~~~~~~~~~~~~~~~~~~
dynamic-uninit.c:4:3: note: Assigned value is garbage or undefined
int j = *i;
^ ~~
1 warning generated.
$
And the same in C++:
$ cat dynamic-uninit.cpp
void f() {
int *i = new int;
int j = *i;
(void)j;
delete i;
}
And the analyzer completely misses this:
$ clang++ --analyze -c dynamic-uninit.cpp -Xclang -analyzer-output=text
$
Previous discussions
So, with the above problem being soooo trivial, its natural to think that the problem must be a lot more difficult than it appears to be, if after one and a half decades the analyzer still misses it. I did some digging, looking for clues in MallocChecker’s history, and discussions regarding CXXNew.
[⚙ D40560 [analyzer] Get construction into `operator new` running in simple cases.]: [analyzer] Get construction into operator new running in simple cases.
An interesting comment in one of the testcases (among a lot of other very interesting discussion in the patch!):
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
delete s;
}
[⚙ D2646 Inline allocator call when c++-allocator-inlining is specified as true]: Inline allocator call when c+±allocator-inlining is specified as true
The patch that added this clue:
void ExprEngine::ProcessNewAllocator(const CXXNewExpr *NE,
ExplodedNode *Pred) {
ExplodedNodeSet Dst;
AnalysisManager &AMgr = getAnalysisManager();
AnalyzerOptions &Opts = AMgr.options;
// TODO: We're not evaluating allocators for all cases just yet as
// we're not handling the return value correctly, which causes false
// positives when the alpha.cplusplus.NewDeleteLeaks check is on.
if (Opts.MayInlineCXXAllocator)
VisitCXXNewAllocatorCall(NE, Pred, Dst);
else {
NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
const LocationContext *LCtx = Pred->getLocationContext();
PostImplicitCall PP(NE->getOperatorNew(), NE->getBeginLoc(), LCtx);
Bldr.generateNode(PP, Pred->getState(), Pred);
}
Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
}
[⚙ D2616 Model Constructor corresponding to a new call]: Model Constructor corresponding to a new call
Multi-part mail about a seemingly large change change to new/delete modeling, but upon inspecting again, seems like it only affect leaks:
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 1
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 2
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 3
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 4
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 5
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 6
[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators, Part 7
I didn’t find much about why modeling this is still missing to this day.
Question
Is there any discussion I may have missed? Is there trap I glanced over that prevented folks from fixing this? Because it doesn’t that hard at first glance.
Cheers,
Kristóf