[-Wunsafe-buffer-usage] Add Fixable for simple pointer dereference

[malavikasamak]

Summary

This patch introduces PointerDereferenceGadget, a FixableGadget that emits fixits to handle cases where a pointer that is identified as unsafe is dereferenced. The current implementation only handles cases where the strategy is to change the type of the raw pointer to std::span. The fixit for this strategy is to fetch the first element from the corresponding span instance.

For example for the code below, the PointerDereferenceGadget emits a fixit for S3 (S1, S2 are to be handled by other gadgets): 
S1: int *ptr = new int[10];
S2: int val1 = ptr[k];  // Unsafe operation
S3: int val2 =  *ptr;  => Fixit: int val2 = ptr[0];

[malavikasamak]

Changing the tests to new format

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

471

FWIW other Fixables don't need it to keep result.

Also, I am not even entirely sure how does lifetime of Result passed to the constructor look like - since it isn't a smart pointer I'd be worried about the possibility of getting a dangling reference.

477

Nit: I think we don't want to hardcode tags ("op") but instead we should just add another tag like BaseDeclRefExprTag.

967

Ultimately we need to also handle the case when getEndCharLoc returns nullopt.

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

972

Trying to make the same pitch as here https://reviews.llvm.org/D142795 :
why don't we replace *DRE with DRE[0] in one fix-it step? The benefit is that we can minimize the use these error-prone locations made from getLocWithOffset.

[malavikasamak]

Addressing the recent comments.

[jkorous]

clang/lib/Analysis/UnsafeBufferUsage.cpp

471

Let's initialize Op to nullptr.

958

I don't think we should/need to do this - we already have the DeclRefExpr we need in BaseDeclRefExpr.

976

This unreachable is not in the if block that checks for strategy. This is likely incorrect for DeclRefExpr-s to non-VarDecl-s.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

499–501

IIRC we wanted to move this method to WarningGadget right? So that we didn't need to implement it here? Or was there something that prevented us from doing that? In the latter case nullptr could be really bad.

959

This is another case where the check should probably be included in the matcher. There's no way we'll fix a non-variable, so it shouldn't be "fixable" (some discussion).

What are the other interesting cases that could be found here? I guess BindingDecl could be interesting. Not sure if lambda captures will show up here.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

961

Clang-format might complain about this.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

499–501

Thank you. Fixed!

958

Good catch! Thank you!

976

Fixed!

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

958

Ok now if the Decl is not a VarDecl, the program will crash with one of the unreachables, because S.lookup(nullptr) won't find anything.

If we're sure it's always a VarDecl we should use cast<> (instead of dyn_cast<>) to crash immediately. Otherwise, we probably should check that it's a variable as part of the matcher.

Definitely worth a test.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

958

Hmmm. I thought that this should always yield a VarDecl, but perhaps it's best to keep the check for it. Just in case.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

958

Yeah I think something along the lines of https://godbolt.org would have crashed it. Might be a good idea to add a test.

Another tiny nitpick, there appears to be a policy to favor early returns over nesting, this looks like a prime example of that.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

972

TODO: I am waiting for us to establish a standard for fixit format before addressing this comment.

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

958

Uh-oh, just noticed that godbolt link is broken. Made a new one: https://godbolt.org/z/1ahTx1E37

[malavikasamak]

Addressing comment on VarDecl checks in matcher and early returns.

[NoQ]

The early return appears to be missing in the final patch 🤔

[NoQ]

Oh, wait, nvm, you did the matcher thing, nice! LGTM then!

clang/lib/Analysis/UnsafeBufferUsage.cpp

907

Now that this is a matcher, we no longer need the cast to be "dynamic".

clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def

 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 FIXABLE_GADGET(ULCArraySubscript)
 FIXABLE_GADGET(DerefSimplePtrArithFixable)
+FIXABLE_GADGET(PointerDereference)
 
 #undef FIXABLE_GADGET
 #undef WARNING_GADGET
 #undef GADGET

clang/lib/Analysis/UnsafeBufferUsage.cpp

public:
   virtual DeclUseList getClaimedVarUseSites() const override {
     if (const auto *DRE =
             dyn_cast<DeclRefExpr>(Node->getBase()->IgnoreImpCasts())) {
       return {DRE};
     }
     return {};
   }
 };
