[analyzer] Add a new checker callback, check::NewAllocator.

[NoQ]

Summary

This patch is roughly based on the discussion we've had in http://lists.llvm.org/pipermail/cfe-dev/2017-December/056314.html about how our support for C++ operator new() should provide useful callbacks to the checkers. As in, this patch tries to do a minimal necessary thing while avoiding all decisions that we didn't have a consensus over.

This patch also absorbs D36708.

In the discussion, we've been mentioning PreAllocator/PostAllocator callbacks. However, we already sort of have them: they are PreCall/PostCall callbacks for the allocator call, which are readily available in the c++-allocator-inlining mode.

This patch also causes no change in the behavior of the pre/post CXXNewExpr callbacks. They remain broken in the c++-allocator-inlining mode for now.

Missing in the current system of callbacks, however, is a callback that would be called before construction while providing the casted return value of operator new (in the sense of D41250: operator new returns void *, but we need to have C *). The user can subscribe on the allocator's PostCall and perform the cast manually, but that's an unpleasant thing to do. So it sounds like a good idea to provide a callback that would contain the relevant information.

A similar "Pre" callback doesn't seem necessary - there's not much additional information we can provide compared to PreCall.

In this patch, I add the callback (with a lot of code that is the usual boilerplate around the callback, though there's still a questionable TODO in CheckNewAllocatorContext::runChecker()) and modify MallocChecker to make use of it when c++-allocator-inlining mode is turned on. Specifically:

• If the allocator (operator new) call is evaluated conservatively, checkNewAllocator() provides the return value of the allocator that is semantically equivalent to the value in checkPostStmt<CXXNewExpr>() that we've been using earlier. Without the cast, the value is different and the checker crashes.
• If the allocator call is inlined, the checker does not treat it as an allocator, but instead as a simple inlined function. That is, it does not start tracking the return value here, and in particular avoids crashes, though it may continue tracking the value if it is already being tracked (see the new-ctor-malloc.cpp test). This policy has already been there because previously we could inline the operator when it was written without operator syntax - see tests in test/Analysis/NewDelete-custom.cpp. Whether this policy was correct or not is a matter of discussion. For now it means that in c++-allocator-inlining mode we'd have new negatives (i.e. lost positives) on code that uses custom allocators which do not boil down to simple malloc.

The std::nothrow_t tests in NewDelete-custom.cpp are affected in a surprising manner. I'd fix them in a follow-up commit.

[NoQ]

• Actually call the new callback when the allocator call is inlined.
• Update checker documentation :)

[NoQ]

TODOs for the future commits:

• Constructor shouldn't cause pointer escape of the newly allocated pointer immediately after the NewAllocator callback, otherwise we ain't gonna find no leaks. Without c++-allocator-inlining, we only started tracking the pointer after the constructor, so this wasn't a problem. Should this be a checker-side workaround, or we should globally disable pointer escape (but not invalidation!) for this-pointer and constructor?
• Refactor other checkers that relied on PostStmt<CXXNewExpr>, namely DynamicTypePropagation and PointerArithmeticChecker, to use the new callback. Then see if we can remove the callback entirely, and probably re-introduce later depending on where the discussion goes.

[xazax.hun]

Maybe debug.AnalysisOrder could be used to test the callback order explicitly. This way the test could also serve as a documentation for the callback order.

[NoQ]

In D41406#960824, @xazax.hun wrote:

Maybe debug.AnalysisOrder could be used to test the callback order explicitly. This way the test could also serve as a documentation for the callback order.

Yep, totally, will do.

[dcoughlin]

Looks good to me with some minor nits inside (and also a request to consider factoring out common code).

include/clang/StaticAnalyzer/Core/CheckerManager.h

311

Does 'wasInlined' really need to have a default argument? It looks like there are only two callers. (Also the capitalization is inconsistent).

lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

138

I find the following confusing: "Post-call for the allocator is called before step (1)." Did you mean "after" step one?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

298

It is probably worth explaining what RetVal is and what it means if it is None in the doxygen.

328

Same comment about documenting RetVal here.

968

Nit: Capitalization of "retVal".

1293

Capitalization, again.

1308

I think it is worth a comment here about why we expect this assert succeed: the value will always be the result of a malloc function or an un-inlined call to the new allocator (if I understand correctly).

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

349

Is it possible/desirable to factor out this logic (running the PostCall callbacks and then the NewAllocator callbacks) into a helper method on ExprEngine then call the helper from both VisitCXXNewAllocatorCall() and processCallExit()?

[xazax.hun]

Do you have a plan for the new false negatives when c++-allocator-inlining is on? Should the user mark allocation functions with attributes?

Also, I was wondering if it would make sense to only have the PostCall callback return the casted memory region in these specific cases instead of introducing a new callback.

Otherwise looks good.

lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

143

Is it possible to have a NonLoc Target? If no, would it be better to make it Loc type?

[NoQ]

In D41406#970985, @xazax.hun wrote:

Also, I was wondering if it would make sense to only have the PostCall callback return the casted memory region in these specific cases instead of introducing a new callback.

Yep, i actually think it's a very good idea. Like, it was obvious to me that it's a bad idea because PostCall and CallEvent should describe the function call just like before - in particular, the moment they capture and the return value should correspond to the actual value returned from the function. And it's kinda obvious by now that there are two different values, both of which may be of interest to the user, and certain amount of modeling (namely, pointer cast) occurs between these two moments.

But i guess we could also squeeze both values into CallEvent. Like, make a new CallEvent sub-class called, say, CXXNewAllocatorCall, which would additionally provide a method, say, .getAllocatedObject(), which would re-evaluate the cast and return the casted object. The whole reusable casting procedure would in this case live in SValBuilder and be accessed from both ExprEngine and CallEvent (in the latter case through .getState()->getStateManager()).

Performance-wise, switching to the CallEvent approach would trade an additional exploded node or two for a potentially unlimited number of re-evaluations of the cast (in every checker) - both seem lightweight enough for now to never bother. Storing the casted value in CallEvent would require making the CallEvent object more bulky, which doesn't seem justified. Futureproofness-wise, we may potentially re-introduce the callback when we actually require two program points, or if our cast procedure becomes slow and we'd want to re-use the result, but both is unlikely.

[NoQ]

which would re-evaluate the cast and return the casted object

Rather not. Because i'm changing my mind again about avoiding the redundant cast in &element{T, HeapSymRegion{conj<T *>}} - this time by not calling evalCast after a conservatively evaluated operator new call (this would ensure that no existing behavior breaks in the conservative case without changing how all casts everywhere work), and this wouldn't be compatible with this CallEvent-based approach because in the CallEvent we have no way of figuring out if the call was inlined or evaluated conservatively. We could still move the cast logic before the PostCall callback, and then retrieve the casted value from the program state (wherever it is). But this is an example of stuff that becomes harder when we merge the callbacks together, and it took me a few hours to stumble upon that, so who knows what other problems would we encounter, so i feel that future-proof-ness is worth it.

[NoQ]

Like, make a new CallEvent sub-class called, say, CXXNewAllocatorCall

Oh, we already have it (CXXAllocatorCall).

[NoQ]

But still, i guess, it is also easier for checker authors to discover a checker callback (it's right there in the CheckerDocumentation, which is short enough to read fully) than to discover a CallEvent sub-class (which was so hard that i never discovered it until like now) and then figure out that it would have the method they need (when they don't even know that they need it).

[NoQ]

Address review comments. Stick to the callback approach for now.

include/clang/StaticAnalyzer/Core/CheckerManager.h

311

That's copied from other methods (eg. runCheckersForPostCall), should i clean them up as well some day?

lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

143

I guess it can be UnknownVal as well, or even UndefinedVal.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

349

I think it's more readable inline. It's a small piece of code with subtle differences between copies (eg. here we have one predecessor node, there we have a set of nodes, then we need to pass the WasInlined flag), and also in VisitCXXNewAllocatorCall() all other checker callbacks are written out directly, so it doesn't make much sense to put these two into a sub-function while leaving three other callbacks in place - rather hurts readability. This code is kind of idiomatic and repeated many times in ExprEngine - maybe we should clean up the whole stuff, but i don't think cleaning up just this place is the right thing to do.

[NoQ]

In D41406#970985, @xazax.hun wrote:

Do you have a plan for the new false negatives when c++-allocator-inlining is on? Should the user mark allocation functions with attributes?

Not immediately - the immediate plan is to simply believe that we'd either see something reasonable inside the call (eg. malloc() or a concrete array), or we'd rather not even try to understand what's going on and how to properly release memory in this case (avoid false positives, which is also good). I guess we shall see if it makes sense to track the allocated value anyway, even in the inlined case, and in this case we'd need to work around tracking stuff twice (eg. operator new(size) { return malloc(size); } would need to be tracked as both new and malloc for the purposes of mismatched deallocator check) or other unexpected issues (because MallocChecker has strong opinions all over the place on how allocated values normally look like). We don't have any annotations in MallocChecker yet, and i didn't think about adding support for them or even see if there are existing useful annotations around.

