[-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

430

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.

436

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

800

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

[ziqingluo-90]

clang/lib/Analysis/UnsafeBufferUsage.cpp

805

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

430

Let's initialize Op to nullptr.

791

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

809

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

458–460

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.

792

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

794

Clang-format might complain about this.

[malavikasamak]

clang/lib/Analysis/UnsafeBufferUsage.cpp

458–460

Thank you. Fixed!

791

Good catch! Thank you!

809

Fixed!

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

791

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

791

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

791

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

805

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

[NoQ]

clang/lib/Analysis/UnsafeBufferUsage.cpp

791

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

791

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

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

 #endif
 
 WARNING_GADGET(Increment)
 WARNING_GADGET(Decrement)
 WARNING_GADGET(ArraySubscript)
 WARNING_GADGET(PointerArithmetic)
 WARNING_GADGET(UnsafeBufferUsageAttr)
 FIXABLE_GADGET(ULCArraySubscript)
+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;
+
+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().bind(BaseDeclRefExprTag)))
+             )
+           ).bind(OperatorTag);
+
+    return expr(isInUnspecifiedLvalueContext(Target));
+  }
+
+  DeclUseList getClaimedVarUseSites() const override {
+    return {BaseDeclRefExpr};
+  }
+
+  virtual const Stmt *getBaseStmt() const final {
+    return nullptr;
+  }
+
+   virtual std::optional<FixItList> getFixits(const Strategy &S) const override;
+};
+
 
 /// A call of a function or method that performs unchecked buffer operations
 /// over one of its pointer parameters.
 class UnsafeBufferUsageAttrGadget : public WarningGadget {
     constexpr static const char *const OpTag = "call_expr";
     const CallExpr *Op;
 
 public:
static std::optional<SourceLocation> getEndCharLoc(const NodeTy *Node,
   std::optional<Token> LastToken = Lexer::findNextToken(
       Node->getEndLoc().getLocWithOffset(-1), SM, LangOpts);
 
   if (!LastToken)
     return std::nullopt;
   return LastToken->getEndLoc().getLocWithOffset(-1);
 }
 
+std::optional<FixItList>
+PointerDereferenceGadget::getFixits(const Strategy &S) const {
+     if (const auto *DRE = dyn_cast<DeclRefExpr>(Op->getSubExpr()->IgnoreImpCasts()))
+      if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+        if (S.lookup(VD) == 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(DRE, SM, Ctx.getLangOpts());
+          if(endOfOperand) {
+            return FixItList{{
+              FixItHint::CreateRemoval(derefRange),
+              FixItHint::CreateInsertion(endOfOperand.value().getLocWithOffset(1), "[0]")
+            }};
+          }
+        }
+      }
+    llvm_unreachable("unsupported strategies for FixableGadgets");
+    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]"
+
+}