[Analyzer] Detect when function pointer is freed

[AndersRonnholm]

Summary

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

[NoQ]

Hello, thanks for the patch!

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

void foo() {
  free(&foo);
}

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?

test/Analysis/malloc.c

1780 ↗

(On Diff #94012)

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.

[danielmarjamaki]

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

[danielmarjamaki]

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

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

[AndersRonnholm]

Updated after comments

[NoQ]

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?

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

private:
   void ReportDoubleFree(CheckerContext &C, SourceRange Range, bool Released,
                         SymbolRef Sym, SymbolRef PrevSym) const;
 
   void ReportDoubleDelete(CheckerContext &C, SymbolRef Sym) const;
 
   void ReportUseZeroAllocated(CheckerContext &C, SourceRange Range,
                               SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext &C, 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,
                              CheckerContext &C) const;
 
   void reportLeak(SymbolRef Sym, ExplodedNode *N, CheckerContext &C) const;
 
   /// The bug visitor which allows us to print extra diagnostics along the
// check that the call to free is proper.
         const Expr *AllocExpr = cast<Expr>(RsBase->getStmt());
         ReportOffsetFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr,
                          AllocExpr);
         return nullptr;
       }
     }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+    ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr);
+    return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
                                               RsBase->isAllocatedOfSizeZero());
 
   // Clean out the info on previous call to free return info.
   State = State->remove<FreeReturnValue>(SymBase);
 
   // Keep track of the return value. If it is NULL, we will know that free
   // failed.
if (ExplodedNode *N = C.generateErrorNode()) {
     if (Sym) {
       R->markInteresting(Sym);
       R->addVisitor(llvm::make_unique<MallocBugVisitor>(Sym));
     }
     C.emitReport(std::move(R));
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext &C, SVal ArgVal,
+                                              SourceRange Range,
+                                              const Expr *FreeExpr) const {
+  if (!ChecksEnabled[CK_MallocChecker])
+    return;
+
+  Optional<MallocChecker::CheckKind> 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<ElementRegion>(MR))
+      MR = ER->getSuperRegion();
+
+    Os << "Argument to ";
+    if (!printAllocDeallocName(Os, C, FreeExpr))
+      Os << "deallocator";
+
+    Os << " is a function pointer";
+
+    auto R = llvm::make_unique<BugReport>(*BT_BadFree[*CheckKind], Os.str(), N);
+    R->markInteresting(MR);
+    R->addRange(Range);
+    C.emitReport(std::move(R));
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C,
                                              const CallExpr *CE,
                                              bool FreesOnFail,
-                                             ProgramStateRef State, 
+                                             ProgramStateRef State,
                                              bool SuffixWithN) const {
   if (!State)
     return nullptr;
 
   if (SuffixWithN && CE->getNumArgs() < 3)
     return nullptr;
   else if (CE->getNumArgs() < 2)
     return nullptr;

cfe/trunk/test/Analysis/malloc.c

int testNoCheckerDataPropogationFromLogicalOpOperandToOpResult(void) {
    char *value = malloc(10);
    int ok = (param && value);
    free(param);
    free(value);
    // Previously we ended up with 'Use of memory after it is freed' on return.
    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.
 
 void testMallocWithParam(int **p) {
   *p = (int*) malloc(sizeof(int));
   *p = 0; // FIXME: should warn here
 }