Skip to content

Respect the [[clang::unsafe_buffer_usage]] attribute for constructors #91777

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 16, 2024
Merged

Respect the [[clang::unsafe_buffer_usage]] attribute for constructors #91777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b318da
Respect the [[clang::unsafe_buffer_usage]] attribute for constructors
danakj May 10, 2024
84da0fe
Remove debug logging
danakj May 10, 2024
d9e7b26
Apply review suggestions
danakj May 14, 2024
7f16dd5
Add TODO comment back, but in the Gadget class
danakj May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
166 changes: 113 additions & 53 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,9 @@ class Gadget {
#endif

virtual bool isWarningGadget() const = 0;
virtual const Stmt *getBaseStmt() const = 0;
// TODO remove this method from WarningGadget interface. It's only used for
// debug prints in FixableGadget.
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
Expand All @@ -513,6 +515,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
Expand Down Expand Up @@ -572,7 +578,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;
Expand Down Expand Up @@ -607,7 +618,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 =
Expand Down Expand Up @@ -648,7 +664,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 =
Expand Down Expand Up @@ -696,7 +717,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())) {
Expand Down Expand Up @@ -734,7 +760,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
Expand Down Expand Up @@ -780,11 +811,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 {
Expand Down Expand Up @@ -833,12 +861,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};
Expand Down Expand Up @@ -888,12 +911,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};
Expand Down Expand Up @@ -921,10 +939,53 @@ 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);
}
const Stmt *getBaseStmt() const override { return Op; }
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 = SpanTwoParamConstructorGadget::matcher();
return stmt(
cxxConstructExpr(HasUnsafeCtorDecl, unless(HasTwoParamSpanCtorDecl))
.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 {}; }
};
Expand Down Expand Up @@ -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 {}; }
};
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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}; }
};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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())};
Expand Down Expand Up @@ -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())};
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still relevant tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!

virtual const Stmt *getBaseStmt() const final { return nullptr; }
SourceLocation getSourceLoc() const override {
return DerefOp->getBeginLoc();
}

virtual DeclUseList getClaimedVarUseSites() const final {
return {BaseDeclRefExpr};
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -2112,20 +2173,19 @@ 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();

// To transform UPC(++p) to UPC((p = p.subspan(1)).data()):
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;
}
}
Expand Down Expand Up @@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S,
}
#ifndef NDEBUG
Handler.addDebugNoteForVar(
VD, F->getBaseStmt()->getBeginLoc(),
VD, F->getSourceLoc(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is the only callsite that needs the SourceLocation for a FixableGadget.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will leave this for a followup, yeah. Thanks

("gadget '" + F->getDebugName() + "' refused to produce a fix")
.str());
#endif
Expand Down Expand Up @@ -3008,8 +3068,8 @@ 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());
G->handleUnsafeOperation(Handler, /*IsRelatedToDecl=*/false,
D->getASTContext());
}

// This return guarantees that most of the machine doesn't run when
Expand Down Expand Up @@ -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) {
Expand All @@ -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());
}
}
}
Loading
Loading