[NoQ]

Rebase (getter rename).

include/clang/StaticAnalyzer/Core/Checker.h

public:
   template <typename CHECKER>
   static void _register(CHECKER *checker, CheckerManager &mgr) {
     mgr._registerForBranchCondition(
       CheckerManager::CheckBranchConditionFunc(checker,
                                                _checkBranchCondition<CHECKER>));
   }
 };
 
+class NewAllocator {
+  template <typename CHECKER>
+  static void _checkNewAllocator(void *checker, const CXXNewExpr *NE,
+                                 SVal Target, CheckerContext &C) {
+    ((const CHECKER *)checker)->checkNewAllocator(NE, Target, C);
+  }
+
+public:
+  template <typename CHECKER>
+  static void _register(CHECKER *checker, CheckerManager &mgr) {
+    mgr._registerForNewAllocator(
+        CheckerManager::CheckNewAllocatorFunc(checker,
+                                              _checkNewAllocator<CHECKER>));
+  }
+};
+
 class LiveSymbols {
   template <typename CHECKER>
   static void _checkLiveSymbols(void *checker, ProgramStateRef state,
                                 SymbolReaper &SR) {
     ((const CHECKER *)checker)->checkLiveSymbols(state, SR);
   }
 
 public:

include/clang/StaticAnalyzer/Core/CheckerManager.h

void runCheckersForEndFunction(NodeBuilderContext &BC,
                                  ExplodedNode *Pred,
                                  ExprEngine &Eng);
 
   /// \brief Run checkers for branch condition.
   void runCheckersForBranchCondition(const Stmt *condition,
                                      ExplodedNodeSet &Dst, ExplodedNode *Pred,
                                      ExprEngine &Eng);
 
+  /// \brief Run checkers between C++ operator new and constructor calls.
+  void runCheckersForNewAllocator(const CXXNewExpr *NE, SVal Target,
+                                  ExplodedNodeSet &Dst,
+                                  ExplodedNode *Pred,
+                                  ExprEngine &Eng,
+                                  bool wasInlined = false);
+
   /// \brief Run checkers for live symbols.
   ///
   /// Allows modifying SymbolReaper object. For example, checkers can explicitly
   /// register symbols of interest as live. These symbols will not be marked
   /// dead and removed.
   void runCheckersForLiveSymbols(ProgramStateRef state,
                                  SymbolReaper &SymReaper);
 
//===----------------------------------------------------------------------===//
   typedef CheckerFn<void (CheckerContext &)>
       CheckBeginFunctionFunc;
 
   typedef CheckerFn<void (CheckerContext &)>
       CheckEndFunctionFunc;
   
   typedef CheckerFn<void (const Stmt *, CheckerContext &)>
       CheckBranchConditionFunc;
-  
+  typedef CheckerFn<void (const CXXNewExpr *, SVal, CheckerContext &)>
+      CheckNewAllocatorFunc;
+  
   typedef CheckerFn<void (SymbolReaper &, CheckerContext &)>
       CheckDeadSymbolsFunc;
   
   typedef CheckerFn<void (ProgramStateRef,SymbolReaper &)> CheckLiveSymbolsFunc;
   
   typedef CheckerFn<ProgramStateRef (ProgramStateRef,
                                      const InvalidatedSymbols *symbols,
                                      ArrayRef<const MemRegion *> ExplicitRegions,
//===----------------------------------------------------------------------===//
 
   void _registerForEndAnalysis(CheckEndAnalysisFunc checkfn);
 
   void _registerForBeginFunction(CheckEndFunctionFunc checkfn);
   void _registerForEndFunction(CheckEndFunctionFunc checkfn);
 
   void _registerForBranchCondition(CheckBranchConditionFunc checkfn);
 
+  void _registerForNewAllocator(CheckNewAllocatorFunc checkfn);
+
   void _registerForLiveSymbols(CheckLiveSymbolsFunc checkfn);
 
   void _registerForDeadSymbols(CheckDeadSymbolsFunc checkfn);
 
   void _registerForRegionChanges(CheckRegionChangesFunc checkfn);
 
   void _registerForPointerEscape(CheckPointerEscapeFunc checkfn);
 
private:
 
   std::vector<CheckEndAnalysisFunc> EndAnalysisCheckers;
 
   std::vector<CheckBeginFunctionFunc> BeginFunctionCheckers;
   std::vector<CheckEndFunctionFunc> EndFunctionCheckers;
 
   std::vector<CheckBranchConditionFunc> BranchConditionCheckers;
 
+  std::vector<CheckNewAllocatorFunc> NewAllocatorCheckers;
+
   std::vector<CheckLiveSymbolsFunc> LiveSymbolsCheckers;
 
   std::vector<CheckDeadSymbolsFunc> DeadSymbolsCheckers;
 
   std::vector<CheckRegionChangesFunc> RegionChangesCheckers;
 
   std::vector<CheckPointerEscapeFunc> PointerEscapeCheckers;
 

lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp

 class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>,
                                        check::PostStmt<DeclStmt>,
                                        check::PreObjCMessage,
                                        check::PostObjCMessage,
                                        check::ObjCMessageNil,
                                        check::PreCall,
                                        check::PostCall,
                                        check::BranchCondition,
+                                       check::NewAllocator,
                                        check::Location,
                                        check::Bind,
                                        check::DeadSymbols,
                                        check::BeginFunction,
                                        check::EndFunction,
                                        check::EndAnalysis,
                                        check::EndOfTranslationUnit,
                                        eval::Call,
public:
   /// \sa checkPreObjCMessage()
   ///
   /// check::PostCall
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const {}
 
   /// \brief Pre-visit of the condition statement of a branch (such as IfStmt).
   void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const {}
 
+  /// \brief Post-visit the C++ operator new's allocation call.
+  ///
+  /// Execution of C++ operator new consists of the following phases: (1) call
+  /// default or overridden operator new() to allocate memory (2) cast the
+  /// return value of operator new() from void pointer type to class pointer
+  /// type, (3) assuming that the value is non-null, call the object's
+  /// constructor over this pointer, (4) declare that the value of the
+  /// new-expression is this pointer. This callback is called between steps
+  /// (2) and (3). Post-call for the allocator is called after step (1).
+  /// Pre-statement for the new-expression is called on step (4) when the value
+  /// of the expression is evaluated.
+  /// \param NE     The C++ new-expression that triggered the allocation.
+  /// \param Target The allocated region, casted to the class type.
+  void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+                         CheckerContext &) const {}
+
   /// \brief Called on a load from and a store to a location.
   ///
   /// The method will be called each time a location (pointer) value is
   /// accessed.
   /// \param Loc    The value of the location (pointer).
   /// \param IsLoad The flag specifying if the location is a store or a load.
   /// \param S      The load is performed while processing the statement.
   ///

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

 
 class MallocChecker : public Checker<check::DeadSymbols,
                                      check::PointerEscape,
                                      check::ConstPointerEscape,
                                      check::PreStmt<ReturnStmt>,
                                      check::PreCall,
                                      check::PostStmt<CallExpr>,
                                      check::PostStmt<CXXNewExpr>,
+                                     check::NewAllocator,
                                      check::PreStmt<CXXDeleteExpr>,
                                      check::PostStmt<BlockExpr>,
                                      check::PostObjCMessage,
                                      check::Location,
                                      eval::Assume>
 {
 public:
   MallocChecker()
public:
   DefaultBool IsOptimistic;
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckName CheckNames[CK_NumCheckKinds];
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
   void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
+  void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+                         CheckerContext &C) const;
   void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const;
   void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
                             bool Assumption) const;
   void checkLocation(SVal l, bool isLoad, const Stmt *S,
private:
   bool isMemFunction(const FunctionDecl *FD, ASTContext &C) const;
   bool isCMemFunction(const FunctionDecl *FD,
                       ASTContext &C,
                       AllocationFamily Family,
                       MemoryOperationKind MemKind) const;
   bool isStandardNewDelete(const FunctionDecl *FD, ASTContext &C) const;
   ///@}
 
+  /// \brief Process C++ operator new()'s allocation, which is the part of C++
+  /// new-expression that goes before the constructor.
+  void processNewAllocation(const CXXNewExpr *NE, CheckerContext &C,
+                            SVal Target) const;
+
   /// \brief Perform a zero-allocation check.
+  /// The optional \p RetVal parameter specifies the newly allocated pointer
+  /// value; if unspecified, the value of expression \p E is used.
   ProgramStateRef ProcessZeroAllocation(CheckerContext &C, const Expr *E,
                                         const unsigned AllocationSizeArg,
-                                        ProgramStateRef State) const;
+                                        ProgramStateRef State,
+                                        Optional<SVal> RetVal = None) const;
 
   ProgramStateRef MallocMemReturnsAttr(CheckerContext &C,
                                        const CallExpr *CE,
                                        const OwnershipAttr* Att,
                                        ProgramStateRef State) const;
   static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
                                       const Expr *SizeEx, SVal Init,
                                       ProgramStateRef State,
                                       AllocationFamily Family = AF_Malloc);
   static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
                                       SVal SizeEx, SVal Init,
                                       ProgramStateRef State,
                                       AllocationFamily Family = AF_Malloc);
 
   static ProgramStateRef addExtentSize(CheckerContext &C, const CXXNewExpr *NE,
-                                       ProgramStateRef State);
+                                       ProgramStateRef State, SVal Target);
 
   // Check if this malloc() for special flags. At present that means M_ZERO or
   // __GFP_ZERO (in which case, treat it like calloc).
   llvm::Optional<ProgramStateRef>
   performKernelMalloc(const CallExpr *CE, CheckerContext &C,
                       const ProgramStateRef &State) const;
 
   /// Update the RefState to reflect the new memory allocation.
