Validate that positional arguments in a format string are self-consistent

Author: apple-fcloutier

clang/include/clang/Sema/Sema.h

@@ -2356,7 +2356,8 @@ class Sema final : public SemaBase {
 
   /// Verify that two format strings (as understood by attribute(format) and
   /// attribute(format_matches) are compatible. If they are incompatible,
-  /// diagnostics will assume that \c AuthoritativeFormatString is correct and
+  /// diagnostics are emitted with the assumption that \c
+  /// AuthoritativeFormatString is correct and
   /// \c TestedFormatString is wrong. If \c FunctionCallArg is provided,
   /// diagnostics will point to it and a note will refer to \c
   /// TestedFormatString or \c AuthoritativeFormatString as appropriate.
@@ -2366,6 +2367,12 @@ class Sema final : public SemaBase {
                                const StringLiteral *TestedFormatString,
                                const Expr *FunctionCallArg = nullptr);
 
+  /// Verify that one format string (as understood by attribute(format)) is
+  /// self-consistent; for instance, that it doesn't have multiple positional
+  /// arguments referring to the same argument in incompatible ways. Diagnose
+  /// if it isn't.
+  bool ValidateFormatString(FormatStringType FST, const StringLiteral *Str);
+
   /// \brief Enforce the bounds of a TCB
   /// CheckTCBEnforcement - Enforces that every function in a named TCB only
   /// directly calls other functions in the same TCB as marked by the

clang/lib/Sema/SemaChecking.cpp

@@ -8664,6 +8664,39 @@ bool Sema::CheckFormatStringsCompatible(
   return false;
 }
 
+bool Sema::ValidateFormatString(FormatStringType Type,
+                                const StringLiteral *Str) {
+  if (Type != Sema::FST_Printf && Type != Sema::FST_NSString &&
+      Type != Sema::FST_Kprintf && Type != Sema::FST_FreeBSDKPrintf &&
+      Type != Sema::FST_OSLog && Type != Sema::FST_OSTrace &&
+      Type != Sema::FST_Syslog)
+    return true;
+
+  FormatStringLiteral RefLit = Str;
+  llvm::SmallVector<EquatableFormatArgument, 9> Args;
+  bool IsObjC = Type == Sema::FST_NSString || Type == Sema::FST_OSTrace;
+  if (!DecomposePrintfHandler::GetSpecifiers(*this, &RefLit, Str, Type, IsObjC,
+                                             true, Args))
+    return false;
+
+  // If positional arguments are used multiple times in the same format string,
+  // ensure that they are used in compatible ways.
+  bool HadError = false;
+  auto Iter = Args.begin();
+  auto End = Args.end();
+  while (Iter != End) {
+    auto PosEnd = std::find_if(Iter + 1, End, [=](const auto &Arg) {
+      return Arg.getPosition() != Iter->getPosition();
+    });
+    for (auto PosIter = Iter + 1; PosIter != PosEnd; ++PosIter) {
+      HadError |= !PosIter->VerifyCompatible(*this, *Iter, Str, true);
+    }
+    Iter = PosEnd;
+  }
+
+  return !HadError;
+}
+
 bool Sema::FormatStringHasSArg(const StringLiteral *FExpr) {
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();

clang/lib/Sema/SemaDeclAttr.cpp

@@ -3832,13 +3832,12 @@ static void handleFormatMatchesAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 
   Expr *FormatStrExpr = AL.getArgAsExpr(2)->IgnoreParenImpCasts();
   if (auto *SL = dyn_cast<StringLiteral>(FormatStrExpr)) {
-    // Aside from whether we started with a string literal, there is generally
-    // no useful checking that we can do here. For instance, "invalid" format
-    // specifiers (like %!) are simply ignored by printf and don't have a
-    // diagnostic.
-    if (auto *NewAttr = S.mergeFormatMatchesAttr(D, AL, Info.Identifier,
-                                                 Info.FormatStringIdx, SL))
-      D->addAttr(NewAttr);
+    Sema::FormatStringType FST =
+        S.GetFormatStringType(Info.Identifier->getName());
+    if (S.ValidateFormatString(FST, SL))
+      if (auto *NewAttr = S.mergeFormatMatchesAttr(D, AL, Info.Identifier,
+                                                   Info.FormatStringIdx, SL))
+        D->addAttr(NewAttr);
     return;
   }
 

clang/test/Sema/format-string-matches.c

@@ -3,15 +3,27 @@
 
 #include <stdarg.h>
 
+__attribute__((format_matches(printf, -1, "%s"))) // expected-error{{'format_matches' attribute parameter 2 is out of bounds}}
+int test_out_of_bounds(void);
+
+__attribute__((format_matches(printf, 0, "%s"))) // expected-error{{'format_matches' attribute parameter 2 is out of bounds}}
+int test_out_of_bounds(void);
+
+__attribute__((format_matches(printf, 1, "%s"))) // expected-error{{'format_matches' attribute parameter 2 is out of bounds}}
+int test_out_of_bounds(void);
+
+__attribute__((format_matches(printf, 1, "%s"))) // expected-error{{format argument not a string type}}
+int test_out_of_bounds_int(int x);
+
+// MARK: -
+// Calling printf with a format from format_matches(printf) diagnoses with
+// that format string
 __attribute__((format(printf, 1, 2)))
 int printf(const char *fmt, ...);
 
 __attribute__((format(printf, 1, 0)))
 int vprintf(const char *fmt, va_list);
 
-// MARK: -
-// Calling printf with a format from format_matches(printf) diagnoses with
-// that format string
 __attribute__((format_matches(printf, 1, "%s %1.5s")))
 void format_str_str0(const char *fmt) {
     printf(fmt, "hello", "world");
@@ -206,3 +218,16 @@ void test_merge_redecl_warn(const char *f);
 
 __attribute__((format_matches(printf, 1, "%s"))) // expected-note{{comparing with this specifier}}
 void test_merge_redecl_warn(const char *f);
+
+// MARK: -
+// Positional madness
+
+__attribute__((format_matches(printf, 1, "%1$s %1$d")))  // \
+    expected-warning{{format specifier 's' is incompatible with 'd'}} \
+    expected-note{{comparing with this specifier}}
+void test_positional_incompatible(const char *f);
+
+void call_positional_incompatible(void) {
+    // expect the attribute was dropped and that there is no diagnostic here
+    test_positional_incompatible("%d %d %d %d %d");
+}