[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
via cfe-commits
cfe-commits at lists.llvm.org
Mon May 13 09:25:47 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: Dana Jansens (danakj)
<details>
<summary>Changes</summary>
This is built on top of https://github.com/llvm/llvm-project/pull/91777, and includes the commits there for now.
---
Patch is 28.06 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91991.diff
5 Files Affected:
- (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h (+5)
- (modified) clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def (+1)
- (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+183-91)
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+26-4)
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp (+105)
``````````diff
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) = 0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0, arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..c7b4fcdce2132 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation getSourceLoc() const = 0;
/// Returns the list of pointer-type variables on which this gadget performs
/// its operation. Typically, there's only one variable. This isn't a list
@@ -513,6 +513,10 @@ class WarningGadget : public Gadget {
static bool classof(const Gadget *G) { return G->isWarningGadget(); }
bool isWarningGadget() const final { return true; }
+
+ virtual void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const = 0;
};
/// Fixable gadgets correspond to code patterns that aren't always unsafe but
@@ -572,7 +576,12 @@ class IncrementGadget : public WarningGadget {
.bind(OpTag));
}
- const UnaryOperator *getBaseStmt() const override { return Op; }
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override {
SmallVector<const DeclRefExpr *, 2> Uses;
@@ -607,7 +616,12 @@ class DecrementGadget : public WarningGadget {
.bind(OpTag));
}
- const UnaryOperator *getBaseStmt() const override { return Op; }
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE =
@@ -648,7 +662,12 @@ class ArraySubscriptGadget : public WarningGadget {
// clang-format on
}
- const ArraySubscriptExpr *getBaseStmt() const override { return ASE; }
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(ASE, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return ASE->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE =
@@ -696,7 +715,12 @@ class PointerArithmeticGadget : public WarningGadget {
.bind(PointerArithmeticTag));
}
- const Stmt *getBaseStmt() const override { return PA; }
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(PA, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return PA->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE = dyn_cast<DeclRefExpr>(Ptr->IgnoreParenImpCasts())) {
@@ -734,7 +758,12 @@ class SpanTwoParamConstructorGadget : public WarningGadget {
.bind(SpanTwoParamConstructorTag));
}
- const Stmt *getBaseStmt() const override { return Ctor; }
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperationInContainer(Ctor, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Ctor->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override {
// If the constructor call is of the form `std::span{var, n}`, `var` is
@@ -780,11 +809,8 @@ class PointerInitGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override {
- // FIXME: This needs to be the entire DeclStmt, assuming that this method
- // makes sense at all on a FixableGadget.
- return PtrInitRHS;
+ SourceLocation getSourceLoc() const override {
+ return PtrInitRHS->getBeginLoc();
}
virtual DeclUseList getClaimedVarUseSites() const override {
@@ -833,12 +859,7 @@ class PtrToPtrAssignmentGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override {
- // FIXME: This should be the binary operator, assuming that this method
- // makes sense at all on a FixableGadget.
- return PtrLHS;
- }
+ SourceLocation getSourceLoc() const override { return PtrLHS->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrLHS, PtrRHS};
@@ -888,12 +909,7 @@ class CArrayToPtrAssignmentGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override {
- // FIXME: This should be the binary operator, assuming that this method
- // makes sense at all on a FixableGadget.
- return PtrLHS;
- }
+ SourceLocation getSourceLoc() const override { return PtrLHS->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
return DeclUseList{PtrLHS, PtrRHS};
@@ -921,10 +937,55 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
}
static Matcher matcher() {
- return stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage))))
- .bind(OpTag));
+ auto HasUnsafeFnDecl =
+ callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
+ return stmt(callExpr(HasUnsafeFnDecl).bind(OpTag));
+ }
+
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
+
+ DeclUseList getClaimedVarUseSites() const override { return {}; }
+};
+
+/// A call of a constructor that performs unchecked buffer operations
+/// over one of its pointer parameters, or constructs a class object that will
+/// perform buffer operations that depend on the correctness of the parameters.
+class UnsafeBufferUsageCtorAttrGadget : public WarningGadget {
+ constexpr static const char *const OpTag = "cxx_construct_expr";
+ const CXXConstructExpr *Op;
+
+public:
+ UnsafeBufferUsageCtorAttrGadget(const MatchFinder::MatchResult &Result)
+ : WarningGadget(Kind::UnsafeBufferUsageCtorAttr),
+ Op(Result.Nodes.getNodeAs<CXXConstructExpr>(OpTag)) {}
+
+ static bool classof(const Gadget *G) {
+ return G->getKind() == Kind::UnsafeBufferUsageCtorAttr;
+ }
+
+ static Matcher matcher() {
+ auto HasUnsafeCtorDecl =
+ hasDeclaration(cxxConstructorDecl(hasAttr(attr::UnsafeBufferUsage)));
+ // std::span(ptr, size) ctor is handled by SpanTwoParamConstructorGadget.
+ auto HasTwoParamSpanCtorDecl = hasDeclaration(
+ cxxConstructorDecl(hasDeclContext(isInStdNamespace()), hasName("span"),
+ parameterCountIs(2)));
+ return stmt(
+ cxxConstructExpr(HasUnsafeCtorDecl, unless(HasTwoParamSpanCtorDecl))
+ .bind(OpTag));
+ }
+
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
}
- const Stmt *getBaseStmt() const override { return Op; }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override { return {}; }
};
@@ -953,7 +1014,13 @@ class DataInvocationGadget : public WarningGadget {
explicitCastExpr(anyOf(has(callExpr), has(parenExpr(has(callExpr)))))
.bind(OpTag));
}
- const Stmt *getBaseStmt() const override { return Op; }
+
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+ Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
DeclUseList getClaimedVarUseSites() const override { return {}; }
};
@@ -990,8 +1057,7 @@ class ULCArraySubscriptGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
+ SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
if (const auto *DRE =
@@ -1031,8 +1097,7 @@ class UPCStandalonePointerGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
+ SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override { return {Node}; }
};
@@ -1070,10 +1135,9 @@ class PointerDereferenceGadget : public FixableGadget {
return {BaseDeclRefExpr};
}
- virtual const Stmt *getBaseStmt() const final { return Op; }
-
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
};
// Represents expressions of the form `&DRE[any]` in the Unspecified Pointer
@@ -1108,8 +1172,7 @@ class UPCAddressofArraySubscriptGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
+ SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
const auto *ArraySubst = cast<ArraySubscriptExpr>(Node->getSubExpr());
@@ -1218,8 +1281,7 @@ class UPCPreIncrementGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
+ SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
return {dyn_cast<DeclRefExpr>(Node->getSubExpr())};
@@ -1264,8 +1326,7 @@ class UUCAddAssignGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &S) const override;
-
- virtual const Stmt *getBaseStmt() const override { return Node; }
+ SourceLocation getSourceLoc() const override { return Node->getBeginLoc(); }
virtual DeclUseList getClaimedVarUseSites() const override {
return {dyn_cast<DeclRefExpr>(Node->getLHS())};
@@ -1315,9 +1376,9 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
virtual std::optional<FixItList>
getFixits(const FixitStrategy &s) const final;
-
- // TODO remove this method from FixableGadget interface
- virtual const Stmt *getBaseStmt() const final { return nullptr; }
+ SourceLocation getSourceLoc() const override {
+ return DerefOp->getBeginLoc();
+ }
virtual DeclUseList getClaimedVarUseSites() const final {
return {BaseDeclRefExpr};
@@ -1326,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple<FixableGadgetList, WarningGadgetList, DeclUseTracker>
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+ const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1422,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -2070,7 +2131,7 @@ UUCAddAssignGadget::getFixits(const FixitStrategy &S) const {
if (S.lookup(VD) == FixitStrategy::Kind::Span) {
FixItList Fixes;
- const Stmt *AddAssignNode = getBaseStmt();
+ const Stmt *AddAssignNode = Node;
StringRef varName = VD->getName();
const ASTContext &Ctx = VD->getASTContext();
@@ -2112,7 +2173,6 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
if (S.lookup(VD) == FixitStrategy::Kind::Span) {
FixItList Fixes;
std::stringstream SS;
- const Stmt *PreIncNode = getBaseStmt();
StringRef varName = VD->getName();
const ASTContext &Ctx = VD->getASTContext();
@@ -2120,12 +2180,12 @@ UPCPreIncrementGadget::getFixits(const FixitStrategy &S) const {
SS << "(" << varName.data() << " = " << varName.data()
<< ".subspan(1)).data()";
std::optional<SourceLocation> PreIncLocation =
- getEndCharLoc(PreIncNode, Ctx.getSourceManager(), Ctx.getLangOpts());
+ getEndCharLoc(Node, Ctx.getSourceManager(), Ctx.getLangOpts());
if (!PreIncLocation)
return std::nullopt;
Fixes.push_back(FixItHint::CreateReplacement(
- SourceRange(PreIncNode->getBeginLoc(), *PreIncLocation), SS.str()));
+ SourceRange(Node->getBeginLoc(), *PreIncLocation), SS.str()));
return Fixes;
}
}
@@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S,
}
#ifndef NDEBUG
Handler.addDebugNoteForVar(
- VD, F->getBaseStmt()->getBeginLoc(),
+ VD, F->getSourceLoc(),
("gadget '" + F->getDebugName() + "' refused to produce a fix")
.str());
#endif
@@ -2970,46 +3030,16 @@ class VariableGroupsManagerImpl : public VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast<CXXMethodDecl>(D)) {
- if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
- for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
- EmitSuggestions = false;
- break;
- }
- }
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation and consider it done. No need to deal
// with fixable gadgets, no need to group operations by variable.
for (const auto &G : WarningGadgets) {
- Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false,
- D->getASTContext());
+ G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false,
+ D->getASTContext());
}
// This return guarantees that most of the machine doesn't run when
@@ -3046,8 +3076,10 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
if (WarningGadgets.empty())
return;
- UnsafeOps = groupWarningGadgetsByVar(std::move(WarningGadgets));
- FixablesForAllVars = groupFixablesByVar(std::move(FixableGadgets));
+ WarningGadgetSets UnsafeOps =
+ groupWarningGadgetsByVar(std::move(WarningGadgets));
+ FixableGadgetSets FixablesForAllVars =
+ groupFixablesByVar(std::move(FixableGadgets));
std::map<const VarDecl *, FixItList> FixItsForVariableGroup;
@@ -3251,8 +3283,8 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
Tracker, Handler, VarGrpMgr);
for (const auto &G : UnsafeOps.noVar) {
- Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/false,
- D->getASTContext());
+ G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false,
+ D->getASTContext());
}
for (const auto &[VD, WarningGadgets] : UnsafeOps.byVar) {
@@ -3263,8 +3295,68 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
: FixItList{},
D, NaiveStrategy);
for (const auto &G : WarningGadgets) {
- Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true,
- D->getASTContext());
+ G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/true,
+ D->getASTContext());
+ }
+ }
+}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector<Stmt*> Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same ...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/91991
More information about the cfe-commits
mailing list