[PATCH] D31650: [Analyzer] Detect when function pointer is freed

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301913: [analyzer] Detect bad free of function pointers 
(authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D31650?vs=95929=97432#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31650

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


Index: cfe/trunk/test/Analysis/malloc.c
===
--- cfe/trunk/test/Analysis/malloc.c
+++ cfe/trunk/test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function 
pointer}}
+}
+
 // 
 // False negatives.
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -401,6 +401,9 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) 
const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1564,6 +1567,11 @@
 }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), 
ParentExpr);
+return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
   RsBase->isAllocatedOfSizeZero());
 
@@ -2024,10 +2032,45 @@
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+  SourceRange Range,
+  const Expr *FreeExpr) const {
+  if (!ChecksEnabled[CK_MallocChecker])
+return;
+
+  Optional CheckKind = getCheckIfTracked(C, 
FreeExpr);
+  if (!CheckKind.hasValue())
+return;
+
+  if (ExplodedNode *N = C.generateErrorNode()) {
+if (!BT_BadFree[*CheckKind])
+  BT_BadFree[*CheckKind].reset(
+  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+
+SmallString<100> Buf;
+llvm::raw_svector_ostream Os(Buf);
+
+const MemRegion *MR = ArgVal.getAsRegion();
+while (const ElementRegion *ER = dyn_cast_or_null(MR))
+  MR = ER->getSuperRegion();
+
+Os << "Argument to ";
+if (!printAllocDeallocName(Os, C, FreeExpr))
+  Os << "deallocator";
+
+Os << " is a function pointer";
+
+auto R = llvm::make_unique(*BT_BadFree[*CheckKind], Os.str(), 
N);
+R->markInteresting(MR);
+R->addRange(Range);
+C.emitReport(std::move(R));
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext ,
  const CallExpr *CE,
  bool FreesOnFail,
- ProgramStateRef State, 
+ ProgramStateRef State,
  bool SuffixWithN) const {
   if (!State)
 return nullptr;


Index: cfe/trunk/test/Analysis/malloc.c
===
--- cfe/trunk/test/Analysis/malloc.c
+++ cfe/trunk/test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function pointer}}
+}
+
 // 
 // False negatives.
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -401,6 +401,9 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo 

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

[Artem Dergachev via Phabricator via cfe-commits]

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

Looks good, thanks!

Did you evaluate this on a large codebase - were warnings plentiful and were 
there any false positives known?

I'd like to summon Anna here for a little bit because that's a new check that 
is enabled by default, so it's always a bit of a historical moment:

- Does the warning message sound reasonable? (it does to me).
- Should we keep it as part of the unix.Malloc checker package, or should we be 
able to enable it separately?


Repository:
  rL LLVM

https://reviews.llvm.org/D31650



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

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

[Anders Rönnholm via Phabricator via cfe-commits]

AndersRonnholm updated this revision to Diff 95929.
AndersRonnholm added a comment.

Updated after comments


Repository:
  rL LLVM

https://reviews.llvm.org/D31650

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


Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function 
pointer}}
+}
+
 // 
 // False negatives.
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -392,6 +392,9 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) 
const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1516,6 +1519,11 @@
 }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), 
ParentExpr);
+return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
   RsBase->isAllocatedOfSizeZero());
 
@@ -1976,6 +1984,42 @@
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+  SourceRange Range,
+  const Expr *FreeExpr) const {
+
+  if (!ChecksEnabled[CK_MallocChecker])
+return;
+
+  Optional CheckKind = getCheckIfTracked(C, 
FreeExpr);
+  if (!CheckKind.hasValue())
+return;
+
+  if (ExplodedNode *N = C.generateErrorNode()) {
+if (!BT_BadFree[*CheckKind])
+  BT_BadFree[*CheckKind].reset(
+  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+
+SmallString<100> Buf;
+llvm::raw_svector_ostream Os(Buf);
+
+const MemRegion *MR = ArgVal.getAsRegion();
+while (const ElementRegion *ER = dyn_cast_or_null(MR))
+  MR = ER->getSuperRegion();
+
+Os << "Argument to ";
+if (!printAllocDeallocName(Os, C, FreeExpr))
+  Os << "deallocator";
+
+Os << " is a function pointer";
+
+auto R = llvm::make_unique(*BT_BadFree[*CheckKind], Os.str(), 
N);
+R->markInteresting(MR);
+R->addRange(Range);
+C.emitReport(std::move(R));
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMem(CheckerContext ,
   const CallExpr *CE,
   bool FreesOnFail,


Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function pointer}}
+}
+
 // 
 // False negatives.
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -392,6 +392,9 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1516,6 +1519,11 @@
 }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr);
+return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
   RsBase->isAllocatedOfSizeZero());
 
@@ -1976,6 +1984,42 @@
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+   

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

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

danielmarjamaki added a comment.

> void *p = malloc(sizeof(fnptr));

