Respect the [[clang::unsafe_buffer_usage]] attribute for constructors #91777
Conversation
llvmbot
added
clang
|
cc: @haoNoQ |
|
@llvm/pr-subscribers-clang Author: Dana Jansens (danakj) ChangesThe -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. Patch is 20.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91777.diff 5 Files Affected:
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..21dea243897c9 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,54 @@ 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(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
+ // cxxOperatorCallExpr(HasUnsafeFnDecl).bind(OpTag)));
+ 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 {}; }
+};
+
+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 +1013,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 +1056,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 +1096,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 +1134,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 +1171,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 +1280,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 +1325,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 +1375,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 +2130,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 +2172,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 +2179,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 +2915,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 +3067,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 +3311,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 +3323,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()(...
[truncated]
|
|
@llvm/pr-subscribers-clang-analysis Author: Dana Jansens (danakj) ChangesThe -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. Patch is 20.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/91777.diff 5 Files Affected:
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..21dea243897c9 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,54 @@ 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(anyOf(callExpr(HasUnsafeFnDecl).bind(OpTag),
+ // cxxOperatorCallExpr(HasUnsafeFnDecl).bind(OpTag)));
+ 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 {}; }
+};
+
+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 +1013,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 +1056,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 +1096,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 +1134,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 +1171,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 +1280,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 +1325,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 +1375,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 +2130,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 +2172,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 +2179,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 +2915,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 +3067,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 +3311,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 +3323,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()(...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
|
Hi! Thank you for digging into this! Sorry for the delay.
Hmm at a glance I'm not sure this should really be illegal. It's a big problem when fixable gadgets overlap, because this means that they'll try to fix the same code in two incompatible ways. I don't immediately see why this would be a problem for warning gadgets that don't try to fix anything. This just means that the code is non-compliant for two different reasons, which is theoretically fine. I also tried to reproduce your assert and I didn't hit it; which one are you running into?
Ooo I love this. My initial thinking was, just make the base Gadget class "public" (move it into Your design can grow into that too - make a separate handler method for each gadget kind (or group of kinds), and it preserves even more encapsulation. Additionally it throws away |
llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp Lines 1373 to 1374 in 2ff43ce These assert that exactly one gadget matched. I think it's kinda worthwhile keeping the warnings independent too, so I don't see this as a bad thing. If we were to mark the span ctor as
👍 |
There was a problem hiding this comment.
llvm-project/clang/lib/Analysis/UnsafeBufferUsage.cpp
Lines 1373 to 1374 in 2ff43ce
These assert that exactly one gadget matched. I think it's kinda worthwhile keeping the warnings independent too, so I don't see this as a bad thing. If we were to mark the span ctor as
[[clang::unsafe_buffer_usage]]for instance, it the span-specific warning gives a better output still, and generating two warnings for the same piece of code is noisy.
Hmm this isn't really what these assertions are about. They don't protect against duplicate gadgets, they protect against duplicate .bind() tags across gadgets. Because gadgets are connected by the anyOf() matcher (as opposed to eachOf()), a single match result will always have exactly one gadget, the first one in the list, even if they match the same statement.
So I think you're right that there's a problem: if you have two gadgets match the same statement, only one will be reported. So your solution is probably necessary. We should also maybe do something about that, so that warning gadgets weren't conflicting this way. But I don't think this assertion guards against that.
If this still doesn't sound right to you, can you include a test where the assertion fails for you? No matter how ridiculous the test is. The machine is complex enough to appreciate ridiculous tests. I think we want to get to the bottom of this.
| auto HasUnsafeCtorDecl = | ||
| hasDeclaration(cxxConstructorDecl(hasAttr(attr::UnsafeBufferUsage))); | ||
| // std::span(ptr, size) ctor is handled by SpanTwoParamConstructorGadget. | ||
| auto HasTwoParamSpanCtorDecl = hasDeclaration( |
There was a problem hiding this comment.
I wouldn't mind something like
auto HasTwoParamSpanCtorDecl = SpanTwoParamConstructorGadget::matcher();
to prevent it from diverging when the other gadget changes. (We could build a fancier conflict resolution system but I don't think it's necessary yet.)
There was a problem hiding this comment.
Oh, yeah good idea. Done.
| @@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, | |||
| } | |||
| #ifndef NDEBUG | |||
| Handler.addDebugNoteForVar( | |||
| VD, F->getBaseStmt()->getBeginLoc(), | |||
| VD, F->getSourceLoc(), | |||
There was a problem hiding this comment.
This is the only place where we do any actual virtual dispatch over getSourceLoc() right? Otherwise we could merge it into handleUnsafeOperation() too. Maybe with a slightly different design we could eliminate this too. Dunno, I don't have anything specific in mind.
There was a problem hiding this comment.
Yeah, this is the only callsite that needs the SourceLocation for a FixableGadget.
There was a problem hiding this comment.
Hmm right, this is literally the only case where we use it at all. Maybe the right thing to do is to make that source location (or whatever we need) a member variable in the Gadget class, and pre-populate it in the findGadgets() method with the root statement. We don't really care what statement it is anyway do we. We just need something to work with. We can even do this under #ifndef NDEBUG because that's the only way we ever use it. And we don't have to define it in every subclass anymore. But if a subclass doesn't like the default value they can still reassign it in constructor.
I.e.
class FixableGadget {
+#ifndef NDEBUG
+SourceLocation DebugLoc;
+#endif
public:
- FixableGadget(Kind K) : Gadget(K) {}
+ FixableGadget(Kind K, const MatchFinder::MatchResult &Result) :
+ Gadget(K),
+ DebugLoc(Result.getNodeAs<Stmt>(getDebugName() + "Gadget")->getBeginLoc())
+ {}
+#ifndef NDEBUG
+ SourceLocation getDebugLoc() const { return DebugLoc; }
+#endif
};
class PointerInitGadget : FixableGadget {
PointerInitGadget(const MatchFinder::MatchResult &Result)
- : FixableGadget(Kind::PointerInit),
+ : FixableGadget(Kind::PointerInit, Result),
PtrInitLHS(Result.Nodes.getNodeAs<VarDecl>(PointerInitLHSTag)),
PtrInitRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerInitRHSTag)) {}
- // And then you no longer need getSourceLoc() in every class.
};(You don't have to do this in order to land the patch. I think it looks great already. I'm just thinking out loud.)
There was a problem hiding this comment.
I will leave this for a followup, yeah. Thanks
| SourceLocation Loc; | ||
| SourceRange Range; | ||
| unsigned MsgParam = 0; | ||
| if (const auto *CtorExpr = dyn_cast<CXXConstructExpr>(Operation)) { |
There was a problem hiding this comment.
This should be a hard cast<>() because you don't really handle the else-branch so it results in an invalid source location. We can make it dynamic again later if we want.
There was a problem hiding this comment.
Got it. The old code was dyn but that was because it handled other cases too. Once this function handles other gadgets we'll adjust it.
Ah yeah, okay, that makes sense. I did bump into the asserts while working on this, but not in the way that I thought. When I remove the
It sounds right to me yeah, especially after testing it. |
| @@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, | |||
| } | |||
| #ifndef NDEBUG | |||
| Handler.addDebugNoteForVar( | |||
| VD, F->getBaseStmt()->getBeginLoc(), | |||
| VD, F->getSourceLoc(), | |||
There was a problem hiding this comment.
Hmm right, this is literally the only case where we use it at all. Maybe the right thing to do is to make that source location (or whatever we need) a member variable in the Gadget class, and pre-populate it in the findGadgets() method with the root statement. We don't really care what statement it is anyway do we. We just need something to work with. We can even do this under #ifndef NDEBUG because that's the only way we ever use it. And we don't have to define it in every subclass anymore. But if a subclass doesn't like the default value they can still reassign it in constructor.
I.e.
class FixableGadget {
+#ifndef NDEBUG
+SourceLocation DebugLoc;
+#endif
public:
- FixableGadget(Kind K) : Gadget(K) {}
+ FixableGadget(Kind K, const MatchFinder::MatchResult &Result) :
+ Gadget(K),
+ DebugLoc(Result.getNodeAs<Stmt>(getDebugName() + "Gadget")->getBeginLoc())
+ {}
+#ifndef NDEBUG
+ SourceLocation getDebugLoc() const { return DebugLoc; }
+#endif
};
class PointerInitGadget : FixableGadget {
PointerInitGadget(const MatchFinder::MatchResult &Result)
- : FixableGadget(Kind::PointerInit),
+ : FixableGadget(Kind::PointerInit, Result),
PtrInitLHS(Result.Nodes.getNodeAs<VarDecl>(PointerInitLHSTag)),
PtrInitRHS(Result.Nodes.getNodeAs<DeclRefExpr>(PointerInitRHSTag)) {}
- // And then you no longer need getSourceLoc() in every class.
};(You don't have to do this in order to land the patch. I think it looks great already. I'm just thinking out loud.)
| @@ -1315,9 +1374,9 @@ class DerefSimplePtrArithFixableGadget : public FixableGadget { | |||
|
|
|||
| virtual std::optional<FixItList> | |||
| getFixits(const FixitStrategy &s) const final; | |||
|
|
|||
| // TODO remove this method from FixableGadget interface | |||
There was a problem hiding this comment.
This one used to return null, but it does return something useful now. So instead of putting this TODO back (and equally putting it on every FixableGadget subclass), I can apply it up in the Gadget class.
There was a problem hiding this comment.
Also it's slightly different (per your comment above), in that we can remove it from the WarningGadget API, leaving it only on FixableGadget for debug prints. I have updated the TODO to say so.
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.
Issue #80482