[analyzer] Why do we suck at modeling C++ dynamic memory?

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

// FIXME: This should warn, but MallocChecker doesn't default-bind regions
// returned by standard operator new to garbage.

Hmm I don’t recall any conspiracy in this area. I suspect that this was simply never implemented.

It doesn’t have to be in the checker, I think most of the default operator new is modeled in ExprEngine directly, but I’m not opposed to moving everything to checkers in general.