[-Wunsafe-buffer-usage] Fix assertion failure in case of BindingDecl

[NoQ]

The check to(varDecl()) is repeated in every gadget as well as here, now they go even further out of sync. It probably makes sense to make a reusable matcher toSupportedVariable() that covers all cases we want to cover(?)

[NoQ]

Aha great! Code reuse!!

I'm further trying to sort out this issue once and for all, because we keep running into it, so I have, uhh, further suggestions.

clang/lib/Analysis/UnsafeBufferUsage.cpp

384

Now, I don't think we actually support BindingDecl yet. We don't really know how to emit a fixit for it, and this is very likely to be as hard as struct member fixits, given that it's about decomposing structs into individual members. Maybe we'll have explicit support for std::pair and std::tuple in the future, but we're not there yet.

So we should probably leave it as to(varDecl()) for now. It's still great that we reused it though!

Another thing we can put there, is the check that the variable is *local*. Then we can remove a lot of manual imperative checks for that in all the getFixits() methods.

699–700

I found one more place where these checks are missing.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

384

Another thing we can put there, is the check that the variable is *local*. Then we can remove a lot of manual imperative checks for that in all the getFixits() methods.

As is tradition, I appear to be making it up as there aren't any duplicated checks for local variables in getFixits() methods. They're already deduplicated to like 2 places.

It's definitely a good question who's really responsible to performing these checks; it depends on whether we see it as a strategy thing ("fix this variable with span, but as if it's a global variable, so slap an attribute on it and introduce safe accessors - that's a different strategy from changing the type of a local variable to span, and we can technically choose the local strategy for a static global, or a global-like strategy for a local variable, kinda why not"), or as a fixable thing ("I'm a fixable who knows how to fix a use of a local variable with span, there's supposed to be another fixable who knows how to fix a similar use of a global variable with span, and we never talk to each other").

But as Rashmi correctly pointed out offline, it's not really valuable to resolve this question right away, and trying to make fixables responsible for this check will only get in the way of gathering statistics and improving parts that actually matter.

It's really valuable to have every part of the machine agree what a supported variable is, but not every aspect needs to be actually checked by each gadget.

[NoQ]

Oo you already updated it. LGTM then, thanks!!

clang/lib/Analysis/UnsafeBufferUsage.cpp

public:
   /// later to group all those variables whose types must be modified together to prevent type
   /// mismatches.
   virtual std::optional<std::pair<const VarDecl *, const VarDecl *>>
   getStrategyImplications() const {
     return std::nullopt;
   }
 };
 
+static auto toSupportedVariable() {
+  return to(varDecl());
+}
+
 using FixableGadgetList = std::vector<std::unique_ptr<FixableGadget>>;
 using WarningGadgetList = std::vector<std::unique_ptr<WarningGadget>>;
 
 /// An increment of a pointer-type value is unsafe as it may run the pointer
 /// out of bounds.
 class IncrementGadget : public WarningGadget {
   static constexpr const char *const OpTag = "op";
   const UnaryOperator *Op;
public:
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PointerInit;
   }
 
   static Matcher matcher() {
     auto PtrInitStmt = declStmt(hasSingleDecl(varDecl(
                                  hasInitializer(ignoringImpCasts(declRefExpr(
-                                                  hasPointerType()).
+                                                  hasPointerType(),
+                                                    toSupportedVariable()).
                                                   bind(PointerInitRHSTag)))).
                                               bind(PointerInitLHSTag)));
 
     return stmt(PtrInitStmt);
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
public:
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PointerAssignment;
   }
 
   static Matcher matcher() {
     auto PtrAssignExpr = binaryOperator(allOf(hasOperatorName("="),
       hasRHS(ignoringParenImpCasts(declRefExpr(hasPointerType(),
-                                               to(varDecl())).
+                                               toSupportedVariable()).
                                    bind(PointerAssignRHSTag))),
                                    hasLHS(declRefExpr(hasPointerType(),
-                                                      to(varDecl())).
+                                                      toSupportedVariable()).
                                           bind(PointerAssignLHSTag))));
 
     return stmt(isInUnspecifiedUntypedContext(PtrAssignExpr));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
   virtual const Stmt *getBaseStmt() const override {
public:
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::ULCArraySubscript;
   }
 
   static Matcher matcher() {
     auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType());
     auto BaseIsArrayOrPtrDRE =
-        hasBase(ignoringParenImpCasts(declRefExpr(ArrayOrPtr)));
+        hasBase(ignoringParenImpCasts(declRefExpr(ArrayOrPtr,
+                                                  toSupportedVariable())));
     auto Target =
         arraySubscriptExpr(BaseIsArrayOrPtrDRE).bind(ULCArraySubscriptTag);
 
     return expr(isInUnspecifiedLvalueContext(Target));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