sorry ... I guess that should be something like "void *p = malloc(100);"


Repository:
  rL LLVM

https://reviews.llvm.org/D31650



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

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

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

danielmarjamaki added a comment.

In https://reviews.llvm.org/D31650#717691, @NoQ wrote:

> Is freeing function pointers always undefined?


I guess not.. however I don't personally see why it would be useful to allocate 
function pointers with malloc.

>   I wonder what happens if we take some JIT-enabled javascript engine, maybe 
> with some on-stack replacement of theirs, it may `malloc()` a memory and use 
> it as a function, and then eventually it'd need to free it by design. 
> However, because we're analyzing a small part of the program, we may fail to 
> see in the analyzer that the symbolic pointer originally comes from 
> `malloc()`. Would such rare but important users be able to avoid/suppress the 
> warning?

Maybe when writing JIT there is some usecase, I don't know. The code could be 
rewritten like:

  void *malloc(unsigned long);
  void free(void*);
  
  typedef void (*fnptr)(int);
  
  void allocatedFunctionPointer() {
void *p = malloc(sizeof(fnptr));
fnptr p2 = (fnptr)p;
free(p);
  }

no warning is written about this code.


Repository:
  rL LLVM

https://reviews.llvm.org/D31650



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

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hello, thanks for the patch!

Because we already warn on freeing concrete function pointers, eg.

  void foo() {
free();
  }

this is a useful addition.

Is freeing function pointers always undefined? I wonder what happens if we take 
some JIT-enabled javascript engine, maybe with some on-stack replacement of 
theirs, it may `malloc()` a memory and use it as a function, and then 
eventually it'd need to free it by design. However, because we're analyzing a 
small part of the program, we may fail to see in the analyzer that the symbolic 
pointer originally comes from `malloc()`. Would such rare but important users 
be able to avoid/suppress the warning?




Comment at: test/Analysis/malloc.c:1780
+  void *p = (void*)fnptr;
+  free(p); // expected-warning {{Argument to free() points to a function 
pointer}}
+}

Nope, it doesn't point to a function pointer. It's still the same function 
pointer.
Also, freeing a pointer to a function pointer is not a bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D31650



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

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

[Anders Rönnholm via Phabricator via cfe-commits]

AndersRonnholm created this revision.

The MallocChecker does not currently detect freeing of function pointers.

Example code.

  void (*fnptr)(int);
  void freeIndirectFunctionPtr() {
void *p = (void*)fnptr;
free(p); // expected-warning {{Argument to free() points to a function 
pointer}}
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D31650

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


Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void*)fnptr;
+  free(p); // expected-warning {{Argument to free() points to a function 
pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void*)fnptr); // expected-warning {{Argument to free() is a function 
pointer}}
+}
+
 // 
 // False negatives.
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -392,6 +392,10 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *DeallocExpr,
+ const Expr *ArgExpr) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1516,6 +1520,12 @@
 }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr,
+  ArgExpr);
+return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
   RsBase->isAllocatedOfSizeZero());
 
@@ -1976,6 +1986,47 @@
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+  SourceRange Range,
+  const Expr *DeallocExpr,
+  const Expr *ArgExpr) const {
+
+  if (!ChecksEnabled[CK_MallocChecker])
+return;
+
+  Optional CheckKind =
+  getCheckIfTracked(C, DeallocExpr);
+  if (!CheckKind.hasValue())
+return;
+
+  if (ExplodedNode *N = C.generateErrorNode()) {
+if (!BT_BadFree[*CheckKind])
+  BT_BadFree[*CheckKind].reset(
+  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+
+SmallString<100> Buf;
+llvm::raw_svector_ostream Os(Buf);
+
+const MemRegion *MR = ArgVal.getAsRegion();
+while (const ElementRegion *ER = dyn_cast_or_null(MR))
+  MR = ER->getSuperRegion();
+
+Os << "Argument to ";
+if (!printAllocDeallocName(Os, C, DeallocExpr))
+  Os << "deallocator";
+
+if (ArgExpr->IgnoreParenCasts()->getType()->isFunctionPointerType())
+  Os << " is a function pointer";
+else
+  Os << " points to a function pointer";
+
+auto R = llvm::make_unique(*BT_BadFree[*CheckKind], Os.str(), 
N);
+R->markInteresting(MR);
+R->addRange(Range);
+C.emitReport(std::move(R));
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMem(CheckerContext ,
   const CallExpr *CE,
   bool FreesOnFail,


Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void*)fnptr;
+  free(p); // expected-warning {{Argument to free() points to a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void*)fnptr); // expected-warning {{Argument to free() is a function pointer}}
+}
+
 // 
 // False negatives.
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -392,6 +392,10 @@
   void ReportUseZeroAllocated(CheckerContext , SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext , SVal ArgVal,
+ SourceRange Range, const Expr *DeallocExpr,
+ const Expr *ArgExpr) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded