Index: clang/include/clang/Basic/Attr.td =================================================================== --- clang/include/clang/Basic/Attr.td +++ clang/include/clang/Basic/Attr.td @@ -2255,7 +2255,7 @@ }]; let Args = [IdentifierArgument<"Module">, VariadicParamIdxArgument<"Args">]; - let Subjects = SubjectList<[HasFunctionProto]>; + let Subjects = SubjectList<[HasFunctionProto, ParmVar]>; let Documentation = [Undocumented]; } Index: clang/lib/Sema/SemaDeclAttr.cpp =================================================================== --- clang/lib/Sema/SemaDeclAttr.cpp +++ clang/lib/Sema/SemaDeclAttr.cpp @@ -1714,7 +1714,50 @@ return false; } -static void handleOwnershipAttr(Sema &S, Decl *D, const ParsedAttr &AL) { +static IdentifierInfo *handleOwnershipAttrModule(Sema &S, + const ParsedAttr &AL) { + if (!AL.isArgIdent(0)) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type) + << AL << 1 << AANT_ArgumentIdentifier; + return nullptr; + } + + IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident; + StringRef ModuleName = Module->getName(); + if (normalizeName(ModuleName)) { + Module = &S.PP.getIdentifierTable().get(ModuleName); + } + + return Module; +} + +static void handleOwnershipParamAttr(Sema &S, ParmVarDecl *D, + const ParsedAttr &AL) { + IdentifierInfo *M = handleOwnershipAttrModule(S, AL); + if (!M) + return; + + if (AL.getNumArgs() > 1) { + S.Diag(AL.getLoc(), diag::err_attribute_too_many_arguments) << AL << 1; + return; + } + + QualType QT = D->getType(); + if (!S.isValidPointerAttrType(QT)) { + SourceRange AttrParmRange = SourceRange(); + SourceRange TypeRange = D->getSourceRange(); + S.Diag(AL.getLoc(), diag::warn_attribute_pointers_only) + << AL << AttrParmRange << TypeRange << 0; + return; + } + + ParamIdx *Start = nullptr; + unsigned Size = 0; + llvm::array_pod_sort(Start, Start + Size); + D->addAttr(::new (S.Context) OwnershipAttr(S.Context, AL, M, Start, Size)); +} + +static void handleOwnershipFuncAttr(Sema &S, Decl *D, const ParsedAttr &AL) { // This attribute must be applied to a function declaration. The first // argument to the attribute must be an identifier, the name of the resource, // for example: malloc. The following arguments must be argument indexes, the @@ -1723,11 +1766,9 @@ // after being held. free() should be __attribute((ownership_takes)), whereas // a list append function may well be __attribute((ownership_holds)). - if (!AL.isArgIdent(0)) { - S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type) - << AL << 1 << AANT_ArgumentIdentifier; + IdentifierInfo *Module = handleOwnershipAttrModule(S, AL); + if (!Module) return; - } // Figure out our Kind. OwnershipAttr::OwnershipKind K = @@ -1750,13 +1791,6 @@ break; } - IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident; - - StringRef ModuleName = Module->getName(); - if (normalizeName(ModuleName)) { - Module = &S.PP.getIdentifierTable().get(ModuleName); - } - SmallVector OwnershipArgs; for (unsigned i = 1; i < AL.getNumArgs(); ++i) { Expr *Ex = AL.getArgAsExpr(i); @@ -8105,7 +8139,10 @@ handleAllocAlignAttr(S, D, AL); break; case ParsedAttr::AT_Ownership: - handleOwnershipAttr(S, D, AL); + if (auto *PVD = dyn_cast(D)) + handleOwnershipParamAttr(S, PVD, AL); + else + handleOwnershipFuncAttr(S, D, AL); break; case ParsedAttr::AT_Naked: handleNakedAttr(S, D, AL); Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -301,11 +301,7 @@ check::NewAllocator, check::PostStmt, check::PostObjCMessage, check::Location, eval::Assume> { public: - /// In pessimistic mode, the checker assumes that it does not know which - /// functions might free the memory. - /// In optimistic mode, the checker assumes that all user-defined functions - /// which might free a pointer are annotated. - DefaultBool ShouldIncludeOwnershipAnnotatedFunctions; + DefaultBool IsOptimisticAnalyzerOptionEnabled; DefaultBool ShouldRegisterNoOwnershipChangeVisitor; @@ -448,6 +444,16 @@ /// if the macro value could be determined, and if yes the value itself. mutable Optional KernelZeroSizePtrValue; + LLVM_NODISCARD + ProgramStateRef ownershipFuncAttr(const CallEvent &Call, CheckerContext &C, + const FunctionDecl *FD, + ProgramStateRef S) const; + + LLVM_NODISCARD + ProgramStateRef ownershipParamAttr(const CallEvent &Call, CheckerContext &C, + const FunctionDecl *FD, + ProgramStateRef S) const; + /// Process C++ operator new()'s allocation, which is the part of C++ /// new-expression that goes before the constructor. LLVM_NODISCARD @@ -486,9 +492,10 @@ /// \param [in] State The \c ProgramState right before allocation. /// \returns The ProgramState right after allocation. LLVM_NODISCARD - ProgramStateRef MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const; + ProgramStateRef ownershipReturnsFuncAttr(CheckerContext &C, + const CallEvent &Call, + const OwnershipAttr *Att, + ProgramStateRef State) const; /// Models memory allocation. /// @@ -545,9 +552,10 @@ /// \param [in] State The \c ProgramState right before allocation. /// \returns The ProgramState right after deallocation. LLVM_NODISCARD - ProgramStateRef FreeMemAttr(CheckerContext &C, const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const; + ProgramStateRef ownershipTakesFuncAttr(CheckerContext &C, + const CallEvent &Call, + const OwnershipAttr *Att, + ProgramStateRef State) const; /// Models memory deallocation. /// @@ -1064,14 +1072,41 @@ if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) return true; - const auto *Func = dyn_cast(Call.getDecl()); - if (Func && Func->hasAttrs()) { - for (const auto *I : Func->specific_attrs()) { + const auto *FD = dyn_cast(Call.getDecl()); + if (!FD) + return false; + + if (FD->hasAttrs()) { + for (const auto *I : FD->specific_attrs()) { OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind(); if (OwnKind == OwnershipAttr::Takes || OwnKind == OwnershipAttr::Holds) return true; } } + + for (const auto *P : FD->parameters()) { + unsigned int Index = P->getFunctionScopeIndex(); + bool IsParamInBounds = Index >= 0 && Index < Call.getNumArgs(); + if (!IsParamInBounds) + continue; + + if (!P->hasAttrs()) + continue; + + for (const auto *I : P->specific_attrs()) { + if (I->getModule()->getName() != "malloc") + continue; + + switch (I->getOwnKind()) { + case OwnershipAttr::Returns: + continue; + case OwnershipAttr::Holds: + case OwnershipAttr::Takes: + return true; + } + } + } + return false; } @@ -1080,11 +1115,34 @@ ReallocatingMemFnMap.lookup(Call)) return true; - if (!ShouldIncludeOwnershipAnnotatedFunctions) + if (!IsOptimisticAnalyzerOptionEnabled) + return false; + + const auto *FD = dyn_cast(Call.getDecl()); + if (!FD) return false; - const auto *Func = dyn_cast(Call.getDecl()); - return Func && Func->hasAttr(); + if (FD->hasAttr()) + return true; + + for (const auto *P : FD->parameters()) { + unsigned int Index = P->getFunctionScopeIndex(); + bool IsParamInBounds = Index >= 0 && Index < Call.getNumArgs(); + if (!IsParamInBounds) + continue; + + if (!P->hasAttrs()) + continue; + + for (const auto *I : P->specific_attrs()) { + if (I->getModule()->getName() != "malloc") + continue; + + return true; + } + } + + return false; } llvm::Optional @@ -1396,33 +1454,96 @@ C.addTransition(State); } +ProgramStateRef MallocChecker::ownershipFuncAttr(const CallEvent &Call, + CheckerContext &C, + const FunctionDecl *FD, + ProgramStateRef S) const { + if (!FD->hasAttrs()) + return S; + + for (const auto *I : FD->specific_attrs()) { + ProgramStateRef AttrS = nullptr; + + switch (I->getOwnKind()) { + case OwnershipAttr::Returns: + AttrS = ownershipReturnsFuncAttr(C, Call, I, S); + break; + case OwnershipAttr::Takes: + case OwnershipAttr::Holds: + AttrS = ownershipTakesFuncAttr(C, Call, I, S); + break; + } + + if (AttrS) + S = AttrS; + } + + return S; +} + +ProgramStateRef MallocChecker::ownershipParamAttr(const CallEvent &Call, + CheckerContext &C, + const FunctionDecl *FD, + ProgramStateRef S) const { + for (const auto *P : FD->parameters()) { + unsigned int Index = P->getFunctionScopeIndex(); + bool IsParamInBounds = Index >= 0 && Index < Call.getNumArgs(); + if (!IsParamInBounds) + continue; + + if (!P->hasAttrs()) + continue; + + for (const auto *I : P->specific_attrs()) { + if (I->getModule()->getName() != "malloc") + continue; + + bool IsAlloc = false; + bool Holds = false; + ProgramStateRef AttrS = nullptr; + const Expr *SizeEx = Call.getArgExpr(Index); + const SVal Init = UndefinedVal(); + + switch (I->getOwnKind()) { + case OwnershipAttr::Returns: + AttrS = MallocMemAux(C, Call, SizeEx, Init, S, AF_Malloc); + break; + case OwnershipAttr::Holds: + Holds = true; + LLVM_FALLTHROUGH; + case OwnershipAttr::Takes: + AttrS = FreeMemAux(C, Call, S, Index, Holds, IsAlloc, AF_Malloc); + break; + } + + if (AttrS) + S = AttrS; + } + } + + return S; +} + void MallocChecker::checkOwnershipAttr(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - const auto *CE = dyn_cast_or_null(Call.getOriginExpr()); - if (!CE) + bool IsEnabled = IsOptimisticAnalyzerOptionEnabled || + ChecksEnabled[CK_MismatchedDeallocatorChecker]; + if (!IsEnabled) return; - const FunctionDecl *FD = C.getCalleeDecl(CE); + + ProgramStateRef S = C.getState(); + if (!S) + return; + + const Decl *D = Call.getDecl(); + const FunctionDecl *FD = dyn_cast_or_null(D); if (!FD) return; - if (ShouldIncludeOwnershipAnnotatedFunctions || - 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()) { - switch (I->getOwnKind()) { - case OwnershipAttr::Returns: - State = MallocMemReturnsAttr(C, Call, I, State); - break; - case OwnershipAttr::Takes: - case OwnershipAttr::Holds: - State = FreeMemAttr(C, Call, I, State); - break; - } - } - } - C.addTransition(State); + + S = ownershipFuncAttr(Call, C, FD, S); + S = ownershipParamAttr(Call, C, FD, S); + + C.addTransition(S); } void MallocChecker::checkPostCall(const CallEvent &Call, @@ -1647,10 +1768,9 @@ C.addTransition(State); } -ProgramStateRef -MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const { +ProgramStateRef MallocChecker::ownershipReturnsFuncAttr( + CheckerContext &C, const CallEvent &Call, const OwnershipAttr *Att, + ProgramStateRef State) const { if (!State) return nullptr; @@ -1734,10 +1854,10 @@ return State->set(Sym, RefState::getAllocated(Family, E)); } -ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C, - const CallEvent &Call, - const OwnershipAttr *Att, - ProgramStateRef State) const { +ProgramStateRef +MallocChecker::ownershipTakesFuncAttr(CheckerContext &C, const CallEvent &Call, + const OwnershipAttr *Att, + ProgramStateRef State) const { if (!State) return nullptr; @@ -3562,7 +3682,7 @@ void ento::registerDynamicMemoryModeling(CheckerManager &mgr) { auto *checker = mgr.registerChecker(); - checker->ShouldIncludeOwnershipAnnotatedFunctions = + checker->IsOptimisticAnalyzerOptionEnabled = mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "Optimistic"); checker->ShouldRegisterNoOwnershipChangeVisitor = mgr.getAnalyzerOptions().getCheckerBooleanOption( Index: clang/test/Analysis/malloc-annotations-param.c =================================================================== --- /dev/null +++ clang/test/Analysis/malloc-annotations-param.c @@ -0,0 +1,270 @@ +// RUN: %clang_analyze_cc1 -analyzer-store=region -verify \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \ +// RUN: -analyzer-checker=alpha.core.CastSize \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-config unix.DynamicMemoryModeling:Optimistic=true %s + +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); +void free(void *); +void *realloc(void *ptr, size_t size); +void *calloc(size_t nmemb, size_t size); +void __attribute((ownership_returns(malloc))) * my_malloc(size_t); +void my_free(void *__attribute__((ownership_takes(malloc)))); +void my_freeBoth(void *__attribute__((ownership_takes(malloc))), void *__attribute__((ownership_takes(malloc)))); +void __attribute((ownership_returns(malloc, 1))) * my_malloc2(size_t); +void my_hold(void *__attribute__((ownership_holds(malloc)))); + +// Duplicate attributes are silly, but not an error. +// Duplicate attribute has no extra effect. +// If two are of different kinds, that is an error and reported as such. +void __attribute((ownership_holds(malloc, 1))) +__attribute((ownership_holds(malloc, 1))) +__attribute((ownership_holds(malloc, 3))) my_hold2(void *, void *, void *); +void *my_malloc3(size_t); +void *myglobalpointer; +struct stuff { + void *somefield; +}; +struct stuff myglobalstuff; + +void f1() { + int *p = malloc(12); + return; // expected-warning{{Potential leak of memory pointed to by}} +} + +void f2() { + int *p = malloc(12); + free(p); + free(p); // expected-warning{{Attempt to free released memory}} +} + +void f2_realloc_0() { + int *p = malloc(12); + realloc(p, 0); + realloc(p, 0); // expected-warning{{Attempt to free released memory}} +} + +void f2_realloc_1() { + int *p = malloc(12); + int *q = realloc(p, 0); // no-warning +} + +// ownership attributes tests +void naf1() { + int *p = my_malloc3(12); + return; // no-warning +} + +void n2af1() { + int *p = my_malloc2(12); + return; // expected-warning{{Potential leak of memory pointed to by}} +} + +void af1() { + int *p = my_malloc(12); + return; // expected-warning{{Potential leak of memory pointed to by}} +} + +void af1_b() { + int *p = my_malloc(12); +} // expected-warning{{Potential leak of memory pointed to by}} + +void af1_c() { + myglobalpointer = my_malloc(12); // no-warning +} + +void af1_d() { + struct stuff mystuff; + mystuff.somefield = my_malloc(12); +} // expected-warning{{Potential leak of memory pointed to by}} + +// Test that we can pass out allocated memory via pointer-to-pointer. +void af1_e(void **pp) { + *pp = my_malloc(42); // no-warning +} + +void af1_f(struct stuff *somestuff) { + somestuff->somefield = my_malloc(12); // no-warning +} + +// Allocating memory for a field via multiple indirections to our arguments is OK. +void af1_g(struct stuff **pps) { + *pps = my_malloc(sizeof(struct stuff)); // no-warning + (*pps)->somefield = my_malloc(42); // no-warning +} + +void af2() { + int *p = my_malloc(12); + my_free(p); + free(p); // expected-warning{{Attempt to free released memory}} +} + +void af2b() { + int *p = my_malloc(12); + free(p); + my_free(p); // expected-warning{{Attempt to free released memory}} +} + +void af2c() { + int *p = my_malloc(12); + free(p); + my_hold(p); // expected-warning{{Attempt to free released memory}} +} + +void af2d() { + int *p = my_malloc(12); + free(p); + my_hold2(0, 0, p); // expected-warning{{Attempt to free released memory}} +} + +// No leak if malloc returns null. +void af2e() { + int *p = my_malloc(12); + if (!p) + return; // no-warning + free(p); // no-warning +} + +// This case inflicts a possible double-free. +void af3() { + int *p = my_malloc(12); + my_hold(p); + free(p); // expected-warning{{Attempt to free non-owned memory}} +} + +int *af4() { + int *p = my_malloc(12); + my_free(p); + return p; // expected-warning{{Use of memory after it is freed}} +} + +// This case is (possibly) ok, be conservative +int *af5() { + int *p = my_malloc(12); + my_hold(p); + return p; // no-warning +} + +// This case tests that storing malloc'ed memory to a static variable which is +// then returned is not leaked. In the absence of known contracts for functions +// or inter-procedural analysis, this is a conservative answer. +int *f3() { + static int *p = 0; + p = malloc(12); + return p; // no-warning +} + +// This case tests that storing malloc'ed memory to a static global variable +// which is then returned is not leaked. In the absence of known contracts for +// functions or inter-procedural analysis, this is a conservative answer. +static int *p_f4 = 0; +int *f4() { + p_f4 = malloc(12); + return p_f4; // no-warning +} + +int *f5() { + int *q = malloc(12); + q = realloc(q, 20); + return q; // no-warning +} + +void f6() { + int *p = malloc(12); + if (!p) + return; // no-warning + else + free(p); +} + +void f6_realloc() { + int *p = malloc(12); + if (!p) + return; // no-warning + else + realloc(p, 0); +} + +char *doit2(); +void pr6069() { + char *buf = doit2(); + free(buf); +} + +void pr6293() { + free(0); +} + +void f7() { + char *x = (char *)malloc(4); + free(x); + x[0] = 'a'; // expected-warning{{Use of memory after it is freed}} +} + +void f7_realloc() { + char *x = (char *)malloc(4); + realloc(x, 0); + x[0] = 'a'; // expected-warning{{Use of memory after it is freed}} +} + +void PR6123() { + int *x = malloc(11); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}} +} + +void PR7217() { + int *buf = malloc(2); // expected-warning{{Cast a region whose size is not a multiple of the destination type size}} + buf[1] = 'c'; // not crash +} + +void mallocCastToVoid() { + void *p = malloc(2); + const void *cp = p; // not crash + free(p); +} + +void mallocCastToFP() { + void *p = malloc(2); + void (*fp)() = p; // not crash + free(p); +} + +// This tests that malloc() buffers are undefined by default +char mallocGarbage() { + char *buf = malloc(2); + char result = buf[1]; // expected-warning{{undefined}} + free(buf); + return result; +} + +// This tests that calloc() buffers need to be freed +void callocNoFree() { + char *buf = calloc(2, 2); + return; // expected-warning{{Potential leak of memory pointed to by}} +} + +// These test that calloc() buffers are zeroed by default +char callocZeroesGood() { + char *buf = calloc(2, 2); + char result = buf[3]; // no-warning + if (buf[1] == 0) { + free(buf); + } + return result; // no-warning +} + +char callocZeroesBad() { + char *buf = calloc(2, 2); + char result = buf[3]; // no-warning + if (buf[1] != 0) { + free(buf); // expected-warning{{never executed}} + } + return result; // expected-warning{{Potential leak of memory pointed to by}} +} + +void testMultipleFreeAnnotations() { + int *p = malloc(12); + int *q = malloc(12); + my_freeBoth(p, q); +} Index: clang/test/Analysis/malloc-annotations-param.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/malloc-annotations-param.cpp @@ -0,0 +1,98 @@ +// RUN: %clang_analyze_cc1 -analyzer-store=region -verify \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=alpha.deadcode.UnreachableCode \ +// RUN: -analyzer-checker=alpha.core.CastSize \ +// RUN: -analyzer-checker=unix.Malloc \ +// RUN: -analyzer-config unix.DynamicMemoryModeling:Optimistic=true %s + +typedef __typeof(sizeof(int)) size_t; +void *malloc(size_t); +void free(void *); + +struct MemoryAllocator { + void __attribute((ownership_returns(malloc))) * my_malloc(size_t); + void __attribute((ownership_takes(malloc, 2))) my_free(void *); + void __attribute((ownership_holds(malloc, 2))) my_hold(void *); +}; + +void *myglobalpointer; + +struct stuff { + void *somefield; +}; + +struct stuff myglobalstuff; + +void af1(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + return; // expected-warning{{Potential leak of memory pointed to by}} +} + +void af1_b(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); +} // expected-warning{{Potential leak of memory pointed to by}} + +void af1_c(MemoryAllocator &Alloc) { + myglobalpointer = Alloc.my_malloc(12); // no-warning +} + +// Test that we can pass out allocated memory via pointer-to-pointer. +void af1_e(MemoryAllocator &Alloc, void **pp) { + *pp = Alloc.my_malloc(42); // no-warning +} + +void af1_f(MemoryAllocator &Alloc, struct stuff *somestuff) { + somestuff->somefield = Alloc.my_malloc(12); // no-warning +} + +// Allocating memory for a field via multiple indirections to our arguments is OK. +void af1_g(MemoryAllocator &Alloc, struct stuff **pps) { + *pps = (struct stuff *)Alloc.my_malloc(sizeof(struct stuff)); // no-warning + (*pps)->somefield = Alloc.my_malloc(42); // no-warning +} + +void af2(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + Alloc.my_free(p); + free(p); // expected-warning{{Attempt to free released memory}} +} + +void af2b(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + free(p); + Alloc.my_free(p); // expected-warning{{Attempt to free released memory}} +} + +void af2c(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + free(p); + Alloc.my_hold(p); // expected-warning{{Attempt to free released memory}} +} + +// No leak if malloc returns null. +void af2e(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + if (!p) + return; // no-warning + free(p); // no-warning +} + +// This case inflicts a possible double-free. +void af3(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + Alloc.my_hold(p); + free(p); // expected-warning{{Attempt to free non-owned memory}} +} + +void *af4(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + Alloc.my_free(p); + return p; // expected-warning{{Use of memory after it is freed}} +} + +// This case is (possibly) ok, be conservative +void *af5(MemoryAllocator &Alloc) { + void *p = Alloc.my_malloc(12); + Alloc.my_hold(p); + return p; // no-warning +} Index: clang/test/Analysis/malloc-annotations.c =================================================================== --- clang/test/Analysis/malloc-annotations.c +++ clang/test/Analysis/malloc-annotations.c @@ -271,5 +271,4 @@ int *p = malloc(12); int *q = malloc(12); my_freeBoth(p, q); -} - +} \ No newline at end of file