public:
 
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::UPCStandalonePointer;
   }
 
   static Matcher matcher() {
     auto ArrayOrPtr = anyOf(hasPointerType(), hasArrayType());
     auto target = expr(
-        ignoringParenImpCasts(declRefExpr(allOf(ArrayOrPtr, to(varDecl()))).bind(DeclRefExprTag)));
+        ignoringParenImpCasts(declRefExpr(allOf(ArrayOrPtr, 
+                              toSupportedVariable())).bind(DeclRefExprTag)));
     return stmt(isInUnspecifiedPointerContext(target));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
   virtual const Stmt *getBaseStmt() const override { return Node; }
 
   virtual DeclUseList getClaimedVarUseSites() const override {
static bool classof(const Gadget *G) {
     return G->getKind() == Kind::PointerDereference;
   }
 
   static Matcher matcher() {
     auto Target =
         unaryOperator(
             hasOperatorName("*"),
             has(expr(ignoringParenImpCasts(
-                declRefExpr(to(varDecl())).bind(BaseDeclRefExprTag)))))
+                declRefExpr(toSupportedVariable()).bind(BaseDeclRefExprTag)))))
             .bind(OperatorTag);
 
     return expr(isInUnspecifiedLvalueContext(Target));
   }
 
   DeclUseList getClaimedVarUseSites() const override {
     return {BaseDeclRefExpr};
   }
public:
   static bool classof(const Gadget *G) {
     return G->getKind() == Kind::UPCAddressofArraySubscript;
   }
 
   static Matcher matcher() {
     return expr(isInUnspecifiedPointerContext(expr(ignoringImpCasts(
         unaryOperator(hasOperatorName("&"),
                       hasUnaryOperand(arraySubscriptExpr(
-                          hasBase(ignoringParenImpCasts(declRefExpr())))))
+                          hasBase(ignoringParenImpCasts(declRefExpr(
+                                                  toSupportedVariable()))))))
             .bind(UPCAddressofArraySubscriptTag)))));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &) const override;
 
   virtual const Stmt *getBaseStmt() const override { return Node; }
 
   virtual DeclUseList getClaimedVarUseSites() const override {
public:
 
   static Matcher matcher() {
     // Note here we match `++Ptr` for any expression `Ptr` of pointer type.
     // Although currently we can only provide fix-its when `Ptr` is a DRE, we
     // can have the matcher be general, so long as `getClaimedVarUseSites` does
     // things right.
     return stmt(isInUnspecifiedPointerContext(expr(ignoringImpCasts(
                     unaryOperator(isPreInc(),
-                      hasUnaryOperand(declRefExpr())
+                      hasUnaryOperand(declRefExpr(
+                                                    toSupportedVariable()))
                       ).bind(UPCPreIncrementTag)))));
   }
 
   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
 
   virtual const Stmt *getBaseStmt() const override { return Node; }
 
   virtual DeclUseList getClaimedVarUseSites() const override {
DerefSimplePtrArithFixableGadget(const MatchFinder::MatchResult &Result)
             Result.Nodes.getNodeAs<DeclRefExpr>(BaseDeclRefExprTag)),
         DerefOp(Result.Nodes.getNodeAs<UnaryOperator>(DerefOpTag)),
         AddOp(Result.Nodes.getNodeAs<BinaryOperator>(AddOpTag)),
         Offset(Result.Nodes.getNodeAs<IntegerLiteral>(OffsetTag)) {}
 
   static Matcher matcher() {
     // clang-format off
     auto ThePtr = expr(hasPointerType(),
-                       ignoringImpCasts(declRefExpr(to(varDecl())).bind(BaseDeclRefExprTag)));
+                       ignoringImpCasts(declRefExpr(toSupportedVariable()).
+                                        bind(BaseDeclRefExprTag)));
     auto PlusOverPtrAndInteger = expr(anyOf(
           binaryOperator(hasOperatorName("+"), hasLHS(ThePtr),
                          hasRHS(integerLiteral().bind(OffsetTag)))
                          .bind(AddOpTag),
           binaryOperator(hasOperatorName("+"), hasRHS(ThePtr),
                          hasLHS(integerLiteral().bind(OffsetTag)))
                          .bind(AddOpTag)));
     return isInUnspecifiedLvalueContext(unaryOperator(
M.addMatcher(
         stmt(
           forEachDescendantStmt(stmt(eachOf(
 #define FIXABLE_GADGET(x)                                                      \
             x ## Gadget::matcher().bind(#x),
 #include "clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def"
             // In parallel, match all DeclRefExprs so that to find out
             // whether there are any uncovered by gadgets.
             declRefExpr(anyOf(hasPointerType(), hasArrayType()),
-                        to(varDecl())).bind("any_dre"),
+                        to(anyOf(varDecl(), bindingDecl()))).bind("any_dre"),
             // Also match DeclStmts because we'll need them when fixing
             // their underlying VarDecls that otherwise don't have
             // any backreferences to DeclStmts.
             declStmt().bind("any_ds")
           )))
       ),
       &CB
     );

clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp

void test_globals() {
   a[7] = 4;  // expected-note{{used in buffer access here}}
 }
 
 void test_decomp_decl() {
   int a[2] = {1, 2};
   auto [x, y] = a;
   x = 9;
 }
+
+void test_claim_use_multiple() {
+  int *a = new int[8];  // expected-warning{{'a' is an unsafe pointer used for buffer access}}
+  a[6] = 9;  // expected-note{{used in buffer access here}}
+  a++;  // expected-note{{used in pointer arithmetic here}} \
+  // debug-note{{safe buffers debug: failed to produce fixit for 'a' : has an unclaimed use}}
+}
+
+struct S
+{
+    int *x;
+};
+ 
+S f() { return S{new int[4]}; }
+
+void test_struct_claim_use() {
+  auto [x] = f();
+  x[6] = 8;  // expected-warning{{unsafe buffer access}}
+  x++;  // expected-warning{{unsafe pointer arithmetic}}
+}