+  /// The optional \p RetVal parameter specifies the newly allocated pointer
+  /// value; if unspecified, the value of expression \p E is used.
   static ProgramStateRef
   MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State,
-                       AllocationFamily Family = AF_Malloc);
+                       AllocationFamily Family = AF_Malloc,
+                       Optional<SVal> RetVal = None);
 
   ProgramStateRef FreeMemAttr(CheckerContext &C, const CallExpr *CE,
                               const OwnershipAttr* Att,
                               ProgramStateRef State) const;
   ProgramStateRef FreeMemAux(CheckerContext &C, const CallExpr *CE,
                              ProgramStateRef state, unsigned Num,
                              bool Hold,
                              bool &ReleasedAllocated,
if (FD->hasAttrs())
           break;
         }
       }
   }
   C.addTransition(State);
 }
 
 // Performs a 0-sized allocations check.
-ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C,
-                                               const Expr *E,
-                                               const unsigned AllocationSizeArg,
-                                               ProgramStateRef State) const {
+ProgramStateRef MallocChecker::ProcessZeroAllocation(
+    CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg,
+    ProgramStateRef State, Optional<SVal> RetVal) const {
   if (!State)
     return nullptr;
 
+  if (!RetVal)
+    RetVal = C.getSVal(E);
+
   const Expr *Arg = nullptr;
 
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     Arg = CE->getArg(AllocationSizeArg);
   }
   else if (const CXXNewExpr *NE = dyn_cast<CXXNewExpr>(E)) {
     if (NE->isArray())
       Arg = NE->getArraySize();
ProgramStateRef MallocChecker::ProcessZeroAllocation(
   SValBuilder &SvalBuilder = C.getSValBuilder();
   DefinedSVal Zero =
       SvalBuilder.makeZeroVal(Arg->getType()).castAs<DefinedSVal>();
 
   std::tie(TrueState, FalseState) =
       State->assume(SvalBuilder.evalEQ(State, *DefArgVal, Zero));
 
   if (TrueState && !FalseState) {
-    SVal retVal = State->getSVal(E, C.getLocationContext());
-    SymbolRef Sym = retVal.getAsLocSymbol();
+    SymbolRef Sym = RetVal->getAsLocSymbol();
     if (!Sym)
       return State;
 
     const RefState *RS = State->get<RegionState>(Sym);
     if (RS) {
       if (RS->isAllocated())
         return TrueState->set<RegionState>(Sym,
                                           RefState::getAllocatedOfSizeZero(RS));
for (const auto *CtorParam : CtorD->parameters()) {
 
     if (CtorParamPointeeT->getAsCXXRecordDecl())
       return true;
   }
 
   return false;
 }
 
-void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
-                                  CheckerContext &C) const {
-
+void MallocChecker::processNewAllocation(const CXXNewExpr *NE,
+                                         CheckerContext &C,
+                                         SVal Target) const {
   if (NE->getNumPlacementArgs())
     for (CXXNewExpr::const_arg_iterator I = NE->placement_arg_begin(),
          E = NE->placement_arg_end(); I != E; ++I)
       if (SymbolRef Sym = C.getSVal(*I).getAsSymbol())
         checkUseAfterFree(Sym, C, *I);
 
   if (!isStandardNewDelete(NE->getOperatorNew(), C.getASTContext()))
     return;
 
   ParentMap &PM = C.getLocationContext()->getParentMap();
   if (!PM.isConsumedExpr(NE) && treatUnusedNewEscaped(NE))
     return;
 
   ProgramStateRef State = C.getState();
   // The return value from operator new is bound to a specified initialization
   // value (if any) and we don't want to loose this value. So we call
   // MallocUpdateRefState() instead of MallocMemAux() which breakes the
   // existing binding.
   State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray
-                                                           : AF_CXXNew);
-  State = addExtentSize(C, NE, State);
-  State = ProcessZeroAllocation(C, NE, 0, State);
+                                                           : AF_CXXNew, Target);
+  State = addExtentSize(C, NE, State, Target);
+  State = ProcessZeroAllocation(C, NE, 0, State, Target);
   C.addTransition(State);
 }
 
+void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
+                                  CheckerContext &C) const {
+  if (!C.getAnalysisManager().getAnalyzerOptions().mayInlineCXXAllocator())
+    processNewAllocation(NE, C, C.getSVal(NE));
+}
+
+void MallocChecker::checkNewAllocator(const CXXNewExpr *NE, SVal Target,
+                                      CheckerContext &C) const {
+  if (!C.wasInlined)
+    processNewAllocation(NE, C, Target);
+}
+
 // Sets the extent value of the MemRegion allocated by
 // new expression NE to its size in Bytes.
 //
 ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C,
                                              const CXXNewExpr *NE,
-                                             ProgramStateRef State) {
+                                             ProgramStateRef State,
+                                             SVal Target) {
   if (!State)
     return nullptr;
   SValBuilder &svalBuilder = C.getSValBuilder();
   SVal ElementCount;
-  const LocationContext *LCtx = C.getLocationContext();
   const SubRegion *Region;
   if (NE->isArray()) {
     const Expr *SizeExpr = NE->getArraySize();
     ElementCount = State->getSVal(SizeExpr, C.getLocationContext());
     // Store the extent size for the (symbolic)region
     // containing the elements.
-    Region = (State->getSVal(NE, LCtx))
-                 .getAsRegion()
+    Region = Target.getAsRegion()
                  ->getAs<SubRegion>()
-                 ->getSuperRegion()
+                 ->StripCasts()
                  ->getAs<SubRegion>();
   } else {
     ElementCount = svalBuilder.makeIntVal(1, true);
-    Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>();
+    Region = Target.getAsRegion()->getAs<SubRegion>();
   }
   assert(Region);
 
   // Set the region's extent equal to the Size in Bytes.
   QualType ElementType = NE->getAllocatedType();
   ASTContext &AstContext = C.getASTContext();
   CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
 
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
   }
 
   return MallocUpdateRefState(C, CE, State, Family);
 }
 
 ProgramStateRef MallocChecker::MallocUpdateRefState(CheckerContext &C,
                                                     const Expr *E,
                                                     ProgramStateRef State,
-                                                    AllocationFamily Family) {
+                                                    AllocationFamily Family,
+                                                    Optional<SVal> RetVal) {
   if (!State)
     return nullptr;
 
   // Get the return value.
-  SVal retVal = State->getSVal(E, C.getLocationContext());
+  if (!RetVal)
+    RetVal = State->getSVal(E, C.getLocationContext());
 
   // We expect the malloc functions to return a pointer.
-  if (!retVal.getAs<Loc>())
+  if (!RetVal->getAs<Loc>())
     return nullptr;
 
-  SymbolRef Sym = retVal.getAsLocSymbol();
+  SymbolRef Sym = RetVal->getAsLocSymbol();
+  // This is a return value of a function that was not inlined, such as malloc()
+  // or new(). We've checked that in the caller. Therefore, it must be a symbol.
   assert(Sym);
 
   // Set the symbol's state to Allocated.
   return State->set<RegionState>(Sym, RefState::getAllocated(Family, E));
 }
 
 ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
                                            const CallExpr *CE,
                                            const OwnershipAttr *Att,

