chrisdangelo updated this revision to Diff 395726.
chrisdangelo edited the summary of this revision.
chrisdangelo added a comment.
This change removes the previous ownership_takes_param work. This change
augments the existing Ownership attribute so that it is now applicable to
function parameters.
Theses changes have been used to run the analyzer on a large C / C++ project.
The tests have been run locally and successfully with `ninja
check-clang-analysis` and `ninja check-clang`.
This diff is functional but is not fully complete. As of this iteration, these
are the current tasks unresolved...
1. This change does not fully validate correct usage in SemaDeclAttr.cpp. More
can be done to validate that ownership attributes are not used in ways that are
in conflict with one another. This conflict can occur with a combination of
ownership attributes applied to the function declaration simultaneously with
parameter annotations.
2. We are interested in deprecating the use of function based ownership
attributes in favor of using function parameter annotations. This change does
not currently provide deprecation warnings.
3. This change does not test semantic analysis of ownership parameters
(malloc-annotations.c).
4. This change does not fully test static analysis of ownership parameters
(attr-ownership.c/cpp).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113530/new/
https://reviews.llvm.org/D113530
Files:
clang/include/clang/Basic/Attr.td
clang/lib/Sema/SemaDeclAttr.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/test/Analysis/malloc-annotations.c
Index: clang/test/Analysis/malloc-annotations.c
===================================================================
--- clang/test/Analysis/malloc-annotations.c
+++ clang/test/Analysis/malloc-annotations.c
@@ -16,6 +16,10 @@
__attribute((ownership_holds(malloc, 1, 2)));
void __attribute((ownership_returns(malloc, 1))) *my_malloc2(size_t);
void __attribute((ownership_holds(malloc, 1))) my_hold(void *);
+void my_free_first_parameter(void * __attribute__((ownership_takes(malloc))));
+void my_free_second_parameter(void *, void * __attribute__((ownership_takes(malloc))));
+void my_free_two_parameters(void * __attribute__((ownership_takes(malloc))), void * __attribute__((ownership_takes(malloc))));
+void __attribute((ownership_takes(malloc, 1))) my_free_both_params_via_param_and_func(void *, void * __attribute__((ownership_takes(malloc))));
// Duplicate attributes are silly, but not an error.
// Duplicate attribute has no extra effect.
@@ -273,3 +277,49 @@
my_freeBoth(p, q);
}
+int *testFreeFirstParameterUseAfterFree() {
+ int *p = malloc(4);
+
+ my_free_first_parameter(p);
+ return p; // expected-warning{{Use of memory after it is freed}}
+}
+
+int *testFreeSecondParameterUseAfterFree() {
+ int *p1 = malloc(4);
+ int *p2 = malloc(4);
+
+ my_free_second_parameter(p1, p2);
+ return p2; // expected-warning{{Use of memory after it is freed}}
+}
+
+int *testFreeSecondParameterNoUseAfterFree() {
+ int *p1 = malloc(4);
+ int *p2 = malloc(4);
+
+ my_free_second_parameter(p1, p2);
+ return p1;
+}
+
+int *testFreeFirstOfTwoParametersUseAfterFree() {
+ int *p1 = malloc(4);
+ int *p2 = malloc(4);
+
+ my_free_two_parameters(p1, p2);
+ return p1; // expected-warning{{Use of memory after it is freed}}
+}
+
+int *testFreeSecondOfTwoParametersUseAfterFree() {
+ int *p1 = malloc(4);
+ int *p2 = malloc(4);
+
+ my_free_two_parameters(p1, p2);
+ return p2; // expected-warning{{Use of memory after it is freed}}
+}
+
+int *testAuthorMayUseFuncDeclAndParamOwnershipAttr() {
+ int *p1 = malloc(4);
+ int *p2 = malloc(4);
+
+ my_free_both_params_via_param_and_func(p1, p2);
+ return p2; // expected-warning{{Use of memory after it is freed}}
+}
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<BlockExpr>,
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,18 @@
/// if the macro value could be determined, and if yes the value itself.
mutable Optional<KernelZeroSizePtrValueTy> 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
@@ -1080,7 +1088,7 @@
ReallocatingMemFnMap.lookup(Call))
return true;
- if (!ShouldIncludeOwnershipAnnotatedFunctions)
+ if (!IsOptimisticAnalyzerOptionEnabled)
return false;
const auto *Func = dyn_cast<FunctionDecl>(Call.getDecl());
@@ -1396,33 +1404,109 @@
C.addTransition(State);
}
-void MallocChecker::checkOwnershipAttr(const CallEvent &Call,
- CheckerContext &C) const {
+static const FunctionDecl *functionDeclForCall(const CallEvent &Call,
+ CheckerContext &C) {
ProgramStateRef State = C.getState();
const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
if (!CE)
- return;
+ return nullptr;
+
const FunctionDecl *FD = C.getCalleeDecl(CE);
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<OwnershipAttr>()) {
- 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;
- }
+ return nullptr;
+
+ return FD;
+}
+
+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<OwnershipAttr>()) {
+ ProgramStateRef AttrS = nullptr;
+
+ switch (I->getOwnKind()) {
+ case OwnershipAttr::Returns:
+ AttrS = MallocMemReturnsAttr(C, Call, I, S);
+ break;
+ case OwnershipAttr::Takes:
+ case OwnershipAttr::Holds:
+ AttrS = FreeMemAttr(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<OwnershipAttr>()) {
+ 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;
+ }
}
- C.addTransition(State);
+
+ return S;
+}
+
+void MallocChecker::checkOwnershipAttr(const CallEvent &Call,
+ CheckerContext &C) const {
+ bool IsEnabled = IsOptimisticAnalyzerOptionEnabled ||
+ ChecksEnabled[CK_MismatchedDeallocatorChecker];
+ if (!IsEnabled)
+ return;
+
+ ProgramStateRef S = C.getState();
+ if (!S)
+ return;
+
+ const FunctionDecl *FD = functionDeclForCall(Call, C);
+ if (!FD)
+ return;
+
+ S = ownershipFuncAttr(Call, C, FD, S);
+ S = ownershipParamAttr(Call, C, FD, S);
+
+ C.addTransition(S);
}
void MallocChecker::checkPostCall(const CallEvent &Call,
@@ -3562,7 +3646,7 @@
void ento::registerDynamicMemoryModeling(CheckerManager &mgr) {
auto *checker = mgr.registerChecker<MallocChecker>();
- checker->ShouldIncludeOwnershipAnnotatedFunctions =
+ checker->IsOptimisticAnalyzerOptionEnabled =
mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "Optimistic");
checker->ShouldRegisterNoOwnershipChangeVisitor =
mgr.getAnalyzerOptions().getCheckerBooleanOption(
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<ParamIdx, 8> 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<ParmVarDecl>(D))
+ handleOwnershipParamAttr(S, PVD, AL);
+ else
+ handleOwnershipFuncAttr(S, D, AL);
break;
case ParsedAttr::AT_Naked:
handleNakedAttr(S, D, AL);
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];
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
