Reduce false positives in warnings about printfs

- Predefined name macros such as `__PRETTY_FUNCTION__` are considered safe. - We need to distinguish between `%s` and `%p` as both accept pointers but the latter is not a buffer operation. This leaves us no choice other than parsing the format string. Fortunately, the building blocks of format parsing have already existed and are quite handy.

Author: ziqingluo-90

clang/include/clang/AST/FormatString.h

@@ -783,6 +783,18 @@ bool ParsePrintfString(FormatStringHandler &H,
 bool ParseFormatStringHasSArg(const char *beg, const char *end,
  const LangOptions &LO, const TargetInfo &Target);
 
+/// Parse C format string and return index (relative to `ArgIndex`) of the first
+/// found `s` specifier. Return 0 if not found.
+/// \param I The start of the C format string; Updated to the first unparsed
+/// position upon return.
+/// \param E The end of the C format string;
+/// \param ArgIndex The argument index of the last found `s` specifier; Or the
+/// argument index of the formatter in initial case.
+unsigned ParseFormatStringFirstSArgIndex(const char *&I, const char *E,
+ unsigned ArgIndex,
+ const LangOptions &LO,
+ const TargetInfo &Target);
+
 bool ParseScanfString(FormatStringHandler &H,
  const char *beg, const char *end, const LangOptions &LO,
  const TargetInfo &Target);

clang/lib/AST/PrintfFormatString.cpp

@@ -483,6 +483,34 @@ bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I,
  return false;
 }
 
