From d21139a34f5115b8cb95949194cfb40809bd0d17 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Sat, 31 Jul 2010 01:52:11 +0000 Subject: [PATCH] After a lengthy design discussion, add support for "ownership attributes" for malloc/free checking. Patch by Andrew McGregor! llvm-svn: 109939 --- clang/include/clang/AST/Attr.h | 115 ++++++++++++ clang/include/clang/Basic/Attr.td | 15 ++ .../clang/Basic/DiagnosticSemaKinds.td | 2 + clang/include/clang/Parse/AttributeList.h | 3 + clang/lib/AST/AttrImpl.cpp | 53 ++++++ clang/lib/Checker/MallocChecker.cpp | 173 +++++++++++++++-- clang/lib/Lex/PPMacroExpansion.cpp | 3 + clang/lib/Parse/AttributeList.cpp | 3 + clang/lib/Sema/SemaDeclAttr.cpp | 175 +++++++++++++++++- clang/test/Analysis/malloc.c | 122 +++++++++++- 10 files changed, 640 insertions(+), 24 deletions(-) diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h index f06c4ab25f09..5775b2be4781 100644 --- a/clang/include/clang/AST/Attr.h +++ b/clang/include/clang/AST/Attr.h @@ -369,6 +369,121 @@ public: static bool classof(const NonNullAttr *A) { return true; } }; +/// OwnershipAttr +/// Ownership attributes are used to annotate pointers that own a resource +/// in order for the analyzer to check correct allocation and deallocation. +/// There are three attributes, ownership_returns, ownership_holds and +/// ownership_takes, represented by subclasses of OwnershipAttr +class OwnershipAttr: public AttrWithString { + protected: + unsigned* ArgNums; + unsigned Size; +public: + enum OwnershipKind { Holds, Takes, Returns, None }; + OwnershipKind OKind; + attr::Kind AKind; +public: + OwnershipAttr(attr::Kind AK, ASTContext &C, unsigned* arg_nums, unsigned size, + llvm::StringRef module, OwnershipKind kind); + + + virtual void Destroy(ASTContext &C); + + OwnershipKind getKind() const { + return OKind; + } + bool isKind(const OwnershipKind k) const { + return OKind == k; + } + + /// Ownership attributes have a 'module', which is the name of a kind of + /// resource that can be checked. + /// The Malloc checker uses the module 'malloc'. + llvm::StringRef getModule() const { + return getString(); + } + void setModule(ASTContext &C, llvm::StringRef module) { + ReplaceString(C, module); + } + bool isModule(const char *m) const { + return getModule().equals(m); + } + + typedef const unsigned *iterator; + iterator begin() const { + return ArgNums; + } + iterator end() const { + return ArgNums + Size; + } + unsigned size() const { + return Size; + } + + virtual Attr *clone(ASTContext &C) const; + + static bool classof(const Attr *A) { + return true; + } + static bool classof(const OwnershipAttr *A) { + return true; + } +}; + +class OwnershipTakesAttr: public OwnershipAttr { +public: + OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums, unsigned size, + llvm::StringRef module); + + virtual Attr *clone(ASTContext &C) const; + + static bool classof(const Attr *A) { + return A->getKind() == attr::OwnershipTakes; + } + static bool classof(const OwnershipTakesAttr *A) { + return true; + } + static bool classof(const OwnershipAttr *A) { + return A->OKind == Takes; + } +}; + +class OwnershipHoldsAttr: public OwnershipAttr { +public: + OwnershipHoldsAttr(ASTContext &C, unsigned* arg_nums, unsigned size, + llvm::StringRef module); + + virtual Attr *clone(ASTContext &C) const; + + static bool classof(const Attr *A) { + return A->getKind() == attr::OwnershipHolds; + } + static bool classof(const OwnershipHoldsAttr *A) { + return true; + } + static bool classof(const OwnershipAttr *A) { + return A->OKind == Holds; + } +}; + +class OwnershipReturnsAttr: public OwnershipAttr { +public: + OwnershipReturnsAttr(ASTContext &C, unsigned* arg_nums, unsigned size, + llvm::StringRef module); + + virtual Attr *clone(ASTContext &C) const; + + static bool classof(const Attr *A) { + return A->getKind() == attr::OwnershipReturns; + } + static bool classof(const OwnershipReturnsAttr *A) { + return true; + } + static bool classof(const OwnershipAttr *A) { + return A->OKind == Returns; + } +}; + class FormatAttr : public AttrWithString { int formatIdx, firstArg; public: diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td index 98871d26204f..76eb9d487871 100644 --- a/clang/include/clang/Basic/Attr.td +++ b/clang/include/clang/Basic/Attr.td @@ -297,6 +297,21 @@ def Overloadable : Attr { let Spellings = ["overloadable"]; } +def OwnershipReturns : Attr { + let Spellings = ["ownership_returns"]; + let Args = [StringArgument<"Module">, IntArgument<"SizeIdx">]; +} + +def OwnershipTakes : Attr { + let Spellings = ["ownership_takes"]; + let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">]; +} + +def OwnershipHolds : Attr { + let Spellings = ["ownership_holds"]; + let Args = [StringArgument<"Module">, IntArgument<"PtrIdx">]; +} + def Packed : Attr { let Spellings = ["packed"]; } diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 99d8eefe2d2b..d30f8139d905 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -838,6 +838,8 @@ def err_attribute_requires_objc_interface : Error< "attribute may only be applied to an Objective-C interface">; def err_nonnull_pointers_only : Error< "nonnull attribute only applies to pointer arguments">; +def err_ownership_type : Error< + "%0 attribute only applies to %1 arguments">; def err_format_strftime_third_parameter : Error< "strftime format attribute requires 3rd parameter to be 0">; def err_format_attribute_requires_variadic : Error< diff --git a/clang/include/clang/Parse/AttributeList.h b/clang/include/clang/Parse/AttributeList.h index b60a94025e09..2b1bd1e63d7f 100644 --- a/clang/include/clang/Parse/AttributeList.h +++ b/clang/include/clang/Parse/AttributeList.h @@ -97,6 +97,9 @@ public: AT_ns_returns_retained, // Clang-specific. AT_objc_gc, AT_overloadable, // Clang-specific. + AT_ownership_holds, // Clang-specific. + AT_ownership_returns, // Clang-specific. + AT_ownership_takes, // Clang-specific. AT_packed, AT_pure, AT_regparm, diff --git a/clang/lib/AST/AttrImpl.cpp b/clang/lib/AST/AttrImpl.cpp index 7277bbce24bb..4146248fb039 100644 --- a/clang/lib/AST/AttrImpl.cpp +++ b/clang/lib/AST/AttrImpl.cpp @@ -48,6 +48,43 @@ NonNullAttr::NonNullAttr(ASTContext &C, unsigned* arg_nums, unsigned size) memcpy(ArgNums, arg_nums, sizeof(*ArgNums)*size); } +OwnershipAttr::OwnershipAttr(attr::Kind AK, ASTContext &C, unsigned* arg_nums, + unsigned size, + llvm::StringRef module, + OwnershipKind kind) : + AttrWithString(AK, C, module), ArgNums(0), Size(0), OKind(kind) { + if (size == 0) + return; + assert(arg_nums); + ArgNums = new (C) unsigned[size]; + Size = size; + AKind = AK; + OKind = kind; + memcpy(ArgNums, arg_nums, sizeof(*ArgNums) * size); +} + + +void OwnershipAttr::Destroy(ASTContext &C) { + if (ArgNums) + C.Deallocate(ArgNums); +} + +OwnershipTakesAttr::OwnershipTakesAttr(ASTContext &C, unsigned* arg_nums, + unsigned size, llvm::StringRef module) : + OwnershipAttr(attr::OwnershipTakes, C, arg_nums, size, module, Takes) { +} + +OwnershipHoldsAttr::OwnershipHoldsAttr(ASTContext &C, unsigned* arg_nums, + unsigned size, llvm::StringRef module) : + OwnershipAttr(attr::OwnershipHolds, C, arg_nums, size, module, Holds) { +} + +OwnershipReturnsAttr::OwnershipReturnsAttr(ASTContext &C, unsigned* arg_nums, + unsigned size, + llvm::StringRef module) : + OwnershipAttr(attr::OwnershipReturns, C, arg_nums, size, module, Returns) { +} + #define DEF_SIMPLE_ATTR_CLONE(ATTR) \ Attr *ATTR##Attr::clone(ASTContext &C) const { \ return ::new (C) ATTR##Attr; \ @@ -147,6 +184,22 @@ Attr *NonNullAttr::clone(ASTContext &C) const { return ::new (C) NonNullAttr(C, ArgNums, Size); } +Attr *OwnershipAttr::clone(ASTContext &C) const { + return ::new (C) OwnershipAttr(AKind, C, ArgNums, Size, getModule(), OKind); +} + +Attr *OwnershipReturnsAttr::clone(ASTContext &C) const { + return ::new (C) OwnershipReturnsAttr(C, ArgNums, Size, getModule()); +} + +Attr *OwnershipTakesAttr::clone(ASTContext &C) const { + return ::new (C) OwnershipTakesAttr(C, ArgNums, Size, getModule()); +} + +Attr *OwnershipHoldsAttr::clone(ASTContext &C) const { + return ::new (C) OwnershipHoldsAttr(C, ArgNums, Size, getModule()); +} + Attr *FormatAttr::clone(ASTContext &C) const { return ::new (C) FormatAttr(C, getType(), formatIdx, firstArg); } diff --git a/clang/lib/Checker/MallocChecker.cpp b/clang/lib/Checker/MallocChecker.cpp index dcc21ca38619..4ed309513e3f 100644 --- a/clang/lib/Checker/MallocChecker.cpp +++ b/clang/lib/Checker/MallocChecker.cpp @@ -24,15 +24,17 @@ using namespace clang; namespace { class RefState { - enum Kind { AllocateUnchecked, AllocateFailed, Released, Escaped } K; + enum Kind { AllocateUnchecked, AllocateFailed, Released, Escaped, Relinquished } K; const Stmt *S; public: RefState(Kind k, const Stmt *s) : K(k), S(s) {} bool isAllocated() const { return K == AllocateUnchecked; } + bool isFailed() const { return K == AllocateFailed; } bool isReleased() const { return K == Released; } bool isEscaped() const { return K == Escaped; } + bool isRelinquished() const { return K == Relinquished; } bool operator==(const RefState &X) const { return K == X.K && S == X.S; @@ -46,6 +48,7 @@ public: } static RefState getReleased(const Stmt *s) { return RefState(Released, s); } static RefState getEscaped(const Stmt *s) { return RefState(Escaped, s); } + static RefState getRelinquished(const Stmt *s) { return RefState(Relinquished, s); } void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); @@ -59,12 +62,13 @@ class MallocChecker : public CheckerVisitor { BuiltinBug *BT_DoubleFree; BuiltinBug *BT_Leak; BuiltinBug *BT_UseFree; + BuiltinBug *BT_UseRelinquished; BuiltinBug *BT_BadFree; IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc; public: MallocChecker() - : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_BadFree(0), + : BT_DoubleFree(0), BT_Leak(0), BT_UseFree(0), BT_UseRelinquished(0), BT_BadFree(0), II_malloc(0), II_free(0), II_realloc(0), II_calloc(0) {} static void *getTag(); bool EvalCallExpr(CheckerContext &C, const CallExpr *CE); @@ -73,9 +77,13 @@ public: void PreVisitReturnStmt(CheckerContext &C, const ReturnStmt *S); const GRState *EvalAssume(const GRState *state, SVal Cond, bool Assumption); void VisitLocation(CheckerContext &C, const Stmt *S, SVal l); + virtual void PreVisitBind(CheckerContext &C, const Stmt *AssignE, + const Stmt *StoreE, SVal location, + SVal val); private: void MallocMem(CheckerContext &C, const CallExpr *CE); + void MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE, const OwnershipAttr* Att); const GRState *MallocMemAux(CheckerContext &C, const CallExpr *CE, const Expr *SizeEx, SVal Init, const GRState *state) { @@ -86,8 +94,9 @@ private: const GRState *state); void FreeMem(CheckerContext &C, const CallExpr *CE); + void FreeMemAttr(CheckerContext &C, const CallExpr *CE, const OwnershipAttr* Att); const GRState *FreeMemAux(CheckerContext &C, const CallExpr *CE, - const GRState *state); + const GRState *state, unsigned Num, bool Hold); void ReallocMem(CheckerContext &C, const CallExpr *CE); void CallocMem(CheckerContext &C, const CallExpr *CE); @@ -156,7 +165,31 @@ bool MallocChecker::EvalCallExpr(CheckerContext &C, const CallExpr *CE) { return true; } - return false; + // Check all the attributes, if there are any. + // There can be multiple of these attributes. + bool rv = false; + if (FD->hasAttrs()) { + for (const Attr *attr = FD->getAttrs(); attr; attr = attr->getNext()) { + if(const OwnershipAttr* Att = dyn_cast(attr)) { + switch (Att->getKind()) { + case OwnershipAttr::Returns: { + MallocMemReturnsAttr(C, CE, Att); + rv = true; + break; + } + case OwnershipAttr::Takes: + case OwnershipAttr::Holds: { + FreeMemAttr(C, CE, Att); + rv = true; + break; + } + default: + break; + } + } + } + } + return rv; } void MallocChecker::MallocMem(CheckerContext &C, const CallExpr *CE) { @@ -165,6 +198,23 @@ void MallocChecker::MallocMem(CheckerContext &C, const CallExpr *CE) { C.addTransition(state); } +void MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE, + const OwnershipAttr* Att) { + if (!Att->isModule("malloc")) + return; + + const unsigned *I = Att->begin(), *E = Att->end(); + if (I != E) { + const GRState *state = + MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(), C.getState()); + C.addTransition(state); + return; + } + const GRState *state = MallocMemAux(C, CE, UnknownVal(), UndefinedVal(), + C.getState()); + C.addTransition(state); +} + const GRState *MallocChecker::MallocMemAux(CheckerContext &C, const CallExpr *CE, SVal Size, SVal Init, @@ -196,25 +246,52 @@ const GRState *MallocChecker::MallocMemAux(CheckerContext &C, } void MallocChecker::FreeMem(CheckerContext &C, const CallExpr *CE) { - const GRState *state = FreeMemAux(C, CE, C.getState()); + const GRState *state = FreeMemAux(C, CE, C.getState(), 0, false); if (state) C.addTransition(state); } +void MallocChecker::FreeMemAttr(CheckerContext &C, const CallExpr *CE, + const OwnershipAttr* Att) { + if (!Att->isModule("malloc")) + return; + + for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) { + const GRState *state = + FreeMemAux(C, CE, C.getState(), *I, Att->isKind(OwnershipAttr::Holds)); + if (state) + C.addTransition(state); + } +} + const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE, - const GRState *state) { - const Expr *ArgExpr = CE->getArg(0); + const GRState *state, unsigned Num, + bool Hold) { + const Expr *ArgExpr = CE->getArg(Num); SVal ArgVal = state->getSVal(ArgExpr); - // If ptr is NULL, no operation is preformed. - if (ArgVal.isZeroConstant()) + DefinedOrUnknownSVal location = cast(ArgVal); + + // Check for null dereferences. + if (!isa(location)) return state; - + + // FIXME: Technically using 'Assume' here can result in a path + // bifurcation. In such cases we need to return two states, not just one. + const GRState *notNullState, *nullState; + llvm::tie(notNullState, nullState) = state->Assume(location); + + // The explicit NULL case, no operation is performed. + if (nullState && !notNullState) + return nullState; + + assert(notNullState); + // Unknown values could easily be okay // Undefined values are handled elsewhere if (ArgVal.isUnknownOrUndef()) - return state; + return notNullState; const MemRegion *R = ArgVal.getAsRegion(); @@ -253,7 +330,7 @@ const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE, // Various cases could lead to non-symbol values here. // For now, ignore them. if (!SR) - return state; + return notNullState; SymbolRef Sym = SR->getSymbol(); @@ -263,14 +340,15 @@ const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE, // called on a pointer that does not get its pointee directly from malloc(). // Full support of this requires inter-procedural analysis. if (!RS) - return state; + return notNullState; // Check double free. if (RS->isReleased()) { ExplodedNode *N = C.GenerateSink(); if (N) { if (!BT_DoubleFree) - BT_DoubleFree = new BuiltinBug("Double free", + BT_DoubleFree + = new BuiltinBug("Double free", "Try to free a memory block that has been released"); // FIXME: should find where it's freed last time. BugReport *R = new BugReport(*BT_DoubleFree, @@ -281,7 +359,9 @@ const GRState *MallocChecker::FreeMemAux(CheckerContext &C, const CallExpr *CE, } // Normal free. - return state->set(Sym, RefState::getReleased(CE)); + if (Hold) + return notNullState->set(Sym, RefState::getRelinquished(CE)); + return notNullState->set(Sym, RefState::getReleased(CE)); } bool MallocChecker::SummarizeValue(llvm::raw_ostream& os, SVal V) { @@ -446,13 +526,13 @@ void MallocChecker::ReallocMem(CheckerContext &C, const CallExpr *CE) { ValMgr.makeIntValWithPtrWidth(0, false)); if (const GRState *stateSizeZero = stateNotEqual->Assume(SizeZero, true)) { - const GRState *stateFree = FreeMemAux(C, CE, stateSizeZero); + const GRState *stateFree = FreeMemAux(C, CE, stateSizeZero, 0, false); if (stateFree) C.addTransition(stateFree->BindExpr(CE, UndefinedVal(), true)); } if (const GRState *stateSizeNotZero=stateNotEqual->Assume(SizeZero,false)) { - const GRState *stateFree = FreeMemAux(C, CE, stateSizeNotZero); + const GRState *stateFree = FreeMemAux(C, CE, stateSizeNotZero, 0, false); if (stateFree) { // FIXME: We should copy the content of the original buffer. const GRState *stateRealloc = MallocMemAux(C, CE, CE->getArg(1), @@ -581,3 +661,62 @@ void MallocChecker::VisitLocation(CheckerContext &C, const Stmt *S, SVal l) { } } } + +void MallocChecker::PreVisitBind(CheckerContext &C, + const Stmt *AssignE, + const Stmt *StoreE, + SVal location, + SVal val) { + // The PreVisitBind implements the same algorithm as already used by the + // Objective C ownership checker: if the pointer escaped from this scope by + // assignment, let it go. However, assigning to fields of a stack-storage + // structure does not transfer ownership. + + const GRState *state = C.getState(); + DefinedOrUnknownSVal l = cast(location); + + // Check for null dereferences. + if (!isa(l)) + return; + + // Before checking if the state is null, check if 'val' has a RefState. + // Only then should we check for null and bifurcate the state. + SymbolRef Sym = val.getLocSymbolInBase(); + if (Sym) { + if (const RefState *RS = state->get(Sym)) { + // If ptr is NULL, no operation is performed. + const GRState *notNullState, *nullState; + llvm::tie(notNullState, nullState) = state->Assume(l); + + // Generate a transition for 'nullState' to record the assumption + // that the state was null. + if (nullState) + C.addTransition(nullState); + + if (!notNullState) + return; + + if (RS->isAllocated()) { + // Something we presently own is being assigned somewhere. + const MemRegion *AR = location.getAsRegion(); + if (!AR) + return; + AR = AR->StripCasts()->getBaseRegion(); + do { + // If it is on the stack, we still own it. + if (AR->hasStackNonParametersStorage()) + break; + + // If the state can't represent this binding, we still own it. + if (notNullState == (notNullState->bindLoc(cast(location), UnknownVal()))) + break; + + // We no longer own this pointer. + notNullState = notNullState->set(Sym, RefState::getRelinquished(StoreE)); + } + while (false); + } + C.addTransition(notNullState); + } + } +} diff --git a/clang/lib/Lex/PPMacroExpansion.cpp b/clang/lib/Lex/PPMacroExpansion.cpp index ebf606e9406f..ad05b40e42a8 100644 --- a/clang/lib/Lex/PPMacroExpansion.cpp +++ b/clang/lib/Lex/PPMacroExpansion.cpp @@ -506,6 +506,9 @@ static bool HasFeature(const Preprocessor &PP, const IdentifierInfo *II) { .Case("cxx_static_assert", LangOpts.CPlusPlus0x) .Case("objc_nonfragile_abi", LangOpts.ObjCNonFragileABI) .Case("objc_weak_class", LangOpts.ObjCNonFragileABI) + .Case("ownership_holds", true) + .Case("ownership_returns", true) + .Case("ownership_takes", true) //.Case("cxx_concepts", false) //.Case("cxx_lambdas", false) //.Case("cxx_nullptr", false) diff --git a/clang/lib/Parse/AttributeList.cpp b/clang/lib/Parse/AttributeList.cpp index 98d5d07e151e..dc94420bef03 100644 --- a/clang/lib/Parse/AttributeList.cpp +++ b/clang/lib/Parse/AttributeList.cpp @@ -118,6 +118,9 @@ AttributeList::Kind AttributeList::getKind(const IdentifierInfo *Name) { .Case("ns_returns_retained", AT_ns_returns_retained) .Case("cf_returns_not_retained", AT_cf_returns_not_retained) .Case("cf_returns_retained", AT_cf_returns_retained) + .Case("ownership_returns", AT_ownership_returns) + .Case("ownership_holds", AT_ownership_holds) + .Case("ownership_takes", AT_ownership_takes) .Case("reqd_work_group_size", AT_reqd_wg_size) .Case("init_priority", AT_init_priority) .Case("no_instrument_function", AT_no_instrument_function) diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 3b82f58be43e..cebe6e7850a1 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -346,10 +346,175 @@ static void HandleNonNullAttr(Decl *d, const AttributeList &Attr, Sema &S) { unsigned* start = &NonNullArgs[0]; unsigned size = NonNullArgs.size(); - std::sort(start, start + size); + llvm::array_pod_sort(start, start + size); d->addAttr(::new (S.Context) NonNullAttr(S.Context, start, size)); } +static void HandleOwnershipAttr(Decl *d, const AttributeList &AL, Sema &S) { + // This attribute must be applied to a function declaration. + // The first argument to the attribute must be a string, + // the name of the resource, for example "malloc". + // The following arguments must be argument indexes, the arguments must be + // of integer type for Returns, otherwise of pointer type. + // The difference between Holds and Takes is that a pointer may still be used + // after being held. free() should be __attribute((ownership_takes)), whereas a list + // append function may well be __attribute((ownership_holds)). + + if (!AL.getParameterName()) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_n_not_string) + << AL.getName()->getName() << 1; + return; + } + // Figure out our Kind, and check arguments while we're at it. + OwnershipAttr::OwnershipKind K = OwnershipAttr::None; + if (AL.getName()->getName().equals("ownership_takes")) { + K = OwnershipAttr::Takes; + if (AL.getNumArgs() < 1) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2; + return; + } + } else if (AL.getName()->getName().equals("ownership_holds")) { + K = OwnershipAttr::Holds; + if (AL.getNumArgs() < 1) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2; + return; + } + } else if (AL.getName()->getName().equals("ownership_returns")) { + K = OwnershipAttr::Returns; + if (AL.getNumArgs() > 1) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) + << AL.getNumArgs() + 1; + return; + } + } + // This should never happen given how we are called. + if (K == OwnershipAttr::None) { + return; + } + + if (!isFunction(d) || !hasFunctionProto(d)) { + S.Diag(AL.getLoc(), diag::warn_attribute_wrong_decl_type) << AL.getName() + << 0 /*function*/; + return; + } + + unsigned NumArgs = getFunctionOrMethodNumArgs(d); + + llvm::StringRef Module = AL.getParameterName()->getName(); + + // Normalize the argument, __foo__ becomes foo. + if (Module.startswith("__") && Module.endswith("__")) + Module = Module.substr(2, Module.size() - 4); + + llvm::SmallVector OwnershipArgs; + + for (AttributeList::arg_iterator I = AL.arg_begin(), E = AL.arg_end(); I != E; ++I) { + + Expr *IdxExpr = static_cast(*I); + llvm::APSInt ArgNum(32); + if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent() + || !IdxExpr->isIntegerConstantExpr(ArgNum, S.Context)) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_not_int) + << AL.getName()->getName() << IdxExpr->getSourceRange(); + continue; + } + + unsigned x = (unsigned) ArgNum.getZExtValue(); + + if (x > NumArgs || x < 1) { + S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_bounds) + << AL.getName()->getName() << x << IdxExpr->getSourceRange(); + continue; + } + --x; + switch (K) { + case OwnershipAttr::Takes: + case OwnershipAttr::Holds: { + // Is the function argument a pointer type? + QualType T = getFunctionOrMethodArgType(d, x); + if (!T->isAnyPointerType() && !T->isBlockPointerType()) { + // FIXME: Should also highlight argument in decl. + S.Diag(AL.getLoc(), diag::err_ownership_type) + << ((K==OwnershipAttr::Takes)?"ownership_takes":"ownership_holds") + << "pointer" + << IdxExpr->getSourceRange(); + continue; + } + break; + } + case OwnershipAttr::Returns: { + if (AL.getNumArgs() > 1) { + // Is the function argument an integer type? + Expr *IdxExpr = static_cast(AL.getArg(0)); + llvm::APSInt ArgNum(32); + if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent() + || !IdxExpr->isIntegerConstantExpr(ArgNum, S.Context)) { + S.Diag(AL.getLoc(), diag::err_ownership_type) + << "ownership_returns" << "integer" + << IdxExpr->getSourceRange(); + return; + } + } + break; + } + // Should never happen, here to silence a warning. + default: { + return; + } + } // switch + + // Check we don't have a conflict with another ownership attribute. + if (d->hasAttrs()) { + for (const Attr *attr = d->getAttrs(); attr; attr = attr->getNext()) { + if (const OwnershipAttr* Att = dyn_cast(attr)) { + // Two ownership attributes of the same kind can't conflict, + // except returns attributes. + if (K == OwnershipAttr::Returns || Att->getKind() != K) { + for (const unsigned *I = Att->begin(), *E = Att->end(); I != E; ++I) { + if (x == *I) { + S.Diag(AL.getLoc(), diag::err_attributes_are_not_compatible) + << AL.getName()->getName() << "ownership_*"; + } + } + } + } + } + } + OwnershipArgs.push_back(x); + } + + unsigned* start = OwnershipArgs.data(); + unsigned size = OwnershipArgs.size(); + llvm::array_pod_sort(start, start + size); + switch (K) { + case OwnershipAttr::Takes: { + if (OwnershipArgs.empty()) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2; + return; + } + d->addAttr(::new (S.Context) OwnershipTakesAttr(S.Context, start, size, + Module)); + break; + } + case OwnershipAttr::Holds: { + if (OwnershipArgs.empty()) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_number_arguments) << 2; + return; + } + d->addAttr(::new (S.Context) OwnershipHoldsAttr(S.Context, start, size, + Module)); + break; + } + case OwnershipAttr::Returns: { + d->addAttr(::new (S.Context) OwnershipReturnsAttr(S.Context, start, size, + Module)); + break; + } + default: + break; + } +} + static bool isStaticVarOrStaticFunciton(Decl *D) { if (VarDecl *VD = dyn_cast(D)) return VD->getStorageClass() == VarDecl::Static; @@ -379,7 +544,7 @@ static void HandleWeakRefAttr(Decl *d, const AttributeList &Attr, Sema &S) { Ctx = Ctx->getLookupContext(); if (!isa(Ctx) && !isa(Ctx) ) { S.Diag(Attr.getLoc(), diag::err_attribute_weakref_not_global_context) << - dyn_cast(d)->getNameAsString(); + dyn_cast(d)->getNameAsString(); return; } } @@ -2009,6 +2174,10 @@ static void ProcessDeclAttribute(Scope *scope, Decl *D, case AttributeList::AT_mode: HandleModeAttr (D, Attr, S); break; case AttributeList::AT_malloc: HandleMallocAttr (D, Attr, S); break; case AttributeList::AT_nonnull: HandleNonNullAttr (D, Attr, S); break; + case AttributeList::AT_ownership_returns: + case AttributeList::AT_ownership_takes: + case AttributeList::AT_ownership_holds: + HandleOwnershipAttr (D, Attr, S); break; case AttributeList::AT_noreturn: HandleNoReturnAttr (D, Attr, S); break; case AttributeList::AT_nothrow: HandleNothrowAttr (D, Attr, S); break; case AttributeList::AT_override: HandleOverrideAttr (D, Attr, S); break; @@ -2087,7 +2256,7 @@ void Sema::ProcessDeclAttributeList(Scope *S, Decl *D, const AttributeList *Attr // but that looks really pointless. We reject it. if (D->hasAttr() && !D->hasAttr()) { Diag(AttrList->getLoc(), diag::err_attribute_weakref_without_alias) << - dyn_cast(D)->getNameAsString(); + dyn_cast(D)->getNameAsString(); return; } } diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c index 91974f672584..e443150e1fbb 100644 --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -4,22 +4,136 @@ 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 __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc, 1))) *my_malloc2(size_t); +void __attribute((ownership_holds(malloc, 1))) my_hold(void *); + +// 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{{Allocated memory never released. Potential memory leak.}} } -void f1_b() { - int *p = malloc(12); // expected-warning{{Allocated memory never released. Potential memory leak.}} -} - void f2() { int *p = malloc(12); free(p); free(p); // expected-warning{{Try to free a memory block that has been released}} } +// ownership attributes tests +void naf1() { + int *p = my_malloc3(12); + return; // no-warning +} + +void n2af1() { + int *p = my_malloc2(12); + return; // expected-warning{{Allocated memory never released. Potential memory leak.}} +} + +void af1() { + int *p = my_malloc(12); + return; // expected-warning{{Allocated memory never released. Potential memory leak.}} +} + +void af1_b() { + int *p = my_malloc(12); // expected-warning{{Allocated memory never released. Potential memory leak.}} +} + +void af1_c() { + myglobalpointer = my_malloc(12); // no-warning +} + +void af1_d() { + struct stuff mystuff; + mystuff.somefield = my_malloc(12); // expected-warning{{Allocated memory never released. Potential memory leak.}} +} + +// 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{{Try to free a memory block that has been released}} +} + +void af2b() { + int *p = my_malloc(12); + free(p); + my_free(p); // expected-warning{{Try to free a memory block that has been released}} +} + +void af2c() { + int *p = my_malloc(12); + free(p); + my_hold(p); // expected-warning{{Try to free a memory block that has been released}} +} + +void af2d() { + int *p = my_malloc(12); + free(p); + my_hold2(0, 0, p); // expected-warning{{Try to free a memory block that has been released}} +} + +// No leak if malloc returns null. +void af2e() { + int *p = my_malloc(12); + if (!p) + return; // no-warning + free(p); // no-warning +} + +// This case would inflict a double-free elsewhere. +// However, this case is considered an analyzer bug since it causes false-positives. +void af3() { + int *p = my_malloc(12); + my_hold(p); + free(p); // no-warning +} + +// This case would inflict a double-free elsewhere. +// However, this case is considered an analyzer bug since it causes false-positives. +int * af4() { + int *p = my_malloc(12); + my_free(p); + return p; // no-warning +} + +// 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. -- GitLab