chrisdangelo updated this revision to Diff 395734.
chrisdangelo added a comment.
This change includes clang-format edits.
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,16 @@
/// 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 +1086,7 @@
ReallocatingMemFnMap.lookup(Call))
return true;
- if (!ShouldIncludeOwnershipAnnotatedFunctions)
+ if (!IsOptimisticAnalyzerOptionEnabled)
return false;
const auto *Func = dyn_cast<FunctionDecl>(Call.getDecl());
@@ -1396,33 +1402,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 +3644,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
