[-Wunsafe-buffer-usage] Check missing and duplicated assignments

Add checks for missing and duplicated assignments in bounds-attributed
groups. This model requires each mutable decl in a group to be assigned
exactly once.

rdar://161608243

Author: patrykstefanski

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

@@ -165,6 +165,22 @@ class UnsafeBufferUsageHandler {
                                              ASTContext &Ctx) {
     handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
   }
+
+  virtual void handleMissingAssignments(
+      const Expr *LastAssignInGroup,
+      const llvm::SmallPtrSetImpl<const ValueDecl *> &Required,
+      const llvm::SmallPtrSetImpl<const ValueDecl *> &Missing,
+      bool IsRelatedToDecl, ASTContext &Ctx) {
+    handleUnsafeOperation(LastAssignInGroup, IsRelatedToDecl, Ctx);
+  }
+
+  virtual void handleDuplicatedAssignment(const BinaryOperator *Assign,
+                                          const BinaryOperator *PrevAssign,
+                                          const ValueDecl *VD,
+                                          bool IsRelatedToDecl,
+                                          ASTContext &Ctx) {
+    handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
+  }
   /* TO_UPSTREAM(BoundsSafety) OFF */
 
   /// Invoked when a fix is suggested against a variable. This function groups

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -14283,6 +14283,12 @@ def warn_cannot_assign_to_immutable_bounds_attributed_object : Warning<
   "its type depends on an inout dependent count|"
   "it's used as dependent count in an inout count-attributed pointer"
   "}2">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_missing_assignments_in_bounds_attributed_group : Warning<
+  "bounds-attributed group requires assigning '%0', assignments to '%1' missing">,
+  InGroup<UnsafeBufferUsage>, DefaultIgnore;
+def warn_duplicated_assignment_in_bounds_attributed_group : Warning<
+  "duplicated assignment to %select{variable|parameter|member}0 %1 in "
+  "bounds-attributed group">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
 #ifndef NDEBUG
 // Not a user-facing diagnostic. Useful for debugging false negatives in
 // -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -5889,14 +5889,73 @@ static bool checkImmutableBoundsAttributedObjects(
   return IsGroupSafe;
 }
 
+// Checks if the bounds-attributed group has missing or duplicated assignments.
+// Each assignable decl in the group must be assigned exactly once.
+//
+// For example:
+//   void foo(int *__counted_by(a + b) p, int a, int b) {
+//     p = ...;
+//     p = ...; // duplicated
+//     a = ...;
+//     // b missing
+//   }
+// Note that the group consists of a, b, and p.
+//
+// The function returns true iff each assignable decl in the group is assigned
+// exactly once.
+static bool checkMissingAndDuplicatedAssignments(
+    const BoundsAttributedAssignmentGroup &Group,
+    UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) {
+  llvm::SmallDenseMap<const ValueDecl *, const BinaryOperator *, 4>
+      AssignedDecls;
+
+  DependentDeclSetTy RequiredDecls = Group.DeclClosure;
+  for (const ValueDecl *VD : Group.DeclClosure) {
+    if (VD->isDependentCountWithoutDeref() &&
+        VD->isDependentCountThatIsUsedInInoutPointer()) {
+      // This value is immutable, so it's not required to be assigned.
+      RequiredDecls.erase(VD);
+    }
+  }
+
+  DependentDeclSetTy MissingDecls = RequiredDecls;
+
+  bool IsGroupSafe = true;
+
+  for (size_t I = 0, N = Group.Assignments.size(); I < N; ++I) {
+    const BinaryOperator *Assign = Group.Assignments[I];
+    const ValueDecl *VD = Group.AssignedObjects[I].Decl;
+
+    const auto [It, Inserted] = AssignedDecls.insert({VD, Assign});
+    if (Inserted)
+      MissingDecls.erase(VD);
+    else {
+      const BinaryOperator *PrevAssign = It->second;
+      Handler.handleDuplicatedAssignment(Assign, PrevAssign, VD,
+                                         /*IsRelatedToDecl=*/false, Ctx);
+      IsGroupSafe = false;
+    }
+  }
+
+  if (!MissingDecls.empty()) {
+    const Expr *LastAssign = Group.Assignments.back();
+    Handler.handleMissingAssignments(LastAssign, RequiredDecls, MissingDecls,
+                                     /*IsRelatedToDecl=*/false, Ctx);
+    IsGroupSafe = false;
+  }
+
+  return IsGroupSafe;
+}
+
 // Checks if the bounds-attributed group is safe. This function returns false
 // iff the assignment group is unsafe and diagnostics have been emitted.
 static bool
 checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group,
                            UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) {
   if (!checkImmutableBoundsAttributedObjects(Group, Handler, Ctx))
     return false;
-
+  if (!checkMissingAndDuplicatedAssignments(Group, Handler, Ctx))
+    return false;
   // TODO: Add more checks.
   return true;
 }

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -2643,6 +2643,48 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
            diag::warn_cannot_assign_to_immutable_bounds_attributed_object)
         << getBoundsAttributedObjectKind(VD) << VD << Kind;
   }