lib/StaticAnalyzer/Core/CheckerManager.cpp

void CheckerManager::runCheckersForBranchCondition(const Stmt *Condition,
                                                    ExplodedNode *Pred,
                                                    ExprEngine &Eng) {
   ExplodedNodeSet Src;
   Src.insert(Pred);
   CheckBranchConditionContext C(BranchConditionCheckers, Condition, Eng);
   expandGraphWithCheckers(C, Dst, Src);
 }
 
+namespace {
+  struct CheckNewAllocatorContext {
+    typedef std::vector<CheckerManager::CheckNewAllocatorFunc> CheckersTy;
+    const CheckersTy &Checkers;
+    const CXXNewExpr *NE;
+    SVal Target;
+    bool WasInlined;
+    ExprEngine &Eng;
+
+    CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
+    CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
+
+    CheckNewAllocatorContext(const CheckersTy &Checkers, const CXXNewExpr *NE,
+                             SVal Target, bool WasInlined, ExprEngine &Eng)
+        : Checkers(Checkers), NE(NE), Target(Target), WasInlined(WasInlined),
+          Eng(Eng) {}
+
+    void runChecker(CheckerManager::CheckNewAllocatorFunc checkFn,
+                    NodeBuilder &Bldr, ExplodedNode *Pred) {
+      // TODO: Does this deserve a custom program point? For now we're re-using
+      // PostImplicitCall because we're guaranteed to use the non-implicit
+      // PostStmt for the PostCall callback, because we have some sort of
+      // call site (CXXNewExpr) in this scenario.
+      ProgramPoint L = PostImplicitCall(NE->getOperatorNew(), NE->getLocStart(),
+                                        Pred->getLocationContext());
+      CheckerContext C(Bldr, Eng, Pred, L, WasInlined);
+      checkFn(NE, Target, C);
+    }
+  };
+}
+
+void CheckerManager::runCheckersForNewAllocator(
+    const CXXNewExpr *NE, SVal Target, ExplodedNodeSet &Dst, ExplodedNode *Pred,
+    ExprEngine &Eng, bool WasInlined) {
+  ExplodedNodeSet Src;
+  Src.insert(Pred);
+  CheckNewAllocatorContext C(NewAllocatorCheckers, NE, Target, WasInlined, Eng);
+  expandGraphWithCheckers(C, Dst, Src);
+}
+
 /// \brief Run checkers for live symbols.
 void CheckerManager::runCheckersForLiveSymbols(ProgramStateRef state,
                                                SymbolReaper &SymReaper) {
   for (unsigned i = 0, e = LiveSymbolsCheckers.size(); i != e; ++i)
     LiveSymbolsCheckers[i](state, SymReaper);
 }
 
 namespace {
void CheckerManager::_registerForEndFunction(CheckEndFunctionFunc checkfn) {
   EndFunctionCheckers.push_back(checkfn);
 }
 
 void CheckerManager::_registerForBranchCondition(
                                              CheckBranchConditionFunc checkfn) {
   BranchConditionCheckers.push_back(checkfn);
 }
 
+void CheckerManager::_registerForNewAllocator(CheckNewAllocatorFunc checkfn) {
+  NewAllocatorCheckers.push_back(checkfn);
+}
+
 void CheckerManager::_registerForLiveSymbols(CheckLiveSymbolsFunc checkfn) {
   LiveSymbolsCheckers.push_back(checkfn);
 }
 
 void CheckerManager::_registerForDeadSymbols(CheckDeadSymbolsFunc checkfn) {
   DeadSymbolsCheckers.push_back(checkfn);
 }
 

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
   StmtNodeBuilder ValueBldr(DstPostCall, DstPostValue, *currBldrCtx);
   for (auto I : DstPostCall) {
     ProgramStateRef State = I->getState();
     ValueBldr.generateNode(
         CNE, I,
         pushCXXNewAllocatorValue(State, CNE, LCtx, State->getSVal(CNE, LCtx)));
   }
 
-  getCheckerManager().runCheckersForPostCall(Dst, DstPostValue,
-                                             *Call, *this);
+  ExplodedNodeSet DstPostPostCallCallback;
+  getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
+                                             DstPostValue, *Call, *this);
+  for (auto I : DstPostPostCallCallback) {
+    getCheckerManager().runCheckersForNewAllocator(
+        CNE, peekCXXNewAllocatorValue(I->getState(), CNE, LCtx), Dst, I, *this);
+  }
 }
 
 void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   // FIXME: Much of this should eventually migrate to CXXAllocatorCall.
   // Also, we need to decide how allocators actually work -- they're not
   // really part of the CXXNewExpr because they happen BEFORE the
   // CXXConstructExpr subexpression. See PR12014 for some discussion.

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

