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

[Leslie Zhai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301384: [analyzer] Teach the MallocChecker about Glib API 
for two arguments (authored by xiangzhai).

Changed prior to commit:
  https://reviews.llvm.org/D30771?vs=96101=96670#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/test/Analysis/gmalloc.c

Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -177,7 +177,10 @@
 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.
@@ -241,7 +244,10 @@
  *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 KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext ) const;
@@ -321,9 +327,12 @@
  bool ,
  bool ReturnsNullOnFailure = false) const;
 
-  ProgramStateRef ReallocMem(CheckerContext , const CallExpr *CE,
- bool FreesMemOnFailure,
- ProgramStateRef State) const;
+  ProgramStateRef ReallocMemAux(CheckerContext , const CallExpr *CE,
+bool FreesMemOnFailure,
+ProgramStateRef State, 
+bool SuffixWithN = false) const;
+  static SVal evalMulForBufferSize(CheckerContext , const Expr *Blocks,
+   const Expr *BlockBytes);
   static ProgramStateRef CallocMem(CheckerContext , const CallExpr *CE,
ProgramStateRef State);
 
@@ -569,6 +578,12 @@
   II_g_try_realloc = ("g_try_realloc");
   II_g_free = ("g_free");
   II_g_memdup = ("g_memdup");
+  II_g_malloc_n = ("g_malloc_n");
+  II_g_malloc0_n = ("g_malloc0_n");
+  II_g_realloc_n = ("g_realloc_n");
+  II_g_try_malloc_n = ("g_try_malloc_n");
+  II_g_try_malloc0_n = ("g_try_malloc0_n");
+  II_g_try_realloc_n = ("g_try_realloc_n");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext ) const {
@@ -617,7 +632,10 @@
   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;
 }
 
@@ -767,6 +785,17 @@
   return None;
 }
 
+SVal MallocChecker::evalMulForBufferSize(CheckerContext , const Expr *Blocks,
+ const Expr *BlockBytes) {
+  SValBuilder  = 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 ) const {
   if (C.wasInlined)
 return;
@@ -813,10 +842,10 @@
   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) {
 

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki accepted this revision.
danielmarjamaki added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Hi Artem,

Could I commit this patch? thanks!

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision now requires changes to proceed.

This looks great, thanks a lot!


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

Never mind :)


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

you can ignore my comment ... LGTM

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


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 96101.
xiangzhai added a comment.

Further reduce redundant code.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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 = 

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

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));
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 96084.
xiangzhai added a comment.

Further reduce redundant code.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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 = 

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:337
+   const Expr *BlockBytes,
+   ProgramStateRef State);
   static ProgramStateRef CallocMem(CheckerContext , const CallExpr *CE,

danielmarjamaki wrote:
> 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.
> 
I will update it tomorrow! I have to gotta home to look after my kid :)


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:337
+   const Expr *BlockBytes,
+   ProgramStateRef State);
   static ProgramStateRef CallocMem(CheckerContext , const CallExpr *CE,

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.



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai marked an inline comment as done.
xiangzhai added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:788
 
+SVal MallocChecker::SValBinMulOp(CheckerContext , const Expr *Blocks,
+ const Expr *BlockBytes, ProgramStateRef 
State) {

danielmarjamaki wrote:
> danielmarjamaki wrote:
> > I have the feeling this should be renamed. Since its purpose is to 
> > calculate the total size maybe MallocChecker::calculateBufferSize()
> 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?
> 
renamed to `evalMulForBufferSize` :)


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95894.
xiangzhai added a comment.

`SValBinMulOp` -> `evalMulForBufferSize`


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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, 

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:788
 
+SVal MallocChecker::SValBinMulOp(CheckerContext , const Expr *Blocks,
+ const Expr *BlockBytes, ProgramStateRef 
State) {

danielmarjamaki wrote:
> I have the feeling this should be renamed. Since its purpose is to calculate 
> the total size maybe MallocChecker::calculateBufferSize()
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?



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> 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.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2040
 
   const Expr *arg0Expr = CE->getArg(0);
   const LocationContext *LCtx = C.getLocationContext();

danielmarjamaki wrote:
> Capital variable: arg0Expr, arg0Val
sorry this is not your code. I remove my comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95887.
xiangzhai added a comment.

Hi Artem,

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

Regards,
Leslie Zhai


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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, 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95880.

Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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 

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95874.
xiangzhai added a comment.

Hi David,

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


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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);
+ 

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

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki requested changes to this revision.
danielmarjamaki added inline comments.
This revision now requires changes to proceed.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:788
 
+SVal MallocChecker::SValBinMulOp(CheckerContext , const Expr *Blocks,
+ const Expr *BlockBytes, ProgramStateRef 
State) {

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



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:790
+ const Expr *BlockBytes, ProgramStateRef 
State) {
+  SValBuilder  = C.getSValBuilder();
+  SVal nBlocks = C.getSVal(Blocks);

Please begin variables with Capitals. svalBuilder,nBlocks,nBlockBytes



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:911
+  if (FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
+SValBuilder  = C.getSValBuilder();
+Init = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);

Capital. svalBuilder



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2040
 
   const Expr *arg0Expr = CE->getArg(0);
   const LocationContext *LCtx = C.getLocationContext();

Capital variable: arg0Expr, arg0Val



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2054
   const Expr *Arg1 = CE->getArg(1);
   if (!Arg1)
 return nullptr;

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?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2061
+const Expr *Arg2 = CE->getArg(2);
+if (!Arg2)
+  return nullptr;

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.



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Leslie Zhai via Phabricator via cfe-commits]

xiangzhai updated this revision to Diff 95684.
xiangzhai added a subscriber: cfe-commits.
xiangzhai added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D30771

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/gmalloc.c

Index: test/Analysis/gmalloc.c
===
--- test/Analysis/gmalloc.c
+++ test/Analysis/gmalloc.c
@@ -13,6 +13,12 @@
 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);
 
@@ -25,6 +31,12 @@
   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);
@@ -38,6 +50,12 @@
   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);
@@ -52,8 +70,100 @@
   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);
+