+
+  static llvm::SmallString<64>
+  DeclSetToStr(const llvm::SmallPtrSetImpl<const ValueDecl *> &Set) {
+    llvm::SmallVector<const ValueDecl *, 4> V(Set.begin(), Set.end());
+    llvm::sort(V.begin(), V.end(), [](const ValueDecl *A, const ValueDecl *B) {
+      return A->getName().compare(B->getName()) < 0;
+    });
+    llvm::SmallString<64> Str;
+    for (const ValueDecl *VD : V) {
+      if (!Str.empty())
+        Str += ", ";
+      Str += VD->getName();
+    }
+    return Str;
+  }
+
+  void handleMissingAssignments(
+      const Expr *LastAssignInGroup,
+      const llvm::SmallPtrSetImpl<const ValueDecl *> &Required,
+      const llvm::SmallPtrSetImpl<const ValueDecl *> &Missing,
+      bool IsRelatedToDecl, ASTContext &Ctx) override {
+
+    llvm::SmallString<64> RequiredAssignments = DeclSetToStr(Required);
+    llvm::SmallString<64> MissingAssignments = DeclSetToStr(Missing);
+    auto Loc =
+        CharSourceRange::getTokenRange(LastAssignInGroup->getSourceRange())
+            .getEnd();
+
+    S.Diag(Loc, diag::warn_missing_assignments_in_bounds_attributed_group)
+        << RequiredAssignments << MissingAssignments;
+  }
+
+  void handleDuplicatedAssignment(const BinaryOperator *Assign,
+                                  const BinaryOperator *PrevAssign,
+                                  const ValueDecl *VD, bool IsRelatedToDecl,
+                                  ASTContext &Ctx) override {
+    S.Diag(Assign->getOperatorLoc(),
+           diag::warn_duplicated_assignment_in_bounds_attributed_group)
+        << getBoundsAttributedObjectKind(VD) << VD;
+    S.Diag(PrevAssign->getOperatorLoc(),
+           diag::note_bounds_safety_previous_assignment);
+  }
   /* TO_UPSTREAM(BoundsSafety) OFF */
 
   void handleUnsafeVariableGroup(const VarDecl *Variable,

clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp

@@ -211,6 +211,134 @@ class immutable_class {
   }
 };
 
+// Missing assignments
+
+void missing_ptr(int *__counted_by(count) p, int count) {
+  count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+}
+
+void missing_count(int *__counted_by(count) p, int count) {
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+}
+
+void missing_structure(int *__counted_by(count) p, int count) {
+  {
+    p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+  }
+  {
+    count = 0;   // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+  }
+}
+
+void missing_structure2(int *__counted_by(count) p, int count) {
+  p = nullptr;   // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+  {
+    count = 0;   // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+  }
+}
+
+void missing_structure3(int *__counted_by(count) p, int count) {
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+  if (count > 0) {
+    count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+  }
+}
+
+void missing_unrelated(int *__counted_by(count) p, int count, int *__counted_by(len) q, int len) {
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+  len = 0;     // expected-warning{{bounds-attributed group requires assigning 'len, q', assignments to 'q' missing}}
+}
+
+void missing_complex_count1(int *__counted_by(a + b) p, int a, int b) {
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'a, b' missing}}
+}
+
+void missing_complex_count2(int *__counted_by(a + b) p, int a, int b) {
+  p = nullptr;
+  a = 0; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'b' missing}}
+}
+
+void missing_complex_count3(int *__counted_by(a + b) p, int a, int b) {
+  b = 0;
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'a' missing}}
+}
+
+void missing_complex_count4(int *__counted_by(a + b) p, int a, int b) {
+  a = 0;
+  b = 0; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'p' missing}}
+}
+
+void missing_complex_ptr1(int *__counted_by(count) p, int *__counted_by(count) q, int count) {
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'count, q' missing}}
+}
+
+void missing_complex_ptr2(int *__counted_by(count) p, int *__counted_by(count) q, int count) {
+  p = nullptr;
+  q = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'count' missing}}
+}
+
+void missing_complex_ptr3(int *__counted_by(count) p, int *__counted_by(count) q, int count) {
+  count = 0;
+  p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'q' missing}}
+}
+
+void missing_complex_ptr4(int *__counted_by(count) p, int *__counted_by(count) q, int count) {
+  q = nullptr;
+  count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'p' missing}}
+}
+
+// Missing assignments in struct
+
+void missing_struct_ptr(cb *c) {
+  c->count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+}
+
+void missing_struct_count(cb *c) {
+  c->p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+}
+
+void missing_struct_unrelated(cb *c, cb *d) {
+  c->p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}}
+  d->count = 0;   // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}}
+}
+
+// Duplicated assignments
+
+void duplicated_ptr(int *__counted_by(count) p, int count) {
+  p = nullptr; // expected-note{{previously assigned here}}
+  p = nullptr; // expected-warning{{duplicated assignment to parameter 'p' in bounds-attributed group}}
+  count = 0;
+}
+
+void duplicated_ptr2(int *__counted_by(count) p, int count) {
+  p = nullptr; // expected-note{{previously assigned here}}
+  count = 0;
+  p = nullptr; // expected-warning{{duplicated assignment to parameter 'p' in bounds-attributed group}}
+}
+
+void duplicated_count(int *__counted_by(count) p, int count) {
+  p = nullptr;
+  count = 0; // expected-note{{previously assigned here}}
+  count = 0; // expected-warning{{duplicated assignment to parameter 'count' in bounds-attributed group}}
+}
+
+void duplicated_count2(int *__counted_by(count) p, int count) {
+  count = 0; // expected-note{{previously assigned here}}
+  p = nullptr;
+  count = 0; // expected-warning{{duplicated assignment to parameter 'count' in bounds-attributed group}}
+}
+
+void duplicated_complex(int *__counted_by(a + b) p,
+                        int *__counted_by(a + b + c) q,
+                        int a, int b, int c) {
+  p = nullptr;
+  q = nullptr; // expected-note{{previously assigned here}}
+  a = 0;
+  b = 0;
+  c = 0;
+  q = nullptr; // expected-warning{{duplicated assignment to parameter 'q' in bounds-attributed group}}
+}
+
 // Assigns to bounds-attributed that we consider too complex to analyze.
 
 void too_complex_assign_to_ptr(int *__counted_by(count) p, int count, int *q) {