[analyzer] Teach the MallocChecker about Glib API for two arguments

[xiangzhai]

Summary

Hi LLVM developers,

This is the split patch about Taught the analyzer about Glib API to check Memory-leak for g_malloc_n, g_realloc_n and sort of XXX_n with two arguments APIs.

Please review my patch, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Fix svn diff generate wrong (gmalloc.c) testcase issue.

[NoQ]

Hello, thanks for the patch!

It seems that ReallocMemN re-uses a lot of code from the original ReallocMem. Is it possible to re-use rather than copy-paste the code? For example, by moving the common code to a separate function like ReallocMemAux, which is then called from both functions, or turning these two functions into one with an extra boolean paramenter.

[xiangzhai]

Hi NoQ,

Thanks for your review!

As you suggested, I rename ReallocMem to ReallocMemAux with an extra boolean paramenter SuffixWithN, please review it, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Fix different type issue.

[xiangzhai]

Fix Arg2 Expr might be nullptr issue.

[xiangzhai]

Further reduce redundant code.

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

791–793 ↗

(On Diff #92755)

You can reduce this to SVal nBlocks = C.getSVal(Blocks).

2070 ↗

(On Diff #92755)

In case SuffixWithN is false, this code multiplies SVal for Arg1 by SVal of Arg1, and i do not think this is what you want. I suggest we do not initialize Arg2 or Arg2ValG in case we don't have the suffix.

Generally, i'd prefer the code for computing the buffer size to be structured as

if (SuffixWithN) {
  /* take two expressions, take two values, multiply */
} else {
  /* take one expression, take its value */
}

Then avoid the ?: operator below by always having the correct value in TotalSizeG.

The good thing about such structure is, we have only one if(), so it might be faster to execute, and all the different variants are gathered in one place, so it's easier to read.

By the way, what does G stand for?

[xiangzhai]

Hi Artem,

Thanks for your review!

I updated my patch using if () {} else {} instead of ? :

And someone used Arg1ValG so I just keep formation TotalSizeG have not idea what G stands for...

Regards,
Leslie Zhai

[NoQ]

I updated my patch using if () {} else {} instead of ? :

Nono, that wasn't the point, i have no preference between those. I mean, you can avoid branching completely. See the new inline comment.

And someone used Arg1ValG so I just keep formation TotalSizeG have not idea what G stands for...

Aha, that explains :)

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

904 ↗

(On Diff #94169)

So, the reason for the crash in D31453 is that you forgot to initialize II_g_try_malloc_n in initIdentifierInfo(). It remains null, and therefore FunI == II_g_try_malloc_n is true whenever FD is an anonymous function (for example, an operator). For such operator, you encounter argument SVals that you cannot multiply (pointers in your case), hence you step onto the assertion.

Note how the assertion was useful for detecting the problem; this is the reason why we don't remove assertions when they fail, but try to find the root cause of the issue :)

2075 ↗

(On Diff #94169)

I still think that variable names should be helpful, and it's not worth it to reuse variables just for reusing variables.

I suggest making TotalSizeG a DefinedOrUnknownSVal, and never using Arg1Val after TotalSizeG is computed, on all branches.

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

904 ↗

(On Diff #94169)

I'm also noticing test failures on existing analyzer tests after applying this patch:

Failing Tests (4):

Clang :: Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
Clang :: Analysis/MismatchedDeallocator-checker-test.mm
Clang :: Analysis/NewDelete-custom.cpp
Clang :: Analysis/NewDelete-variadic.cpp

These would need to be fixed. It is likely that the problem with II_g_try_malloc_n is causing these failures, because they are related to custom operators.

[xiangzhai]

Hi Artem,

Thanks for your review!

So, the reason for the crash in D31453 is that you forgot to initialize II_g_try_malloc_n in initIdentifierInfo().

Sorry for my stupid careless...

And I changed TotalSizeG to TotalSize then TotalSize.castAs<DefinedOrUnknownSVal>() as you suggested.

For double check, I run the testcase:

Clang :: Analysis/gmalloc.c
Clang :: Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
Clang :: Analysis/NewDelete-custom.cpp
Clang :: Analysis/NewDelete-variadic.cpp
k3bdevice.cpp

Regards,
Leslie Zhai

[zaks.anna]

Hi Leslie,

How do you run the tests? You should run all clang (or at least all analyzer) tests when testing the patches, not just select test cases.

[xiangzhai]

Hi Anna,

Thanks for your correcting! Sorry for my mistake, I use make check-clang-analysis instead of just select test cases.

Regards,
Leslie Zhai

[xiangzhai]

ping :)

[NoQ]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2091–2094 ↗

(On Diff #94313)

I'll try to explain again. I believe that in a better world, this line would look like this:

ProgramStateRef stateMalloc = MallocMemAux(C, CE, TotalSize, 
                                           UndefinedVal(), StatePtrIsNull);

Please change the code above so that this line, as i wrote it, worked, by computing total size correctly on all branches.

[xiangzhai]

Hi Artem,

Thanks for your review! I will update my patch tomorrow, good night in my time zone :)

Regards,
Leslie Zhai

[xiangzhai]

Hi Artem,

I updated my patch as you suggested: never using Arg1Val after TotalSize is computed, on all branches. please review it, thanks a lot!

Regards,
Leslie Zhai

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

788 ↗

(On Diff #95684)

I have the feeling this should be renamed. Since its purpose is to calculate the total size maybe MallocChecker::calculateBufferSize()

790 ↗

(On Diff #95684)

Please begin variables with Capitals. svalBuilder,nBlocks,nBlockBytes

911 ↗

(On Diff #95684)

Capital. svalBuilder

2040 ↗

(On Diff #95684)

Capital variable: arg0Expr, arg0Val

2054 ↗

(On Diff #95684)

is this "if (!Arg1)" needed? It seems to me that if CE->getNumArgs() returns >= 2 then CE->getArg(1) should be non-NULL. Does it crash/assert if you remove this condition?

2061 ↗

(On Diff #95684)

hmm.. seems weird if CE->getArg(2) returns nullptr even though CE->getNumArgs() >= 3 .. is it possible to remove this check or does that cause some crash etc?

Generally, i'd prefer the code for computing the buffer size to be structured as

I would also prefer such code.

[xiangzhai]

Hi Daniel,

Thanks for your review!

I updated my patch as you suggested! Please review it, thanks a lot! but I argue that:

I have the feeling this should be renamed. Since its purpose is to calculate the total size maybe MallocChecker::calculateBufferSize()

Yes! it is difficult to suitably name variable, function, project, and even my kid :) but calculateBufferSize is not make sense:

1. how to calculate? by evalBinOp do BO_Mul operation.
2. for what? for bytes size so forBufferSize is ok.

Capital variable: arg0Expr, arg0Val

I hold the view that I need to respect original developers' code, and it need a Global Patch for Capital variable, just like KDE's Use nullptr everywhere

is this "if (!Arg1)" needed? It seems to me that if CE->getNumArgs() returns >= 2 then CE->getArg(1) should be non-NULL. Does it crash/assert if you remove this condition?

The same story.

Regards,
Leslie Zhai

[NoQ]

Yay, now that's a lot cleaner!

Regarding style, recently we tend to ask for updating the style in the new code, and i don't think the Golden Rule (http://llvm.org/docs/CodingStandards.html#introduction) does much in our case, but i don't have a strong opinion on that. Maybe we could make a checker to make sure all SValBuilder references are called, say, SVB, and all SVal instances have names ending with Val, and have ourselves fix all the issues reported :) But because the analyzer is done by very few people, i don't think this would happen soon, but having the new code stylish is kind of nice anyway.

However, the null-check for the argument expression is also a performance issue (the compiler would never guess it's always non-null), so i believe it should definitely be removed.

[xiangzhai]

Hi Artem,

Thanks for your review! I updated my patch, please review it.

Regards,
Leslie Zhai

[danielmarjamaki]

I hold the view that I need to respect original developers' code, and it need a Global Patch for Capital variable, just like KDE's Use nullptr everywhere

Yes I was too picky. Please respect the original developers' code.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2040 ↗

(On Diff #95684)

sorry this is not your code. I remove my comment.

[xiangzhai]

Hi Daniel,

I respect you! the father of cppcheck and KDE's buildbot use cppcheck for example Static Analysis K3B thanks for your great job!

Regards,
Leslie Zhai

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

788 ↗

(On Diff #95684)

please respond to comments by clicking on the "Reply" button for the comment. So your responce will be shown near my comment.

yes it can be hard to figure out a good name.

alright, you didn't like my suggestion then let's skip that.

do you need to start the name with "sval". does that indicate that a "sval" is returned? then that would be unusual imho.

I guess I don't have a strong opinion but I would also remove "Bin". The "Mul" says that imho.

how about evalMulForBufferSize?

[xiangzhai]

SValBinMulOp -> evalMulForBufferSize

[xiangzhai]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

788 ↗

(On Diff #95684)

renamed to evalMulForBufferSize :)

[danielmarjamaki]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

337 ↗

(On Diff #95894)

Thanks for renaming the function I am happy now with that name. :-)

hmm.. if you have CheckerContext parameter already then ProgramStateRef parameter seems redundant. You should be able to use C.getState().

However looking at surrounding code it seems you can provide ProgramStateRef for consistency.

I don't have a strong opinion, but I would remove this State.

[xiangzhai]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

337 ↗

(On Diff #95894)

I will update it tomorrow! I have to gotta home to look after my kid :)

[xiangzhai]

Further reduce redundant code.

[danielmarjamaki]

I don't have further comments except that I would personally rewrite:

// Get the value of the size argument.
SVal TotalSize = State->getSVal(Arg1, LCtx);
if (SuffixWithN) {
  const Expr *Arg2 = CE->getArg(2);
  TotalSize = evalMulForBufferSize(C, Arg1, Arg2);
}

to:

// Get the value of the size argument.
SVal TotalSize;
if (!SuffixWithN) {
  TotalSize = State->getSVal(Arg1, LCtx);
} else {
  TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2));
}

[xiangzhai]

Further reduce redundant code.

[danielmarjamaki]

you can ignore my comment ... LGTM

I don't want to accept this revision. Hopefully NoQ or somebody else can do that.

[xiangzhai]

Never mind :)

[NoQ]

This looks great, thanks a lot!

[xiangzhai]

Hi Artem,

Could I commit this patch? thanks!

Regards,
Leslie Zhai

[danielmarjamaki]

If you have svn write permission then please do it.

If you need svn write permission, please see http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

MallocChecker()
       : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
         II_free(nullptr), II_realloc(nullptr), II_calloc(nullptr),
         II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr),
         II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr),
         II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
         II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr),
         II_g_malloc0(nullptr), II_g_realloc(nullptr), II_g_try_malloc(nullptr), 
         II_g_try_malloc0(nullptr), II_g_try_realloc(nullptr), 
-        II_g_free(nullptr), II_g_memdup(nullptr) {}
+        II_g_free(nullptr), II_g_memdup(nullptr), II_g_malloc_n(nullptr), 
+        II_g_malloc0_n(nullptr), II_g_realloc_n(nullptr), 
+        II_g_try_malloc_n(nullptr), II_g_try_malloc0_n(nullptr), 
+        II_g_try_realloc_n(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
   enum CheckKind {
     CK_MallocChecker,
     CK_NewDeleteChecker,
     CK_NewDeleteLeaksChecker,
     CK_MismatchedDeallocatorChecker,
private:
   mutable std::unique_ptr<BugType> BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_UseZerroAllocated[CK_NumCheckKinds];
   mutable IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, *II_free,
                          *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
                          *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
                          *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
                          *II_win_wcsdup, *II_g_malloc, *II_g_malloc0, 
                          *II_g_realloc, *II_g_try_malloc, *II_g_try_malloc0, 
-                         *II_g_try_realloc, *II_g_free, *II_g_memdup;
+                         *II_g_try_realloc, *II_g_free, *II_g_memdup, 
+                         *II_g_malloc_n, *II_g_malloc0_n, *II_g_realloc_n, 
+                         *II_g_try_malloc_n, *II_g_try_malloc0_n, 
+                         *II_g_try_realloc_n;
   mutable Optional<uint64_t> KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext &C) const;
 
   /// \brief Determine family of a deallocation expression.
   AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const;
 
   /// \brief Print names of allocators and deallocators.
ProgramStateRef FreeMemAux(CheckerContext &C, const CallExpr *CE,
                              bool ReturnsNullOnFailure = false) const;
   ProgramStateRef FreeMemAux(CheckerContext &C, const Expr *Arg,
                              const Expr *ParentExpr,
                              ProgramStateRef State,
                              bool Hold,
                              bool &ReleasedAllocated,
                              bool ReturnsNullOnFailure = false) const;
 
-  ProgramStateRef ReallocMem(CheckerContext &C, const CallExpr *CE,
+  ProgramStateRef ReallocMemAux(CheckerContext &C, const CallExpr *CE,
                                 bool FreesMemOnFailure,
-                             ProgramStateRef State) const;
+                                ProgramStateRef State, 
+                                bool SuffixWithN = false) const;
+  static SVal evalMulForBufferSize(CheckerContext &C, const Expr *Blocks,
+                                   const Expr *BlockBytes);
   static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE,
                                    ProgramStateRef State);
 
   ///\brief Check if the memory associated with this symbol was released.
   bool isReleased(SymbolRef Sym, CheckerContext &C) const;
 
   bool checkUseAfterFree(SymbolRef Sym, CheckerContext &C, const Stmt *S) const;
 
void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
   II_g_malloc = &Ctx.Idents.get("g_malloc");
   II_g_malloc0 = &Ctx.Idents.get("g_malloc0");
   II_g_realloc = &Ctx.Idents.get("g_realloc");
   II_g_try_malloc = &Ctx.Idents.get("g_try_malloc");
   II_g_try_malloc0 = &Ctx.Idents.get("g_try_malloc0");
   II_g_try_realloc = &Ctx.Idents.get("g_try_realloc");
   II_g_free = &Ctx.Idents.get("g_free");
   II_g_memdup = &Ctx.Idents.get("g_memdup");
+  II_g_malloc_n = &Ctx.Idents.get("g_malloc_n");
+  II_g_malloc0_n = &Ctx.Idents.get("g_malloc0_n");
+  II_g_realloc_n = &Ctx.Idents.get("g_realloc_n");
+  II_g_try_malloc_n = &Ctx.Idents.get("g_try_malloc_n");
+  II_g_try_malloc0_n = &Ctx.Idents.get("g_try_malloc0_n");
+  II_g_try_realloc_n = &Ctx.Idents.get("g_try_realloc_n");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const {
   if (isCMemFunction(FD, C, AF_Malloc, MemoryOperationKind::MOK_Any))
     return true;
 
   if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any))
     return true;
if (FD->getKind() == Decl::Function) {
     if (Family == AF_Malloc && CheckAlloc) {
       if (FunI == II_malloc || FunI == II_realloc || FunI == II_reallocf ||
           FunI == II_calloc || FunI == II_valloc || FunI == II_strdup ||
           FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup ||
           FunI == II_win_wcsdup || FunI == II_kmalloc ||
           FunI == II_g_malloc || FunI == II_g_malloc0 || 
           FunI == II_g_realloc || FunI == II_g_try_malloc || 
           FunI == II_g_try_malloc0 || FunI == II_g_try_realloc ||
-          FunI == II_g_memdup)
+          FunI == II_g_memdup || FunI == II_g_malloc_n || 
+          FunI == II_g_malloc0_n || FunI == II_g_realloc_n || 
+          FunI == II_g_try_malloc_n || FunI == II_g_try_malloc0_n || 
+          FunI == II_g_try_realloc_n)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckFree) {
       if (FunI == II_if_freenameindex)
         return true;
     }
 
llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc(
   if (TrueState && !FalseState) {
     SVal ZeroVal = C.getSValBuilder().makeZeroVal(Ctx.CharTy);
     return MallocMemAux(C, CE, CE->getArg(0), ZeroVal, TrueState);
   }
 
   return None;
 }
 
+SVal MallocChecker::evalMulForBufferSize(CheckerContext &C, const Expr *Blocks,
+                                         const Expr *BlockBytes) {
+  SValBuilder &SB = C.getSValBuilder();
+  SVal BlocksVal = C.getSVal(Blocks);
+  SVal BlockBytesVal = C.getSVal(BlockBytes);
+  ProgramStateRef State = C.getState();
+  SVal TotalSize = SB.evalBinOp(State, BO_Mul, BlocksVal, BlockBytesVal,
+                                SB.getContext().getSizeType());
+  return TotalSize;
+}
+
 void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
   if (C.wasInlined)
     return;
 
   const FunctionDecl *FD = C.getCalleeDecl(CE);
   if (!FD)
     return;
 
if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
         State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
     } else if (FunI == II_valloc) {
       if (CE->getNumArgs() < 1)
         return;
       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
       State = ProcessZeroAllocation(C, CE, 0, State);
     } else if (FunI == II_realloc || FunI == II_g_realloc || 
                FunI == II_g_try_realloc) {
-      State = ReallocMem(C, CE, false, State);
+      State = ReallocMemAux(C, CE, false, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_reallocf) {
-      State = ReallocMem(C, CE, true, State);
+      State = ReallocMemAux(C, CE, true, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_calloc) {
       State = CallocMem(C, CE, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_free || FunI == II_g_free) {
       State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
     } else if (FunI == II_strdup || FunI == II_win_strdup ||
if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
       SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
       State = MallocMemAux(C, CE, CE->getArg(0), zeroVal, State);
       State = ProcessZeroAllocation(C, CE, 0, State);
     } else if (FunI == II_g_memdup) {
       if (CE->getNumArgs() < 2)
         return;
       State = MallocMemAux(C, CE, CE->getArg(1), UndefinedVal(), State);
       State = ProcessZeroAllocation(C, CE, 1, State);
+    } else if (FunI == II_g_malloc_n || FunI == II_g_try_malloc_n || 
+               FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
+      if (CE->getNumArgs() < 2)
+        return;
+      SVal Init = UndefinedVal();
+      if (FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
+        SValBuilder &SB = C.getSValBuilder();
+        Init = SB.makeZeroVal(SB.getContext().CharTy);
+      }
+      SVal TotalSize = evalMulForBufferSize(C, CE->getArg(0), CE->getArg(1));
+      State = MallocMemAux(C, CE, TotalSize, Init, State);
+      State = ProcessZeroAllocation(C, CE, 0, State);
+      State = ProcessZeroAllocation(C, CE, 1, State);
+    } else if (FunI == II_g_realloc_n || FunI == II_g_try_realloc_n) {
+      if (CE->getNumArgs() < 3)
+        return;
+      State = ReallocMemAux(C, CE, false, State, true);
+      State = ProcessZeroAllocation(C, CE, 1, State);
+      State = ProcessZeroAllocation(C, CE, 2, State);
     }
   }
 
   if (IsOptimistic || ChecksEnabled[CK_MismatchedDeallocatorChecker]) {
     // Check all the attributes, if there are any.
     // There can be multiple of these attributes.
     if (FD->hasAttrs())
       for (const auto *I : FD->specific_attrs<OwnershipAttr>()) {
if (ExplodedNode *N = C.generateErrorNode()) {
     if (Sym) {
       R->markInteresting(Sym);
       R->addVisitor(llvm::make_unique<MallocBugVisitor>(Sym));
     }
     C.emitReport(std::move(R));
   }
 }
 
-ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
+ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C,
                                              const CallExpr *CE,
                                              bool FreesOnFail,
-                                          ProgramStateRef State) const {
+                                             ProgramStateRef State, 
+                                             bool SuffixWithN) const {
   if (!State)
     return nullptr;
 
-  if (CE->getNumArgs() < 2)
+  if (SuffixWithN && CE->getNumArgs() < 3)
+    return nullptr;
+  else if (CE->getNumArgs() < 2)
     return nullptr;
 
   const Expr *arg0Expr = CE->getArg(0);
   const LocationContext *LCtx = C.getLocationContext();
   SVal Arg0Val = State->getSVal(arg0Expr, LCtx);
   if (!Arg0Val.getAs<DefinedOrUnknownSVal>())
     return nullptr;
   DefinedOrUnknownSVal arg0Val = Arg0Val.castAs<DefinedOrUnknownSVal>();
 
   SValBuilder &svalBuilder = C.getSValBuilder();
 
   DefinedOrUnknownSVal PtrEQ =
     svalBuilder.evalEQ(State, arg0Val, svalBuilder.makeNull());
 
-  // Get the size argument. If there is no size arg then give up.
+  // Get the size argument.
   const Expr *Arg1 = CE->getArg(1);
-  if (!Arg1)
-    return nullptr;
 
   // Get the value of the size argument.
-  SVal Arg1ValG = State->getSVal(Arg1, LCtx);
-  if (!Arg1ValG.getAs<DefinedOrUnknownSVal>())
+  SVal TotalSize = State->getSVal(Arg1, LCtx);
+  if (SuffixWithN)
+    TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2));
+  if (!TotalSize.getAs<DefinedOrUnknownSVal>())
     return nullptr;
-  DefinedOrUnknownSVal Arg1Val = Arg1ValG.castAs<DefinedOrUnknownSVal>();
 
   // Compare the size argument to 0.
   DefinedOrUnknownSVal SizeZero =
-    svalBuilder.evalEQ(State, Arg1Val,
+    svalBuilder.evalEQ(State, TotalSize.castAs<DefinedOrUnknownSVal>(),
                        svalBuilder.makeIntValWithPtrWidth(0, false));
 
   ProgramStateRef StatePtrIsNull, StatePtrNotNull;
   std::tie(StatePtrIsNull, StatePtrNotNull) = State->assume(PtrEQ);
   ProgramStateRef StateSizeIsZero, StateSizeNotZero;
   std::tie(StateSizeIsZero, StateSizeNotZero) = State->assume(SizeZero);
   // We only assume exceptional states if they are definitely true; if the
   // state is under-constrained, assume regular realloc behavior.
   bool PrtIsNull = StatePtrIsNull && !StatePtrNotNull;
   bool SizeIsZero = StateSizeIsZero && !StateSizeNotZero;
 
   // If the ptr is NULL and the size is not 0, the call is equivalent to
   // malloc(size).
-  if ( PrtIsNull && !SizeIsZero) {
-    ProgramStateRef stateMalloc = MallocMemAux(C, CE, CE->getArg(1),
+  if (PrtIsNull && !SizeIsZero) {
+    ProgramStateRef stateMalloc = MallocMemAux(C, CE, TotalSize,
                                                UndefinedVal(), StatePtrIsNull);
     return stateMalloc;
   }
 
   if (PrtIsNull && SizeIsZero)
     return State;
 
   // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size).
if (ProgramStateRef stateFree = FreeMemAux(C, CE, StateSizeIsZero, 0,
       // any constrains on the output pointer.
       return stateFree;
     }
 
   // Default behavior.
   if (ProgramStateRef stateFree =
         FreeMemAux(C, CE, State, 0, false, ReleasedAllocated)) {
 
-    ProgramStateRef stateRealloc = MallocMemAux(C, CE, CE->getArg(1),
+    ProgramStateRef stateRealloc = MallocMemAux(C, CE, TotalSize,
                                                 UnknownVal(), stateFree);
     if (!stateRealloc)
       return nullptr;
 
     ReallocPairKind Kind = RPToBeFreedAfterFailure;
     if (FreesOnFail)
       Kind = RPIsFreeOnFailure;
     else if (!ReleasedAllocated)
ProgramStateRef MallocChecker::CallocMem(CheckerContext &C, const CallExpr *CE,
                                          ProgramStateRef State) {
   if (!State)
     return nullptr;
 
   if (CE->getNumArgs() < 2)
     return nullptr;
 
   SValBuilder &svalBuilder = C.getSValBuilder();
-  const LocationContext *LCtx = C.getLocationContext();
-  SVal count = State->getSVal(CE->getArg(0), LCtx);
-  SVal elementSize = State->getSVal(CE->getArg(1), LCtx);
-  SVal TotalSize = svalBuilder.evalBinOp(State, BO_Mul, count, elementSize,
-                                        svalBuilder.getContext().getSizeType());
   SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
+  SVal TotalSize = evalMulForBufferSize(C, CE->getArg(0), CE->getArg(1));
 
   return MallocMemAux(C, CE, TotalSize, zeroVal, State);
 }
 
 LeakInfo
 MallocChecker::getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
                                  CheckerContext &C) const {
   const LocationContext *LeakContext = N->getLocationContext();

cfe/trunk/test/Analysis/gmalloc.c

 // RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
 typedef void* gpointer;
 typedef const void* gconstpointer;
 typedef unsigned long gsize;
 typedef unsigned int guint;
 
 gpointer g_malloc(gsize n_bytes);
 gpointer g_malloc0(gsize n_bytes);
 gpointer g_realloc(gpointer mem, gsize n_bytes);
 gpointer g_try_malloc(gsize n_bytes);
 gpointer g_try_malloc0(gsize n_bytes);
 gpointer g_try_realloc(gpointer mem, gsize n_bytes);
+gpointer g_malloc_n(gsize n_blocks, gsize n_block_bytes);
+gpointer g_malloc0_n(gsize n_blocks, gsize n_block_bytes);
+gpointer g_realloc_n(gpointer mem, gsize n_blocks, gsize n_block_bytes);
+gpointer g_try_malloc_n(gsize n_blocks, gsize n_block_bytes);
+gpointer g_try_malloc0_n(gsize n_blocks, gsize n_block_bytes);
+gpointer g_try_realloc_n(gpointer mem, gsize n_blocks, gsize n_block_bytes);
 void g_free(gpointer mem);
 gpointer g_memdup(gconstpointer mem, guint byte_size);
 
 static const gsize n_bytes = 1024;
 
 void f1() {
   gpointer g1 = g_malloc(n_bytes);
   gpointer g2 = g_malloc0(n_bytes);
   g1 = g_realloc(g1, n_bytes * 2);
   gpointer g3 = g_try_malloc(n_bytes);
   gpointer g4 = g_try_malloc0(n_bytes);
   g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char));
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char));
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char));
 
   g_free(g1);
   g_free(g2);
   g_free(g2); // expected-warning{{Attempt to free released memory}}
 }
 
 void f2() {
   gpointer g1 = g_malloc(n_bytes);
   gpointer g2 = g_malloc0(n_bytes);
   g1 = g_realloc(g1, n_bytes * 2);
   gpointer g3 = g_try_malloc(n_bytes);
   gpointer g4 = g_try_malloc0(n_bytes);
   g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char));
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char));
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char));
 
   g_free(g1);
   g_free(g2);
   g_free(g3);
   g3 = g_memdup(g3, n_bytes); // expected-warning{{Use of memory after it is freed}}
 }
 
 void f3() {
   gpointer g1 = g_malloc(n_bytes);
   gpointer g2 = g_malloc0(n_bytes);
   g1 = g_realloc(g1, n_bytes * 2);
   gpointer g3 = g_try_malloc(n_bytes);
   gpointer g4 = g_try_malloc0(n_bytes);
   g3 = g_try_realloc(g3, n_bytes * 2); // expected-warning{{Potential leak of memory pointed to by 'g4'}}
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g6'}}
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g5'}}
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g8'}}
+
+  g_free(g1); // expected-warning{{Potential leak of memory pointed to by 'g7'}}
+  g_free(g2);
+  g_free(g3);
+}
+
+void f4() {
+  gpointer g1 = g_malloc(n_bytes);
+  gpointer g2 = g_malloc0(n_bytes);
+  g1 = g_realloc(g1, n_bytes * 2);
+  gpointer g3 = g_try_malloc(n_bytes);
+  gpointer g4 = g_try_malloc0(n_bytes);
+  g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g6'}}
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g5'}}
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g8'}}
+
+  g_free(g1); // expected-warning{{Potential leak of memory pointed to by 'g7'}}
+  g_free(g2);
+  g_free(g3);
+  g_free(g4);
+}
+
+void f5() {
+  gpointer g1 = g_malloc(n_bytes);
+  gpointer g2 = g_malloc0(n_bytes);
+  g1 = g_realloc(g1, n_bytes * 2);
+  gpointer g3 = g_try_malloc(n_bytes);
+  gpointer g4 = g_try_malloc0(n_bytes);
+  g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g6'}}
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char));
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g8'}}
+
+  g_free(g1); // expected-warning{{Potential leak of memory pointed to by 'g7'}}
+  g_free(g2);
+  g_free(g3);
+  g_free(g4);
+  g_free(g5);
+}
+
+void f6() {
+  gpointer g1 = g_malloc(n_bytes);
+  gpointer g2 = g_malloc0(n_bytes);
+  g1 = g_realloc(g1, n_bytes * 2);
+  gpointer g3 = g_try_malloc(n_bytes);
+  gpointer g4 = g_try_malloc0(n_bytes);
+  g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char));
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char));
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g8'}}
+
+  g_free(g1); // expected-warning{{Potential leak of memory pointed to by 'g7'}}
+  g_free(g2);
+  g_free(g3);
+  g_free(g4);
+  g_free(g5);
+  g_free(g6);
+}
+
+void f7() {
+  gpointer g1 = g_malloc(n_bytes);
+  gpointer g2 = g_malloc0(n_bytes);
+  g1 = g_realloc(g1, n_bytes * 2);
+  gpointer g3 = g_try_malloc(n_bytes);
+  gpointer g4 = g_try_malloc0(n_bytes);
+  g3 = g_try_realloc(g3, n_bytes * 2);
+  gpointer g5 = g_malloc_n(n_bytes, sizeof(char));
+  gpointer g6 = g_malloc0_n(n_bytes, sizeof(char));
+  g5 = g_realloc_n(g5, n_bytes * 2, sizeof(char));
+  gpointer g7 = g_try_malloc_n(n_bytes, sizeof(char));
+  gpointer g8 = g_try_malloc0_n(n_bytes, sizeof(char));
+  g7 = g_try_realloc_n(g7, n_bytes * 2, sizeof(char)); // expected-warning{{Potential leak of memory pointed to by 'g8'}}
 
   g_free(g1);
   g_free(g2);
   g_free(g3);
+  g_free(g4);
+  g_free(g5);
+  g_free(g6);
+  g_free(g7);
 }