[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
Dana Jansens via cfe-commits
cfe-commits at lists.llvm.org
Mon May 13 11:48:15 PDT 2024
https://github.com/danakj updated https://github.com/llvm/llvm-project/pull/91991
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 ++++++++++++------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38 ++++
5 files changed, 176 insertions(+), 57 deletions(-)
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..b24588e722aaa 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};
@@ -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
@@ -3008,8 +3068,9 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
// 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());
+ llvm::errs() << "Warnings not EmitSuggestions\n";
+ G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false,
+ D->getASTContext());
}
// This return guarantees that most of the machine doesn't run when
@@ -3251,8 +3312,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 +3324,8 @@ 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());
}
}
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 6992ba9ad9a75..b3dd4bc6aa8f5 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2256,11 +2256,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
Range = UO->getSubExpr()->getSourceRange();
MsgParam = 1;
}
- } else if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) {
- S.Diag(CtorExpr->getLocation(),
- diag::warn_unsafe_buffer_usage_in_container);
} else {
- if (isa<CallExpr>(Operation)) {
+ if (isa<CallExpr>(Operation) || isa<CXXConstructExpr>(Operation)) {
// note_unsafe_buffer_operation doesn't have this mode yet.
assert(!IsRelatedToDecl && "Not implemented yet!");
MsgParam = 3;
@@ -2295,6 +2292,23 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
}
}
+ void handleUnsafeOperationInContainer(const Stmt *Operation,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) override {
+ SourceLocation Loc;
+ SourceRange Range;
+ unsigned MsgParam = 0;
+ if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) {
+ Loc = CtorExpr->getLocation();
+ }
+ S.Diag(Loc, diag::warn_unsafe_buffer_usage_in_container);
+ if (IsRelatedToDecl) {
+ assert(!SuggestSuggestions &&
+ "Variables blamed for unsafe buffer usage without suggestions!");
+ S.Diag(Loc, diag::note_unsafe_buffer_operation) << MsgParam << Range;
+ }
+ }
+
void handleUnsafeVariableGroup(const VarDecl *Variable,
const VariableGroupsManager &VarGrpMgr,
FixItList &&Fixes, const Decl *D,
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
index 7df01c46438c7..bfc34b55c1f66 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -85,3 +85,41 @@ void testInheritance() {
BC->func(); // expected-warning{{function introduces unsafe buffer manipulation}}
BC->func1();
}
+
+class UnsafeMembers {
+public:
+ UnsafeMembers() {}
+
+ [[clang::unsafe_buffer_usage]]
+ UnsafeMembers(int) {}
+
+ [[clang::unsafe_buffer_usage]]
+ explicit operator int() { return 0; }
+
+ [[clang::unsafe_buffer_usage]]
+ void Method() {}
+
+ [[clang::unsafe_buffer_usage]]
+ void operator()() {}
+
+ [[clang::unsafe_buffer_usage]]
+ int operator+(UnsafeMembers) { return 0; }
+};
+
+template <class... Vs>
+int testFoldExpression(Vs&&... v) {
+ return (... + v); // expected-warning{{function introduces unsafe buffer manipulation}}
+}
+
+// https://github.com/llvm/llvm-project/issues/80482
+void testClassMembers() {
+ UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ (void)static_cast<int>(UnsafeMembers()); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ UnsafeMembers().Method(); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ UnsafeMembers()(); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ testFoldExpression(UnsafeMembers(), UnsafeMembers());
+}
>From 84da0fe9aa5d80c41ef78e79f6d47730f5be7880 Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Fri, 10 May 2024 13:53:23 -0400
Subject: [PATCH 2/3] Remove debug logging
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index b24588e722aaa..9e07b9cbb6aea 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -3068,7 +3068,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
// 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) {
- llvm::errs() << "Warnings not EmitSuggestions\n";
G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false,
D->getASTContext());
}
>From c9135681a3860a45e9180f8460e09d795a4caaa0 Mon Sep 17 00:00:00 2001
From: danakj <danakj at chromium.org>
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 3/3] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 ++++++++++++------
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++++++++++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 9e07b9cbb6aea..4b60498a76298 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,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;
@@ -1483,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)};
}
@@ -3030,39 +3030,9 @@ 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
@@ -3106,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;
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+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 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;
+ }
+ }
+
+ Stmts.push_back(FD->getBody());
+
+ if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+ Stmts.push_back(CI->getInit());
+ }
+ }
+ }
+
+ if (const auto *FD = dyn_cast<FieldDecl>(D)) {
+ // Visit in-class initializers for fields.
+ if (!FD->hasInClassInitializer())
+ return;
+
+ Stmts.push_back(FD->getInClassInitializer());
+ }
+
+ if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) {
+ Stmts.push_back(D->getBody());
+ }
+
+ assert(!Stmts.empty());
+
+ for (Stmt *S : Stmts) {
+ auto [FixableGadgets, WarningGadgets, Tracker] =
+ findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
+ applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
+ std::move(Tracker), Handler, EmitSuggestions);
+ }
+}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index b3dd4bc6aa8f5..9aedae6a71b59 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2468,6 +2468,14 @@ class CallableVisitor : public RecursiveASTVisitor<CallableVisitor> {
CallableVisitor(llvm::function_ref<void(const Decl *)> Callback)
: Callback(Callback) {}
+ bool VisitFieldDecl(FieldDecl *Node) {
+ if (cast<DeclContext>(Node->getParent())->isDependentContext())
+ return true; // Not to analyze dependent decl
+ if (Node->hasInClassInitializer())
+ Callback(Node);
+ return true;
+ }
+
bool VisitFunctionDecl(FunctionDecl *Node) {
if (cast<DeclContext>(Node)->isDependentContext())
return true; // Not to analyze dependent decl
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
index bfc34b55c1f66..a0efecefe1c89 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp
@@ -111,6 +111,42 @@ int testFoldExpression(Vs&&... v) {
return (... + v); // expected-warning{{function introduces unsafe buffer manipulation}}
}
+struct HoldsUnsafeMembers {
+ HoldsUnsafeMembers()
+ : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ [[clang::unsafe_buffer_usage]]
+ HoldsUnsafeMembers(int i)
+ : FromCtor(i), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{i} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ HoldsUnsafeMembers(float f)
+ : HoldsUnsafeMembers(0) {} // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ UnsafeMembers FromCtor;
+ UnsafeMembers FromCtor2;
+ UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
+
+struct SubclassUnsafeMembers : public UnsafeMembers {
+ SubclassUnsafeMembers()
+ : UnsafeMembers(3) // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ [[clang::unsafe_buffer_usage]]
+ SubclassUnsafeMembers(int i)
+ : UnsafeMembers(i) // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+};
+
+struct AggregateUnsafeMembers {
+ UnsafeMembers f1;
+ UnsafeMembers f2{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
+
// https://github.com/llvm/llvm-project/issues/80482
void testClassMembers() {
UnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
@@ -122,4 +158,35 @@ void testClassMembers() {
UnsafeMembers()(); // expected-warning{{function introduces unsafe buffer manipulation}}
testFoldExpression(UnsafeMembers(), UnsafeMembers());
+
+ HoldsUnsafeMembers();
+ HoldsUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ SubclassUnsafeMembers();
+ SubclassUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ (void)AggregateUnsafeMembers{
+ .f1 = UnsafeMembers(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ };
+
+ (void)AggregateUnsafeMembers(3); // expected-warning{{function introduces unsafe buffer manipulation}}
+
+ // FIXME: This should generate a warning but InitListExpr construction is
+ // treated as calling an implicit constructor, while ParentListInitExpr (the
+ // one above) is not. So `MatchDescendantVisitor::shouldVisitImplicitCode()`
+ // must return true for this to generate a warning. However that moves the
+ // SourceLocation of warnings from field initializers that construct through
+ // InitListExpr, preventing us from matching warnings against them.
+ (void)AggregateUnsafeMembers{3};
}
+
+struct NotCalledHoldsUnsafeMembers {
+ NotCalledHoldsUnsafeMembers()
+ : FromCtor(3), // expected-warning{{function introduces unsafe buffer manipulation}}
+ FromCtor2{3} // expected-warning{{function introduces unsafe buffer manipulation}}
+ {}
+
+ UnsafeMembers FromCtor;
+ UnsafeMembers FromCtor2;
+ UnsafeMembers FromField{3}; // expected-warning{{function introduces unsafe buffer manipulation}}
+};
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
index a1ddc384e0d9b..945a4e06d6afe 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-in-container-span-construct.cpp
@@ -154,3 +154,13 @@ namespace test_flag {
}
} //namespace test_flag
+
+struct HoldsStdSpan {
+ char* Ptr;
+ unsigned Size;
+ std::span<char> Span{Ptr, Size}; // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+
+ HoldsStdSpan(char* P, unsigned S)
+ : Span(P, S) // expected-warning{{the two-parameter std::span construction is unsafe as it can introduce mismatch between buffer size and the bound information}}
+ {}
+};
More information about the cfe-commits
mailing list