[-Wunsafe-buffer-usage] Implement fixits from const size array to std::array

jkorous-apple · jkorous-apple · commit 38255d1ebe41 · 2024-01-30T15:52:22.000-08:00
Array subscript on a const size array is not bounds-checked. The idiomatic
replacement is std::array which is bounds-safe in hardened mode of libc++.

This commit extends the fixit-producing machine to consider std::array as a
transformation target type and teaches it to handle the array subscript on const
size arrays with a trivial (empty) fixit.

The transformation is straightforward for most cases:
T array[N]; =&gt; std::array&lt;T, N&gt; array;
but for array of const pointers it needs to be:
T* const array[N]; =&gt; const std::array&lt;T*, N&gt; array;

This case and other special cases are to be implemented in follow-up
patches.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -95,6 +95,8 @@ class UnsafeBufferUsageHandler {
 #endif
 
 public:
+  enum class TargetType { Span, Array };
+
   UnsafeBufferUsageHandler() = default;
   virtual ~UnsafeBufferUsageHandler() = default;
 
@@ -112,9 +114,11 @@ class UnsafeBufferUsageHandler {
   ///
   /// `D` is the declaration of the callable under analysis that owns `Variable`
   /// and all of its group mates.
-  virtual void handleUnsafeVariableGroup(const VarDecl *Variable,
-                                         const VariableGroupsManager &VarGrpMgr,
-                                         FixItList &&Fixes, const Decl *D) = 0;
+  virtual void
+  handleUnsafeVariableGroup(const VarDecl *Variable,
+                            const VariableGroupsManager &VarGrpMgr,
+                            FixItList &&Fixes, const Decl *D,
+                            const FixitStrategy &VarTargetTypes) = 0;
 
 #ifndef NDEBUG
 public:
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1495,6 +1495,7 @@ ULCArraySubscriptGadget::getFixits(const FixitStrategy &S) const {
         return FixItList{};
       }
       case FixitStrategy::Kind::Array:
+        return FixItList{};
       case FixitStrategy::Kind::Wontfix:
       case FixitStrategy::Kind::Iterator:
       case FixitStrategy::Kind::Vector:
@@ -2462,6 +2463,71 @@ static FixItList fixVariableWithSpan(const VarDecl *VD,
   return fixLocalVarDeclWithSpan(VD, Ctx, getUserFillPlaceHolder(), Handler);
 }
 
+static FixItList fixVarDeclWithArray(const VarDecl *D, const ASTContext &Ctx,
+                                     UnsafeBufferUsageHandler &Handler) {
+  FixItList FixIts{};
+
+  if (auto CAT = dyn_cast<clang::ConstantArrayType>(D->getType())) {
+    const QualType &ArrayEltT = CAT->getElementType();
+    assert(!ArrayEltT.isNull() && "Trying to fix a non-array type variable!");
+    // Producing fix-it for variable declaration---make `D` to be of std::array
+    // type:
+    SmallString<32> Replacement;
+    raw_svector_ostream OS(Replacement);
+
+    // For most types the transformation is simple:
+    //   T foo[10]; => std::array<T, 10> foo;
+    // Cv-specifiers are straigtforward:
+    //   const T foo[10]; => std::array<const T, 10> foo;
+    // Pointers are straightforward:
+    //   T * foo[10]; => std::array<T *, 10> foo;
+    //
+    // However, for const pointers the transformation is different:
+    //   T * const foo[10]; => const std::array<T *, 10> foo;
+    if (ArrayEltT->isPointerType() && ArrayEltT.isConstQualified()) {
+      DEBUG_NOTE_DECL_FAIL(D, " : const size array of const pointers");
+      // FIXME: implement the support
+      // FIXME: bail if the const pointer is a typedef
+      return {};
+    }
+
+    std::optional<StringRef> IdentText =
+        getVarDeclIdentifierText(D, Ctx.getSourceManager(), Ctx.getLangOpts());
+
+    if (!IdentText) {
+      DEBUG_NOTE_DECL_FAIL(D, " : failed to locate the identifier");
+      return {};
+    }
+
+    OS << "std::array<" << ArrayEltT.getAsString() << ", "
+       << getAPIntText(CAT->getSize()) << "> " << IdentText->str();
+
+    FixIts.push_back(FixItHint::CreateReplacement(
+        SourceRange{D->getBeginLoc(), D->getTypeSpecEndLoc()}, OS.str()));
+  }
+
+  return FixIts;
+}
+
+static FixItList fixVariableWithArray(const VarDecl *VD,
+                                      const DeclUseTracker &Tracker,
+                                      const ASTContext &Ctx,
+                                      UnsafeBufferUsageHandler &Handler) {
+  const DeclStmt *DS = Tracker.lookupDecl(VD);
+  assert(DS && "Fixing non-local variables not implemented yet!");
+  if (!DS->isSingleDecl()) {
+    // FIXME: to support handling multiple `VarDecl`s in a single `DeclStmt`
+    return {};
+  }
+  // Currently DS is an unused variable but we'll need it when
+  // non-single decls are implemented, where the pointee type name
+  // and the '*' are spread around the place.
+  (void)DS;
+
+  // FIXME: handle cases where DS has multiple declarations
+  return fixVarDeclWithArray(VD, Ctx, Handler);
+}
+
 // TODO: we should be consistent to use `std::nullopt` to represent no-fix due
 // to any unexpected problem.
 static FixItList
@@ -2507,7 +2573,13 @@ fixVariable(const VarDecl *VD, FixitStrategy::Kind K,
     DEBUG_NOTE_DECL_FAIL(VD, " : not a pointer");
     return {};
   }
-  case FixitStrategy::Kind::Array:
+  case FixitStrategy::Kind::Array: {
+    if (VD->isLocalVarDecl() && isa<clang::ConstantArrayType>(VD->getType()))
+      return fixVariableWithArray(VD, Tracker, Ctx, Handler);
+
+    DEBUG_NOTE_DECL_FAIL(VD, " : not a local const-size array");
+    return {};
+  }
   case FixitStrategy::Kind::Iterator:
   case FixitStrategy::Kind::Vector:
     llvm_unreachable("FixitStrategy not implemented yet!");
@@ -2696,7 +2768,10 @@ static FixitStrategy
 getNaiveStrategy(llvm::iterator_range<VarDeclIterTy> UnsafeVars) {
   FixitStrategy S;
   for (const VarDecl *VD : UnsafeVars) {
-    S.set(VD, FixitStrategy::Kind::Span);
+    if (isa<ConstantArrayType>(VD->getType()))
+      S.set(VD, FixitStrategy::Kind::Array);
+    else
+      S.set(VD, FixitStrategy::Kind::Span);
   }
   return S;
 }
@@ -3018,9 +3093,9 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
     auto FixItsIt = FixItsForVariableGroup.find(VD);
     Handler.handleUnsafeVariableGroup(VD, VarGrpMgr,
                                       FixItsIt != FixItsForVariableGroup.end()
-                                      ? std::move(FixItsIt->second)
-                                      : FixItList{},
-                                      D);
+                                          ? std::move(FixItsIt->second)
+                                          : FixItList{},
+                                      D, NaiveStrategy);
     for (const auto &G : WarningGadgets) {
       Handler.handleUnsafeOperation(G->getBaseStmt(), /*IsRelatedToDecl=*/true,
                                     D->getASTContext());
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2297,7 +2297,8 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,
                                  const VariableGroupsManager &VarGrpMgr,
-                                 FixItList &&Fixes, const Decl *D) override {
+                                 FixItList &&Fixes, const Decl *D,
+                                 const FixitStrategy &VarTargetTypes) override {
     assert(!SuggestSuggestions &&
            "Unsafe buffer usage fixits displayed without suggestions!");
     S.Diag(Variable->getLocation(), diag::warn_unsafe_buffer_variable)
@@ -2312,7 +2313,18 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
       // NOT explain how the variables are grouped as the reason is non-trivial
       // and irrelavant to users' experience:
       const auto VarGroupForVD = VarGrpMgr.getGroupOfVar(Variable, &BriefMsg);
-      unsigned FixItStrategy = 0; // For now we only have 'std::span' strategy
+      unsigned FixItStrategy = 0;
+      switch (VarTargetTypes.lookup(Variable)) {
+      case clang::FixitStrategy::Kind::Span:
+        FixItStrategy = 0;
+        break;
+      case clang::FixitStrategy::Kind::Array:
+        FixItStrategy = 1;
+        break;
+      default:
+        assert(false && "We support only std::span and std::array");
+      };
+
       const auto &FD =
           S.Diag(Variable->getLocation(),
                  BriefMsg ? diag::note_unsafe_buffer_variable_fixit_together
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -verify %s
+
+// CHECK-NOT: [-Wunsafe-buffer-usage]
+
+
+void foo(unsigned idx) {
+  int buffer[10];         // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds}}
+                          // expected-note@-1{{change type of 'buffer' to 'std::array' to preserve bounds information}}
+  buffer[idx] = 0;        // expected-note{{used in buffer access here}}
+}
+
+int global_buffer[10];    // expected-warning{{'global_buffer' is an unsafe buffer that does not perform bounds}}
+void foo2(unsigned idx) {
+  global_buffer[idx] = 0;        // expected-note{{used in buffer access here}}
+}
+
+struct Foo {
+  int member_buffer[10];
+};
+void foo2(Foo& f, unsigned idx) {
+  f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-debug.cpp
@@ -32,13 +32,11 @@ void foo() {
                         // debug-note{{safe buffers debug: gadget 'ULCArraySubscript' refused to produce a fix}}
 }
 
-void failed_decl() {
-  int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} \
-              // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : not a pointer}}
-  
-  for (int i = 0; i < 10; i++) {
-    a[i] = i;  // expected-note{{used in buffer access here}}
-  }
+void failed_decl(const int* out, unsigned idx) {
+  const int* const a[10] = {nullptr};  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}} \
+              // debug-note{{safe buffers debug: failed to produce fixit for declaration 'a' : const size array of const pointers}}
+
+  out = a[idx];  // expected-note{{used in buffer access here}}
 }
 
 void failed_multiple_decl() {
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-array.cpp
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array(unsigned idx) {
+  int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+  buffer[idx] = 0;
+}
+
+void unsupported_multi_decl1(unsigned idx) {
+  int a, buffer[10];
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  buffer[idx] = 0;
+}
+
+void unsupported_multi_decl2(unsigned idx) {
+  int buffer[10], b;
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  buffer[idx] = 0;
+}
+
+void local_array_ptr_to_const(unsigned idx, const int*& a) {
+  const int * buffer[10] = {a};
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:25}:"std::array<const int *, 10> buffer"
+  a = buffer[idx];
+}
+
+void local_array_const_ptr(unsigned idx, int*& a) {
+  int * const buffer[10] = {a};
+// FIXME: implement support
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  a = buffer[idx];
+
+}
+
+void local_array_const_ptr_to_const(unsigned idx, const int*& a) {
+  const int * const buffer[10] = {a};
+// FIXME: implement support
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  a = buffer[idx];
+
+}
+
+template<typename T>
+void local_array_in_template(unsigned idx) {
+  T buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:.*-[[@LINE-1]]:.*}
+  buffer[idx] = 0;
+}
+// Instantiate the template function to force its analysis.
+template void local_array_in_template<int>(unsigned); // FIXME: expected note {{in instantiation of}}
+
+void macro_as_identifier(unsigned idx) {
+#define MY_BUFFER buffer
+  // Although fix-its include macros, the macros do not overlap with
+  // the bounds of the source range of these fix-its. So these fix-its
+  // are valid.
+
+  int MY_BUFFER[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:20}:"std::array<int, 10> MY_BUFFER"
+  MY_BUFFER[idx] = 0;
+
+#undef MY_BUFFER
+}
+
+void unsupported_fixit_overlapping_macro(unsigned idx) {
+#define MY_INT int
+  MY_INT buffer[10];
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+  buffer[idx] = 0;
+#undef MY_INT
+}
+
+void subscript_negative() {
+  int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+
+  // For constant-size arrays any negative index will lead to buffer underflow.
+  // std::array::operator[] has unsigned parameter so the value will be casted to unsigned.
+  // This will very likely be buffer overflow but hardened std::array catch these at runtime.
+  buffer[-5] = 0;
+}
+
+void subscript_signed(int signed_idx) {
+  int buffer[10];
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:17}:"std::array<int, 10> buffer"
+
+  // For constant-size arrays any negative index will lead to buffer underflow.
+  // std::array::operator[] has unsigned parameter so the value will be casted to unsigned.
+  // This will very likely be buffer overflow but hardened std::array catch these at runtime.
+  buffer[signed_idx] = 0;
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -61,6 +61,7 @@ void testArraySubscripts(int *p, int **pp) {
       );
 
     int a[10];          // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+                        // expected-note@-1{{change type of 'a' to 'std::array' to preserve bounds information}}
     int b[10][10];      // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
   foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
@@ -174,6 +175,7 @@ auto file_scope_lambda = [](int *ptr) {
 void testLambdaCapture() {
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
   int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'b' to 'std::array' to preserve bounds information}}
   int c[10];
 
   auto Lam1 = [a]() {
@@ -191,7 +193,9 @@ void testLambdaCapture() {
 
 void testLambdaImplicitCapture() {
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'a' to 'std::array' to preserve bounds information}}
   int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
+                          // expected-note@-1{{change type of 'b' to 'std::array' to preserve bounds information}}
   
   auto Lam1 = [=]() {
     return a[1];           // expected-note{{used in buffer access here}}
@@ -344,6 +348,7 @@ template<typename T> void fArr(T t[]) {
   // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}}
   foo(t[1]);    // expected-note{{used in buffer access here}}
   T ar[8];      // expected-warning{{'ar' is an unsafe buffer that does not perform bounds checks}}
+                // expected-note@-1{{change type of 'ar' to 'std::array' to preserve bounds information}}
   foo(ar[5]);   // expected-note{{used in buffer access here}}
 }
 