+unsigned clang::analyze_format_string::ParseFormatStringFirstSArgIndex(
+ const char *&I, const char *E, unsigned ArgIndex, const LangOptions &LO,
+ const TargetInfo &Target) {
+ unsigned argIndex = ArgIndex;
+
+ // Keep looking for a %s format specifier until we have exhausted the string.
+ FormatStringHandler H;
+ while (I != E) {
+ const PrintfSpecifierResult &FSR =
+ ParsePrintfSpecifier(H, I, E, argIndex, LO, Target, false, false);
+ // Did a fail-stop error of any kind occur when parsing the specifier?
+ // If so, don't do any more processing.
+ if (FSR.shouldStop())
+ return false;
+ // Did we exhaust the string or encounter an error that
+ // we can recover from?
+ if (!FSR.hasValue())
+ continue;
+ const analyze_printf::PrintfSpecifier &FS = FSR.getValue();
+ // Return true if this a %s format specifier.
+ if (FS.getConversionSpecifier().getKind() ==
+ ConversionSpecifier::Kind::sArg) {
+ return FS.getPositionalArgIndex();
+ }
+ }
+ return false;
+}
+
 bool clang::analyze_format_string::parseFormatStringHasFormattingSpecifiers(
  const char *Begin, const char *End, const LangOptions &LO,
  const TargetInfo &Target) {

clang/lib/Analysis/UnsafeBufferUsage.cpp

@@ -12,6 +12,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/FormatString.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/StmtVisitor.h"
@@ -611,6 +612,38 @@ static bool isNullTermPointer(const Expr *Ptr) {
  return false;
 }
 
+// Return true iff at least one of following cases holds:
+// 1. Format string is a literal and there is an unsafe pointer argument
+// corresponding to an `s` specifier;
+// 2. Format string is not a literal and there is least an unsafe pointer
+// argument (including the formatter argument).
+static bool hasUnsafeFormatOrSArg(const Expr *Fmt, unsigned FmtArgIdx,
+ const CallExpr *Call, ASTContext &Ctx) {
+ if (auto *SL = dyn_cast<StringLiteral>(Fmt->IgnoreParenImpCasts())) {
+ StringRef FmtStr = SL->getString();
+ auto I = FmtStr.begin();
+ auto E = FmtStr.end();
+ unsigned ArgIdx = FmtArgIdx;
+
+ do {
+ ArgIdx = analyze_format_string::ParseFormatStringFirstSArgIndex(
+ I, E, ArgIdx, Ctx.getLangOpts(), Ctx.getTargetInfo());
+ if (ArgIdx && Call->getNumArgs() > ArgIdx &&
+ !isNullTermPointer(Call->getArg(ArgIdx)))
+ return true;
+ } while (ArgIdx);
+ return false;
+ }
+ // If format is not a string literal, we cannot analyze the format string.
+ // In this case, this call is considered unsafe if at least one argument
+ // (including the format argument) is unsafe pointer.
+ return llvm::any_of(
+ llvm::make_range(Call->arg_begin() + FmtArgIdx, Call->arg_end()),
+ [](const Expr *Arg) {
+ return Arg->getType()->isPointerType() && !isNullTermPointer(Arg);
+ });
+}
+
 // Matches a call to one of the `-printf" functions (excluding the ones with
 // va_list, or `-sprintf`s) that taking pointer-to-char-as-string arguments but
 // fail to guarantee their null-termination. In other words, these calls are
@@ -626,19 +659,18 @@ AST_MATCHER_P(CallExpr, unsafeStringInPrintfs, StringRef, CoreName) {
  if (Prefix.ends_with("w"))
  Prefix = Prefix.drop_back(1);
 
- auto AnyUnsafeStrPtr = [](const Expr *Arg) -> bool {
- return Arg->getType()->isPointerType() && !isNullTermPointer(Arg);
- };
-
  if (Prefix.empty() ||
  Prefix == "k") // printf: all pointer args should be null-terminated
- return any_of(Node.arguments(), AnyUnsafeStrPtr);
- if (Prefix == "f" && Node.getNumArgs() > 1)
- return any_of(llvm::make_range(Node.arg_begin() + 1, Node.arg_end()),
- AnyUnsafeStrPtr);
- if (Prefix == "sn" && Node.getNumArgs() > 2) {
- return any_of(llvm::make_range(Node.arg_begin() + 2, Node.arg_end()),
- AnyUnsafeStrPtr);
+ return hasUnsafeFormatOrSArg(Node.getArg(0), 0, &Node,
+ Finder->getASTContext());
+ if (Prefix == "f")
+ return hasUnsafeFormatOrSArg(Node.getArg(1), 1, &Node,
+ Finder->getASTContext());
+ if (Prefix == "sn") {
+ // The first two arguments need to be in safe patterns, which is checked
+ // by `isSafeSizedby`:
+ return hasUnsafeFormatOrSArg(Node.getArg(2), 2, &Node,
+ Finder->getASTContext());
  }
  return false; // A call to a "-printf" falls into another category.
 }

clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp

@@ -70,13 +70,20 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) {
  sscanf(p, "%s%d", "hello", *p); // expected-warning{{function sscanf introduces unsafe buffer access}}
  sscanf_s(p, "%s%d", "hello", *p); // expected-warning{{function sscanf_s introduces unsafe buffer access}}
  fprintf((FILE*)p, "%P%d%p%i hello world %32s", *p, *p, p, *p, p); // expected-warning{{function fprintf introduces unsafe buffer access}} expected-note{{use 'std::string::c_str' or string literal as string pointer to guarantee null-termination}}
+ fprintf((FILE*)p, "%P%d%p%i hello world %32s", *p, *p, p, *p, "hello"); // no warn
  printf("%s%d", "hello", *p); // no warn
  snprintf(s.data(), s.size_bytes(), "%s%d", "hello", *p); // no warn
+ snprintf(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn
  strlen("hello");// no warn
 }
 
-void v(std::string s1) {
- snprintf(s1.data(), s1.size_bytes(), "%s%d", s1.c_str(), 0); // no warn
+void v(std::string s1, int *p) {
+ snprintf(s1.data(), s1.size_bytes(), "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str()); // no warn
+ snprintf(s1.data(), s1.size_bytes(), s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str()); // no warn
+ printf("%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str()); // no warn
+ printf(s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str()); // no warn
+ fprintf((FILE*)0, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str()); // no warn
+ fprintf((FILE*)0, s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str()); // no warn
 }