for (ExplodedNodeSet::iterator I = CleanedNodes.begin(),
     NodeBuilderContext Ctx(Engine, calleeCtx->getCallSiteBlock(), CEENode);
     SaveAndRestore<const NodeBuilderContext*> NBCSave(currBldrCtx,
         &Ctx);
     SaveAndRestore<unsigned> CBISave(currStmtIdx, calleeCtx->getIndex());
 
     CallEventRef<> UpdatedCall = Call.cloneWithState(CEEState);
 
     ExplodedNodeSet DstPostCall;
+    if (const CXXNewExpr *CNE = dyn_cast_or_null<CXXNewExpr>(CE)) {
+      ExplodedNodeSet DstPostPostCallCallback;
+      getCheckerManager().runCheckersForPostCall(DstPostPostCallCallback,
+                                                 CEENode, *UpdatedCall, *this,
+                                                 /*WasInlined=*/true);
+      for (auto I : DstPostPostCallCallback) {
+        getCheckerManager().runCheckersForNewAllocator(
+            CNE, peekCXXNewAllocatorValue(I->getState(), CNE,
+                                          calleeCtx->getParent()),
+            DstPostCall, I, *this,
+            /*WasInlined=*/true);
+      }
+    } else {
       getCheckerManager().runCheckersForPostCall(DstPostCall, CEENode,
                                                  *UpdatedCall, *this,
                                                  /*WasInlined=*/true);
-
+    }
     ExplodedNodeSet Dst;
     if (const ObjCMethodCall *Msg = dyn_cast<ObjCMethodCall>(Call)) {
       getCheckerManager().runCheckersForPostObjCMessage(Dst, DstPostCall, *Msg,
                                                         *this,
                                                         /*WasInlined=*/true);
     } else if (CE) {
       getCheckerManager().runCheckersForPostStmt(Dst, DstPostCall, CE,
                                                  *this, /*WasInlined=*/true);

test/Analysis/NewDelete-custom.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -fblocks -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -DLEAKS=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DALLOCATOR_INLINING=1 -fblocks -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete,cplusplus.NewDeleteLeaks,unix.Malloc -std=c++11 -analyzer-config c++-allocator-inlining=true -DLEAKS=1 -DALLOCATOR_INLINING=1 -fblocks -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
-#ifndef LEAKS
+#if !LEAKS
 // expected-no-diagnostics
 #endif
 
 
 void *allocator(std::size_t size);
 
 void *operator new[](std::size_t size) throw() { return allocator(size); }
 void *operator new(std::size_t size) throw() { return allocator(size); }
 void *operator new(std::size_t size, std::nothrow_t& nothrow) throw() { return allocator(size); }
 void *operator new(std::size_t, double d);
 
 class C {
 public:
   void *operator new(std::size_t);  
 };
 
 void testNewMethod() {
   void *p1 = C::operator new(0); // no warn
 
   C *p2 = new C; // no warn
 
   C *c3 = ::new C;
 }
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
 // expected-warning@-2{{Potential leak of memory pointed to by 'c3'}}
 #endif
 
 void testOpNewArray() {
   void *p = operator new[](0); // call is inlined, no warn
 }
 
 void testNewExprArray() {
   int *p = new int[0];
 }
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
 #endif
 
 
 //----- Custom non-placement operators
 void testOpNew() {
   void *p = operator new(0); // call is inlined, no warn
 }
 
 void testNewExpr() {
   int *p = new int;
 }
-#ifdef LEAKS
+#if LEAKS && !ALLOCATOR_INLINING
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
 #endif
 
 
 //----- Custom NoThrow placement operators
 void testOpNewNoThrow() {
   void *p = operator new(0, std::nothrow);
 }
-#ifdef LEAKS
+#if LEAKS
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
 #endif
 
 void testNewExprNoThrow() {
   int *p = new(std::nothrow) int;
 }
-#ifdef LEAKS
+#if LEAKS
 // expected-warning@-2{{Potential leak of memory pointed to by 'p'}}
 #endif
 
 //----- Custom placement operators
 void testOpNewPlacement() {
   void *p = operator new(0, 0.1); // no warn
-} 
+}
 
 void testNewExprPlacement() {
   int *p = new(0.1) int; // no warn
 }

test/Analysis/new-ctor-malloc.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection,unix.Malloc -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+
+void clang_analyzer_eval(bool);
+
+typedef __typeof__(sizeof(int)) size_t;
+
+void *malloc(size_t size);
+
+void *operator new(size_t size) throw() {
+  void *x = malloc(size);
+  if (!x)
+    return nullptr;
+  return x;
+}
+
+void checkNewAndConstructorInlining() {
+  int *s = new int;
+} // expected-warning {{Potential leak of memory pointed to by 's'}}