Add kfree( ) to MallocChecker.cpp

[andrewmw94]

Summary

Adds the kfree( ) function to MallocChecker.cpp

[andrewmw94]

Change the test file kmalloc-linux.c to use kfree( ). Sorry if there was a way to put both patch files together; I wasn't able to find one.

[dcoughlin]

Thanks for the patches! I've commandeered this revision to be able to update the diff to merge the two patch files into one. You should commandeer it back by using the 'Commandeer Revision' action.

By the way, there are instructions for how to create and upload diffs using Phabricator at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[dcoughlin]

Merge the two patches into one diff.

[dcoughlin]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

593 ↗

(On Diff #67862)

It looks like you are using tabs here. The LLVM style guide calls for spaces. http://llvm.org/docs/CodingStandards.html#use-spaces-instead-of-tabs

If you haven't seen it, the clang-format tool can help automatically format code to conform to LLVM's expected style.

950 ↗

(On Diff #67862)

It looks like this method has recently been renamed "parameters()" and so your patch doesn't compile. Are you using top of trunk from the llvm.org repo?

1296 ↗

(On Diff #67862)

I think it would good to add a test that covers this code. There are analogous tests in test/Analysis/free.c -- that would be a good place for this added test.

1309 ↗

(On Diff #67862)

Should there be a test for this case, as well? This would be exercised if code is allocated with kmalloc() and then freed in some way other than kfree (such as free() or delete). There are analogous tests in the MismatchedDeallocator test files.

2206 ↗

(On Diff #67862)

There are tabs on these two lines, as well.

[andrewmw94]

I agree that more tests would be good. Where should I put them? Should it be in a separate file or should I just have kmalloc/kfree tests next to the malloc/free ones?

[dcoughlin]

I would recommend putting the tests in existing test files next to tests that for similar diagnostics for other allocation/free schemes. So the test for printExpectedAllocName() should go in test/Analysis/free.c and the test for printExpectedDeallocName() should probably go in test/Analysis/MismatchedDeallocator-checker-test.mm.

[andrewmw94]

One more thing, obviously it should mimic malloc/free's behavior in complaining about delete/new being used. Should it also complain about free/malloc being used? I can't imagine that would be something people would usually intend to do, but I don't think it's really "incorrect." Thought?

[dcoughlin]

In D23375#527291, @andrewmw94 wrote:

One more thing, obviously it should mimic malloc/free's behavior in complaining about delete/new being used. Should it also complain about free/malloc being used? I can't imagine that would be something people would usually intend to do, but I don't think it's really "incorrect." Thought?

I'm not familiar with linux kernel programming. Is free() available in the kernel? If someone did define their own free() and used it with kmalloc'd memory, would they consider this a false positive?

MallocChecker.cpp

 
 // Used to check correspondence between allocators and deallocators.
 enum AllocationFamily {
   AF_None,
   AF_Malloc,
   AF_CXXNew,
   AF_CXXNewArray,
   AF_IfNameIndex,
+  AF_KMalloc,
   AF_Alloca
 };
 
 class RefState {
   enum Kind { // Reference to allocated memory.
               Allocated,
               // Reference to zero-allocated memory.
               AllocatedOfSizeZero,
class MallocChecker : public Checker<check::DeadSymbols,
                                      eval::Assume>
 {
 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_kfree(nullptr), II_if_nameindex(nullptr),
+        II_if_freenameindex(nullptr), II_wcsdup(nullptr),
+        II_win_wcsdup(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_BadFree[CK_NumCheckKinds];
   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_kfree, *II_if_nameindex, *II_if_freenameindex,
+                         *II_wcsdup, *II_win_wcsdup;
   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.
static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
                                       const Expr *SizeEx, SVal Init,
                                       ProgramStateRef State,
                                       AllocationFamily Family = AF_Malloc);
   static ProgramStateRef MallocMemAux(CheckerContext &C, const CallExpr *CE,
                                       SVal SizeEx, SVal Init,
                                       ProgramStateRef State,
                                       AllocationFamily Family = AF_Malloc);
 
-  // Check if this malloc() for special flags. At present that means M_ZERO or
+  // Check this malloc() for special flags. At present that means M_ZERO or
   // __GFP_ZERO (in which case, treat it like calloc).
   llvm::Optional<ProgramStateRef>
   performKernelMalloc(const CallExpr *CE, CheckerContext &C,
                       const ProgramStateRef &State) const;
 
   /// Update the RefState to reflect the new memory allocation.
   static ProgramStateRef
   MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State,
void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
   II_realloc = &Ctx.Idents.get("realloc");
   II_reallocf = &Ctx.Idents.get("reallocf");
   II_calloc = &Ctx.Idents.get("calloc");
   II_valloc = &Ctx.Idents.get("valloc");
   II_strdup = &Ctx.Idents.get("strdup");
   II_strndup = &Ctx.Idents.get("strndup");
   II_wcsdup = &Ctx.Idents.get("wcsdup");
   II_kmalloc = &Ctx.Idents.get("kmalloc");
+  II_kfree = &Ctx.Idents.get("kfree");
   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");
 }
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_kfree)
         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)
if (FunI == II_malloc) {
       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_kfree) {
       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) {
       State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
if (!ConstructE)
     return false;
 
   if (!NE->getAllocatedType()->getAsCXXRecordDecl())
     return false;
 
   const CXXConstructorDecl *CtorD = ConstructE->getConstructor();
 
   // Iterate over the constructor parameters.
-  for (const auto *CtorParam : CtorD->parameters()) {
+  for (const auto *CtorParam : CtorD->params()) {
 
     QualType CtorParamPointeeT = CtorParam->getType()->getPointeeType();
     if (CtorParamPointeeT.isNull())
       continue;
 
     CtorParamPointeeT = getDeepPointeeType(CtorParamPointeeT);
 
     if (CtorParamPointeeT->getAsCXXRecordDecl())
void MallocChecker::printExpectedAllocName(raw_ostream &os, CheckerContext &C,
                                            const Expr *E) const {
   AllocationFamily Family = getAllocationFamily(C, E);
 
   switch(Family) {
     case AF_Malloc: os << "malloc()"; return;
     case AF_CXXNew: os << "'new'"; return;
     case AF_CXXNewArray: os << "'new[]'"; return;
     case AF_IfNameIndex: os << "'if_nameindex()'"; return;
+    case AF_KMalloc: os <<"kmalloc()"; return;
     case AF_Alloca:
     case AF_None: llvm_unreachable("not a deallocation expression");
   }
 }
 
 void MallocChecker::printExpectedDeallocName(raw_ostream &os,
                                              AllocationFamily Family) const {
   switch(Family) {
     case AF_Malloc: os << "free()"; return;
     case AF_CXXNew: os << "'delete'"; return;
     case AF_CXXNewArray: os << "'delete[]'"; return;
     case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
+    case AF_KMalloc: os << "kfree()"; return;
     case AF_Alloca:
     case AF_None: llvm_unreachable("suspicious argument");
   }
 }
 
 ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
                                           const Expr *ArgExpr,
                                           const Expr *ParentExpr,

 }
 
 Optional<MallocChecker::CheckKind>
 MallocChecker::getCheckIfTracked(AllocationFamily Family,
                                  bool IsALeakCheck) const {
   switch (Family) {
   case AF_Malloc:
   case AF_Alloca:
+  case AF_KMalloc:
   case AF_IfNameIndex: {
     if (ChecksEnabled[CK_MallocChecker])
       return CK_MallocChecker;
 
     return Optional<MallocChecker::CheckKind>();
   }
   case AF_CXXNew:
   case AF_CXXNewArray: {
if (const AnyFunctionCall *FC = dyn_cast<AnyFunctionCall>(&Call)) {
     const FunctionDecl *FD = FC->getDecl();
     if (!FD)
       return;
 
     ASTContext &Ctx = C.getASTContext();
     if (ChecksEnabled[CK_MallocChecker] &&
         (isCMemFunction(FD, Ctx, AF_Malloc, MemoryOperationKind::MOK_Free) ||
          isCMemFunction(FD, Ctx, AF_IfNameIndex,
-                        MemoryOperationKind::MOK_Free)))
+      MemoryOperationKind::MOK_Free) ||
+   isCMemFunction(FD, Ctx, AF_KMalloc, MemoryOperationKind::MOK_Free)))
       return;
 
     if (ChecksEnabled[CK_NewDeleteChecker] &&
         isStandardNewDelete(FD, Ctx))
       return;
   }
 
   // Check if the callee of a method is deleted.