+
+class PointerDereferenceGadget : public FixableGadget {
+  static constexpr const char *const BaseDeclRefExprTag = "BaseDRE";
+  static constexpr const char *const OperatorTag = "op";
+
+  const DeclRefExpr *BaseDeclRefExpr = nullptr;
+  const UnaryOperator *Op = nullptr;
+
+public:
+  PointerDereferenceGadget(const MatchFinder::MatchResult &Result)
+      : FixableGadget(Kind::PointerDereference),
+        BaseDeclRefExpr(
+            Result.Nodes.getNodeAs<DeclRefExpr>(BaseDeclRefExprTag)),
+        Op(Result.Nodes.getNodeAs<UnaryOperator>(OperatorTag)) {}
+
+  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)))))
+            .bind(OperatorTag);
+
+    return expr(isInUnspecifiedLvalueContext(Target));
+  }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    return {BaseDeclRefExpr};
+  }
+
+  virtual const Stmt *getBaseStmt() const final { return Op; }
+
+  virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+};
+
 } // namespace
 
 namespace {
 // An auxiliary tracking facility for the fixit analysis. It helps connect
 // declarations to its and make sure we've covered all uses with our analysis
 // before we try to fix the declaration.
 class DeclUseTracker {
   using UseSetTy = SmallSet<const DeclRefExpr *, 16>;
static StringRef getExprText(const Expr *E, const SourceManager &SM,
 
   return Lexer::getSourceText(
       CharSourceRange::getCharRange(E->getBeginLoc(), LastCharLoc), SM,
       LangOpts);
 }
 
 std::optional<FixItList>
 DerefSimplePtrArithFixableGadget::getFixits(const Strategy &s) const {
   const VarDecl *VD = dyn_cast<VarDecl>(BaseDeclRefExpr->getDecl());
 
   if (VD && s.lookup(VD) == Strategy::Kind::Span) {
     ASTContext &Ctx = VD->getASTContext();
     // std::span can't represent elements before its begin()
     if (auto ConstVal = Offset->getIntegerConstantExpr(Ctx))
       if (ConstVal->isNegative())
         return std::nullopt;
 
if (VD && s.lookup(VD) == Strategy::Kind::Span) {
     return FixItList{
         {FixItHint::CreateRemoval(StarWithTrailWhitespace),
          FixItHint::CreateReplacement(PlusWithSurroundingWhitespace, "["),
          FixItHint::CreateReplacement(ClosingParenWithPrecWhitespace, "]")}};
   }
   return std::nullopt; // something wrong or unsupported, give up
 }
 
+std::optional<FixItList>
+PointerDereferenceGadget::getFixits(const Strategy &S) const {
+  const VarDecl *VD = cast<VarDecl>(BaseDeclRefExpr->getDecl());
+  switch (S.lookup(VD)) {
+  case Strategy::Kind::Span: {
+    ASTContext &Ctx = VD->getASTContext();
+    SourceManager &SM = Ctx.getSourceManager();
+    // Required changes: *(ptr); => (ptr[0]); and *ptr; => ptr[0]
+    // Deletes the *operand
+    CharSourceRange derefRange = clang::CharSourceRange::getCharRange(
+        Op->getBeginLoc(), Op->getBeginLoc().getLocWithOffset(1));
+    // Inserts the [0]
+    std::optional<SourceLocation> endOfOperand =
+        getEndCharLoc(BaseDeclRefExpr, SM, Ctx.getLangOpts());
+    if (endOfOperand) {
+      return FixItList{{FixItHint::CreateRemoval(derefRange),
+                        FixItHint::CreateInsertion(
+                            endOfOperand.value().getLocWithOffset(1), "[0]")}};
+    }
+  }
+  case Strategy::Kind::Iterator:
+  case Strategy::Kind::Array:
+  case Strategy::Kind::Vector:
+    llvm_unreachable("Strategy not implemented yet!");
+  case Strategy::Kind::Wontfix:
+    llvm_unreachable("Invalid strategy!");
+  }
+
+  return std::nullopt;
+}
+
 // For a non-null initializer `Init` of `T *` type, this function returns
 // `FixItHint`s producing a list initializer `{Init,  S}` as a part of a fix-it
 // to output stream.
 // In many cases, this function cannot figure out the actual extent `S`.  It
 // then will use a place holder to replace `S` to ask users to fill `S` in.  The
 // initializer shall be used to initialize a variable of type `std::span<T>`.
 //
 // FIXME: Support multi-level pointers

clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-deref.cpp

+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+void basic_dereference() {
+  int tmp;
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  tmp = p[5];
+  int val = *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:15-[[@LINE-2]]:15}:"[0]"
+}
+
+int return_method() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+  int tmp = p[5];
+  return *p;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"[0]"
+}
+
+void foo(int v) {
+}
+
+void method_invocation() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  int tmp = p[5];
+
+  foo(*p);
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:8}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:9-[[@LINE-2]]:9}:"[0]"
+}
+
+void binary_operation() {
+  auto p = new int[10];
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:11}:"std::span<int> p"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:12-[[@LINE-2]]:12}:"{"
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-3]]:23-[[@LINE-3]]:23}:", 10}"
+
+  int tmp = p[5];
+
+  int k = *p + 20;
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:12}:""
+  // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:13}:"[0]"
+
+}
+