[analyzer] Taught the analyzer about Glib API to check Memory-leak

[xiangzhai]

Summary

Hi LLVM developers,

I have taught the analyzer about Glib API to check Memory-leak issue, please review my patch, thanks a lot!

Regards,
Leslie Zhai

[zaks.anna]

Thanks for the patch!

Could you, please, resubmit the patch with context as described here http://llvm.org/docs/Phabricator.html
Also, please, add tests. See test/Analysis/ for examples.

[xiangzhai]

Hi Anna,

Thanks for your review!

I resubmit the patch with context, please check is it correct, thanks a lot!
And I add testcase: test/Analysis/gmalloc.c

[zaks.anna]

Phabricator still says that context is not available. Please, pass -U9999 when generating the patch.

Thanks!
Anna

test/Analysis/gmalloc.c

1 ↗

(On Diff #83339)

Please, just include core checkers and the Malloc checker. There is no need for UnreachableCode, CastSize and ExprInspection here.

43 ↗

(On Diff #83339)

This test will fail. For example, I am getting:
$ /Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/./bin/clang -cc1 -internal-isystem /Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/bin/../lib/clang/4.0.0/include -nostdsysteminc -analyze -analyzer-checker=osx.SecKeychainAPI,unix.Malloc -fblocks /Volumes/Data/ws/clang/test/Analysis/gmalloc.c -verify
error: no expected directives found: consider use of 'expected-no-diagnostics'
error: 'warning' diagnostics seen but not expected:

File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 43: Attempt to free released memory
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 70: Use of memory after it is freed
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 79: Potential leak of memory pointed to by 'g4'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 82: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 83: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 85: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 87: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 106: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 107: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 109: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 111: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 130: Potential leak of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 131: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 133: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 135: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 155: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 157: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 159: Potential leak of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 179: Potential leak of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 181: Potential leak of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 183: Potential leak of memory pointed to by 'g7'

22 errors generated.

The "-verify" option you pass on command line to the test checks that every warning/analyzer issue matches a comment with a specific pattern:
// expected-warning {"Text of warning here"}

You should run the tests to make sure all of them pass after your patch. If you build with ninja, you's run "ninja check-clang" if you build with make, you can follow instructions here http://clang-analyzer.llvm.org/checker_dev_manual.html#testing

[xiangzhai]

Hi Anna,

Thanks for your review!

I use:

svn diff --diff-cmd=diff -x -U999999 lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/gmalloc.c > Glib-MallocChecker.patch

to generate the patch, please check is it correct.

And I run:

clang -cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify gmalloc.c

There is *NO* issue, please check it, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Make II_g_*malloc*_n more sensible!

[zaks.anna]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

810 ↗

(On Diff #83758)

Why did you remove the old behavior here and below?

I would expect this patch to be strictly additive. If gmalloc APIs take a different number of arguments, please, process them separately. You might need to factor out the processing code to avoid code duplication.

838 ↗

(On Diff #83758)

Should this be conditional on the number of arguments instead of adding two calls to ProcessZeroAllocation?

846 ↗

(On Diff #83758)

This change in how calloc is handled broke the Analysis/malloc.c test.

[xiangzhai]

Hi Anna,

Thanks for your review!

I process them (for example, g_malloc_n, take a different number of arguments) separately.

And I run the testcases for Analysis/malloc.c and Analysis/gmalloc.c

Regards,
Leslie Zhai

[zaks.anna]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

885 ↗

(On Diff #84209)

I am not sure this is correct as the third argument is "size of allocation", which in this case would be the value of CE->getArg(0) times the value of CE->getArg(2).

The current implementation of MallocMemAux would need to be extended to incorporate this:
` // Set the region's extent equal to the Size parameter.

const SymbolicRegion *R =
    dyn_cast_or_null<SymbolicRegion>(RetVal.getAsRegion());
if (!R)
  return nullptr;
if (Optional<DefinedOrUnknownSVal> DefinedSize =
        Size.getAs<DefinedOrUnknownSVal>()) {
  SValBuilder &svalBuilder = C.getSValBuilder();
  DefinedOrUnknownSVal Extent = R->getExtent(svalBuilder);
  DefinedOrUnknownSVal extentMatchesSize =
      svalBuilder.evalEQ(State, Extent, *DefinedSize);

  State = State->assume(extentMatchesSize, true);
  assert(State);
}`

My suggestion is to submit the patch without the 'n' variants and extend MallocMemAux to deal with them as a follow up patch.

889 ↗

(On Diff #84209)

Should this be 'getNumArgs() < 3' ?

891 ↗

(On Diff #84209)

Unfortunately, ReallocMem also assumes a single size argument:

` // Get the size argument. If there is no size arg then give up.

const Expr *Arg1 = CE->getArg(1);
if (!Arg1)
  return nullptr;`

[xiangzhai]

Hi Anna,

Thank you so much for the review!

I will try to implement extend MallocMemAux and ReallocMem in the follow up patch.

Regards,
Leslie Zhai

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

885 ↗

(On Diff #84209)

Hi Anna,

Thanks for your review!

I am not sure this is correct as the third argument is "size of allocation", which in this case would be the value of CE->getArg(0) times the value of CE->getArg(2).

g_malloc_n``` just use two arguments https://developer.gnome.org/glib/stable/glib-Memory-Allocation.html#g-malloc-n

so in this case would be the value of CE->getArg(0) x CE->getArg(1)?

> My suggestion is to submit the patch without the 'n' variants and extend MallocMemAux to deal with them as a follow up patch.

I will drop XXX_n from this patch, then implement them until extend MallocMemAux and ReallocMem implemented in the follow up patch.

Regards,
Leslie Zhai

[xiangzhai]

Hi Anna,

I am not clear why need to calculate the precise allocated size?

For example:

void *ptr = malloc(size);
void *gptr = g_malloc_n(n_blocks, n_block_bytes);

Memory-leak and Use-after-free issues' MallocChecker need the *size* or *n_blocks* x *n_block_bytes*, but if the size is inaccurate, the checker is also *able* to detect the issues https://bugzilla.gnome.org/show_bug.cgi?id=777077

Regards,
Leslie Zhai

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

889 ↗

(On Diff #84209)

sorry typo...

[xiangzhai]

Hi Anna,

I added svalBinMulOp to take BO_Mul evalBinOp for g_malloc_n's two arguments: State->getSVal(CE->getArg(0), C.getLocationContext()) MUL State->getSVal(CE->getArg(1), C.getLocationContext()), so it does NOT need to change MallocMemAux any more, but just use it in this way:

State = MallocMemAux(C, CE,  
  svalBinMulOp(C, State->getSVal(CE->getArg(0), C.getLocationContext()), 
  State->getSVal(CE->getArg(1), C.getLocationContext()), State), 
  UndefinedVal(), State);

And I also added ReallocMemN for g_realloc_n, it also use svalBinMulOp for !StateSizeIsZero check.

Please indicate my mistake, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Fixed the confused

State->getSVal(CE->getArg(1), C.getLocationContext());

with

CE->getArg(1)

issue.

[zaks.anna]

Could you please split the patch into two - one with the previously reviewed support for functions that take a single size value and another patch that models the two size arguments (num and size). It's easier to review patches if they do not grow new functionality. Splitting the patch would also play nicely with the incremental development policy of LLVM.

Thanks!

[xiangzhai]

Hi Anna,

Thanks for your suggest!

Firstly I uploaded Glib-MallocChecker-single-size-value.patch for code review, if submitted to UPSTREAM, then upload another one, correct?

Regards,
Leslie Zhai

[zaks.anna]

Firstly I uploaded Glib-MallocChecker-single-size-value.patch for code review, if submitted to UPSTREAM, then upload another one, correct?

Yes. By the way, you can model XXX_n variants similarly to how calloc is modeled (see CallocMem).

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

785 ↗

(On Diff #89460)

g_malloc0 needs to be initialized with zeros, not UndefinedVal(). See the relevant part in MallocChecker::CallocMem:

SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
return MallocMemAux(C, CE, TotalSize, zeroVal, State);

[xiangzhai]

Hi Anna,

Thanks for your review!

I updated the Glib-MallocChecker-single-size-value.patch to use zeroVal for g_malloc0 and g_try_malloc0

and Yes, I refer CallocMem to implement SValBinMulOp in the Glib-MallocChecker-all-in-one.patch if Glib-MallocChecker-single-size-value.patch could be submitted to UPSTREAM, then I create another patch for XXX_n, thanks for your suggest!

Regards,
Leslie Zhai

[zaks.anna]

I am not clear why need to calculate the precise allocated size?

The information generated by this checker is used for array bounds checking. For example, see https://reviews.llvm.org/D24307

This patch looks good. Do you have commit access or should I commit it on your behalf?

[xiangzhai]

Hi Anna,

Thanks for your review!

The information generated by this checker is used for array bounds checking. For example, see https://reviews.llvm.org/D24307

I will read carefully about that ;-)

This patch looks good. Do you have commit access or should I commit it on your behalf?

Please commit it on my behalf, thanks a lot!

Regards,
Leslie Zhai

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

 {
 public:
   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_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) {}
 
   /// 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_FreeAlloca[CK_NumCheckKinds];
   mutable std::unique_ptr<BugType> BT_MismatchedDealloc;
   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_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;
   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.
void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
   II_kmalloc = &Ctx.Idents.get("kmalloc");
   II_if_nameindex = &Ctx.Idents.get("if_nameindex");
   II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
 
   //MSVC uses `_`-prefixed instead, so we check for them too.
   II_win_strdup = &Ctx.Idents.get("_strdup");
   II_win_wcsdup = &Ctx.Idents.get("_wcsdup");
   II_win_alloca = &Ctx.Idents.get("_alloca");
+
+  // Glib
+  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");
 }
 
 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;
bool MallocChecker::isCMemFunction(const FunctionDecl *FD,
   bool CheckAlloc = (MemKind == MemoryOperationKind::MOK_Any ||
                      MemKind == MemoryOperationKind::MOK_Allocate);
 
   if (FD->getKind() == Decl::Function) {
     const IdentifierInfo *FunI = FD->getIdentifier();
     initIdentifierInfo(C);
 
     if (Family == AF_Malloc && CheckFree) {
-      if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf)
+      if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf || 
+          FunI == II_g_free)
         return true;
     }
 
     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_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)
         return true;
     }
 
     if (Family == AF_IfNameIndex && CheckFree) {
       if (FunI == II_if_freenameindex)
         return true;
     }
 
void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
 
   ProgramStateRef State = C.getState();
   bool ReleasedAllocatedMemory = false;
 
   if (FD->getKind() == Decl::Function) {
     initIdentifierInfo(C.getASTContext());
     IdentifierInfo *FunI = FD->getIdentifier();
 
-    if (FunI == II_malloc) {
+    if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
       if (CE->getNumArgs() < 1)
         return;
       if (CE->getNumArgs() < 3) {
         State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
         if (CE->getNumArgs() == 1)
           State = ProcessZeroAllocation(C, CE, 0, State);
       } else if (CE->getNumArgs() == 3) {
         llvm::Optional<ProgramStateRef> MaybeState =
if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
         State = MaybeState.getValue();
       else
         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) {
+    } else if (FunI == II_realloc || FunI == II_g_realloc || 
+               FunI == II_g_try_realloc) {
       State = ReallocMem(C, CE, false, State);
       State = ProcessZeroAllocation(C, CE, 1, State);
     } else if (FunI == II_reallocf) {
       State = ReallocMem(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) {
+    } 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 ||
                FunI == II_wcsdup || FunI == II_win_wcsdup) {
       State = MallocUpdateRefState(C, CE, State);
     } else if (FunI == II_strndup) {
       State = MallocUpdateRefState(C, CE, State);
     } else if (FunI == II_alloca || FunI == II_win_alloca) {
       if (CE->getNumArgs() < 1)
if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) {
         llvm_unreachable("not a new/delete operator");
     } else if (FunI == II_if_nameindex) {
       // Should we model this differently? We can allocate a fixed number of
       // elements with zeros in the last one.
       State = MallocMemAux(C, CE, UnknownVal(), UnknownVal(), State,
                            AF_IfNameIndex);
     } else if (FunI == II_if_freenameindex) {
       State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
+    } else if (FunI == II_g_malloc0 || FunI == II_g_try_malloc0) {
+      if (CE->getNumArgs() < 1)
+        return;
+      SValBuilder &svalBuilder = C.getSValBuilder();
+      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);
     }
   }
 
   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>()) {

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);
+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);
+
+  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);
+
+  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'}}
+
+  g_free(g1);
+  g_free(g2);
+  g_free(g3);
+}