Model Constructor corresponding to a new call

[karthikthecool]

Summary

Hi Jordan,
This patch is not yet complete and I'm not completly sure about this patch yet, as in if this is the correct way to model allocator call.
I would like to get a few inputs if i'm in the right direction.

Since we have modelled Allocator in CFG i'm now trying to plugin in the same into SA Core.
I'm a bit confused on what part of VisitCXXNewExpr will go into VisitCXXNewAllocatorCall and if that is required?

In this patch i have just called the relevent allocator function in VisitCXXNewAllocatorCall and proceeded.
In VisitCXXConstructExpr i check if this constructor was call due to a call to new in which case i use the CXXNewExpr to conjure a symbol and use the memregion returned to call the constructor.
Later when VisitCXXNewExpr the same region is returned for the CXXNewExpr and i continue with other initialization.

This seem to work and constructor is now getting inlined and relevent warnings are now being detected but i'm not sure if this approach is correct.

Could you guide me if we can follow this approach? If not how exactly to model VisitCXXNewAllocatorCall call to reuse the allocated memregion in VisitCXXConstructExpr?

Any inputs would be greatly appreciated.

Thanks
Karthik Bhat

[jordan_rose]

Let's not worry about the constructor for now. I imagine it will look something like what you've done (I just skimmed it), but for now let's just get the call happening in the right place. You can always use something like clang_analyzer_checkInlined(true) to make sure it's actually happening.

I appreciate that it looks fairly simple to get that part working. :-)

Later, I think we'll want the default region stuff (the "standard global new" model) to appear in VisitCXXAllocatorCall, or even to be in a checker (which would mean using evalCall instead of defaultEvalCall), so that by the time we get to the constructor we can rely on always having a region.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

375

Don't forget to change this stack trace message. :-)

[karthikthecool]

Hi Jordan,
I suppose you wanted me to inline the allocator call during a call to new first. This patch inlines the allocator call but i'm still facing few problems.
If i code as -

void* operator new(size_t num)
{
  return 0;
}

int main() {
  int* i = new int(1);
  *i = 1;
  delete i;
  return 0;
}

In the above code we are not able to detect the null deref at *i = 1; . I suppose that is because we are conjuring a new symbol in VisitCXXNewExpr which is being used as output of new expression. My doubt was how do i make sure that the VisitCXXNewExpr uses the output of VisitCXXNewAllocatorCall?
I tried something like -

1. conjure a symbol in VisitCXXNewAllocatorCall.
2. Bind it to conjured symbol to CXXNewExpr
3. Call getSVal in VisitCXXNewExpr with the CXXNewExpr and location context to get back the conjured symbol.

but unfortunetly in VisitCXXNewExpr when i call getSVal with CXXNewExpr and LocationContext i get the result as Unknown instead of the symbol conjured in VisitCXXNewAllocatorCall.
How do i make sure that VisitCXXNewExpr uses the result of VisitCXXNewAllocatorCall? It would be great if you could guide me here.

Thanks
Karthik Bhat

[jordan_rose]

I think that's basically the strategy we'll want to use, but I don't even think we should worry about that right now. Incremental development is a good thing. My guess is that it's not working because the CXXNewExpr isn't considered live after the allocator CFG node, so we'll have to go tweak the LiveVariables analysis.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

667–669

I forgot about this. Let's put this under an analyzer-config flag and have it off by default, because it's an important bit of false positive prevention. (Sorry!)

We don't need to be able to inline deallocators to turn this on, just have them in the CFG. But inlining them probably isn't much work after that.

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

                           const Stmt *S, bool IsBaseDtor,
                           ExplodedNode *Pred, ExplodedNodeSet &Dst);
 
+  void VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+                                ExplodedNode *Pred,
+                                ExplodedNodeSet &Dst);
+
   void VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                        ExplodedNodeSet &Dst);
 

lib/StaticAnalyzer/Core/ExprEngine.cpp

 
 void ExprEngine::ProcessNewAllocator(const CXXNewExpr *NE,
                                      ExplodedNode *Pred) {
-  //TODO: Implement VisitCXXNewAllocatorCall
   ExplodedNodeSet Dst;
-  NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
-  const LocationContext *LCtx = Pred->getLocationContext();
-  PostImplicitCall PP(NE->getOperatorNew(), NE->getLocStart(), LCtx);
-  Bldr.generateNode(PP, Pred->getState(), Pred);
+  VisitCXXNewAllocatorCall(NE, Pred, Dst);
   Engine.enqueue(Dst, currBldrCtx->getBlock(), currStmtIdx);
 }
 

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

                                              *Call, *this);
 }
 
+void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
+                                          ExplodedNode *Pred,
+                                          ExplodedNodeSet &Dst) {
+  ProgramStateRef State = Pred->getState();
+  const LocationContext *LCtx = Pred->getLocationContext();
+  CallEventManager &CEMgr = getStateManager().getCallEventManager();
+  CallEventRef<CXXAllocatorCall> Call =
+    CEMgr.getCXXAllocatorCall(CNE, State, LCtx);
+  PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
+                                Call->getSourceRange().getBegin(),
+                                "Error evaluating New Allocator Call");
+
+  ExplodedNodeSet DstPreCall;
+  getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
+                                            *Call, *this);
+
+  ExplodedNodeSet DstInvalidated;
+  StmtNodeBuilder Bldr(DstPreCall, DstInvalidated, *currBldrCtx);
+  for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = DstPreCall.end();
+       I != E; ++I)
+    defaultEvalCall(Bldr, *I, *Call);
+  getCheckerManager().runCheckersForPostCall(Dst, DstInvalidated,
+                                             *Call, *this);
+}
+
+
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

     break;
   }
   case CE_CXXAllocator:
-    // Do not inline allocators until we model deallocators.
-    // This is unfortunate, but basically necessary for smart pointers and such.
-    return CIP_DisallowedAlways;
+    break;
   case CE_ObjCMessage:
     if (!Opts.mayInlineObjCMethod())
       return CIP_DisallowedAlways;

test/Analysis/NewDelete-custom.cpp

 }
 #ifdef LEAKS
 // expected-warning@-2{{Potential leak of memory pointed to by 'c3'}}
+// expected-warning@-4{{Potential memory leak}}
 #endif
 
 void testOpNewArray() {

 }
 #ifdef LEAKS
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
+// expected-warning@-4{{Potential memory leak}}
 #endif
 
 

 }
 #ifdef LEAKS
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
+// expected-warning@-4{{Potential memory leak}}
 #endif
 
 

test/Analysis/inline.cpp

 // This is the standard placement new.
 inline void* operator new(size_t, void* __p) throw()
 {
+  clang_analyzer_checkInlined(true);// expected-warning{{TRUE}}
   return __p;
 }