Author: Ziqing Luo Date: 2026-01-08T09:20:27-08:00 New Revision: 2926b4100db1db2322c8e713cb33eccb7e82132b
URL: https://github.com/llvm/llvm-project/commit/2926b4100db1db2322c8e713cb33eccb7e82132b DIFF: https://github.com/llvm/llvm-project/commit/2926b4100db1db2322c8e713cb33eccb7e82132b.diff LOG: [-Wunsafe-buffer-usage] Fix crashes in "Add check for custom printf/scanf functions #173096" (#174683) The previous PR #173096 assumes that format attribute parameters always refer to valid indices of arguments. It is a wrong assumption in itself because the second attribute parameter could specify the index after the last named parameter for variadic functions and no actual arguments passed beyond named parameters. In addition, clang (possibly incorrectly) allows the following uses of the attribute: ``` void f(const char *) __attribute__((__format__ (__printf__, 1, 2))); // The second attribute argument 2 will not refer to any valid argument at any call of 'f' void g(const char *) __attribute__((__format__ (__printf__, 1, 99))); // Clang is even quiet on this, if assertions are disabled :( ``` Added: Modified: clang/lib/Analysis/UnsafeBufferUsage.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index da19b9d3902f1..7c0eaa2e589f5 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -29,7 +29,6 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/APSInt.h" #include "llvm/ADT/STLFunctionalExtras.h" -#include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include <cstddef> @@ -756,6 +755,9 @@ static const Expr *tryConstantFoldConditionalExpr(const Expr *E, // A pointer type expression is known to be null-terminated, if it has the // form: E.c_str(), for any expression E of `std::string` type. static bool isNullTermPointer(const Expr *Ptr, ASTContext &Ctx) { + // Strip CXXDefaultArgExpr before check: + if (const auto *DefaultArgE = dyn_cast<CXXDefaultArgExpr>(Ptr)) + Ptr = DefaultArgE->getExpr(); Ptr = tryConstantFoldConditionalExpr(Ptr, Ctx); if (isa<clang::StringLiteral>(Ptr->IgnoreParenImpCasts())) return true; @@ -2221,17 +2223,41 @@ class UnsafeFormatAttributedFunctionCallGadget : public WarningGadget { }); const Expr *UnsafeArg; - if (AnyAttr && !IsPrintf && - (CE->getNumArgs() >= static_cast<unsigned>(Attr->getFirstArg()))) { - // for scanf-like functions, any format argument is considered unsafe: + if (!AnyAttr) + return false; + + // FormatAttribute indexes are 1-based: + unsigned FmtIdx = Attr->getFormatIdx() - 1; + std::optional<unsigned> FmtArgIdx = Attr->getFirstArg() - 1; + + if (isa<CXXMemberCallExpr>(CE)) { + // For CXX member calls, attribute parameters are specified as if there is + // an implicit "this". The implicit "this" is invisible through CallExpr + // `CE`. (What makes it even less ergonomic is that the + // implicit "this" is visible through CallExpr `CE` for CXX operator + // calls!) + --FmtIdx; + --*FmtArgIdx; + } else if (CE->getStmtClass() != Stmt::CallExprClass && + !isa<CXXOperatorCallExpr>(CE)) + return false; // Ignore unsupported CallExpr subclasses + if (*FmtArgIdx >= CE->getNumArgs()) + // Format arguments are allowed to be absent when variadic parameter is + // used. So we need to check if those arguments exist. Moreover, when + // variadic parameter is NOT used, `Attr->getFirstArg()` could be an + // out-of-bound value. E.g., + // clang does not complain about `__attribute__((__format__(__printf__, 2, + // 99))) void f(int, char *);`. + FmtArgIdx = std::nullopt; + + if (AnyAttr && !IsPrintf && FmtArgIdx) { + // For scanf-like functions, any format argument is considered unsafe: Result.addNode(Tag, DynTypedNode::create(*CE)); return true; } + // For printf-like functions: if (AnyAttr && libc_func_matchers::hasUnsafeFormatOrSArg( - Ctx, CE, UnsafeArg, - // FormatAttribute indexes are 1-based: - /* FmtIdx= */ Attr->getFormatIdx() - 1, - /* FmtArgIdx= */ Attr->getFirstArg() - 1)) { + Ctx, CE, UnsafeArg, FmtIdx, FmtArgIdx)) { Result.addNode(Tag, DynTypedNode::create(*CE)); Result.addNode(UnsafeStringTag, DynTypedNode::create(*UnsafeArg)); return true; diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp index facbb0eb995e9..d824463ad9618 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp @@ -136,6 +136,10 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) { snprintf(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn snwprintf(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn snwprintf_s(s.data(), s.size_bytes(), "%s%d", __PRETTY_FUNCTION__, *p); // no warn + snprintf(s.data(), s.size_bytes(), "%s%d"); // no warn + snprintf(s.data(), s.size_bytes(), "%s%d"); // no warn + snwprintf(s.data(), s.size_bytes(), "%s%d"); // no warn + snwprintf_s(s.data(), s.size_bytes(), "%s%d"); // no warn wprintf(L"hello %ls", L"world"); // no warn wprintf(L"hello %ls", WS.c_str()); // no warn strlen("hello");// no warn @@ -265,7 +269,30 @@ void myprintf_2(const char *, int, const char *) __attribute__((__format__ (__pr void myprintf_3(const char *, const char *, int, const char *) __attribute__((__format__ (__printf__, 2, 4))); void myscanf(const char *, ...) __attribute__((__format__ (__scanf__, 1, 2))); -void test_myprintf(char * Str, std::string StdStr) { +void myprintf_default(const char *, const char * Fmt = "hello %s", + int X = 0, const char * Str = "world") __attribute__((__format__ (__printf__, 2, 4))); + +struct FormatAttrTestMember { + void myprintf(const char *, ...) __attribute__((__format__ (__printf__, 2, 3))); + void myprintf_2(const char *, int, const char *) __attribute__((__format__ (__printf__, 2, 4))); + void myprintf_3(const char *, const char *, int, const char *) __attribute__((__format__ (__printf__, 3, 5))); + void myscanf(const char *, ...) __attribute__((__format__ (__scanf__, 2, 3))); + + void operator()(const char * Fmt, ...) __attribute__((__format__ (__printf__, 2, 3))); + void operator[](const char * Fmt) __attribute__((__format__ (__printf__, 2, 3))); + + + static const char * StrField; + + void myprintf_default(const char *, const char * Fmt = "hello %s", + int X = 0, const char * Str = StrField) __attribute__((__format__ (__printf__, 3, 5))); +}; + +struct FormatAttrTestMember2 { + void operator()(const char * Fmt, char *) __attribute__((__format__ (__printf__, 2, 3))); +}; + +void test_format_attr(char * Str, std::string StdStr) { myprintf("hello", Str); myprintf("hello %s", StdStr.c_str()); myprintf("hello %s", Str); // expected-warning{{function 'myprintf' is unsafe}} \ @@ -280,11 +307,65 @@ void test_myprintf(char * Str, std::string StdStr) { myprintf_3("irrelevant", "hello %s", 0, StdStr.c_str()); myprintf_3("irrelevant", "hello %s", 0, Str); // expected-warning{{function 'myprintf_3' is unsafe}} \ expected-note{{string argument is not guaranteed to be null-terminated}} - myscanf("hello %s"); myscanf("hello %s", Str); // expected-warning{{function 'myscanf' is unsafe}} + myprintf_default("irrelevant"); + int X; myscanf("hello %d", &X); // expected-warning{{function 'myscanf' is unsafe}} + + // Test member functions: + FormatAttrTestMember Obj; + + Obj.myprintf("hello", Str); + Obj.myprintf("hello %s", StdStr.c_str()); + Obj.myprintf("hello %s", Str); // expected-warning{{function 'myprintf' is unsafe}} \ + expected-note{{string argument is not guaranteed to be null-terminated}} + + Obj.myprintf_2("hello", 0, Str); + Obj.myprintf_2("hello %s", 0, StdStr.c_str()); + Obj.myprintf_2("hello %s", 0, Str); // expected-warning{{function 'myprintf_2' is unsafe}} \ + expected-note{{string argument is not guaranteed to be null-terminated}} + + Obj.myprintf_3("irrelevant", "hello", 0, Str); + Obj.myprintf_3("irrelevant", "hello %s", 0, StdStr.c_str()); + Obj.myprintf_3("irrelevant", "hello %s", 0, Str); // expected-warning{{function 'myprintf_3' is unsafe}} \ + expected-note{{string argument is not guaranteed to be null-terminated}} + + Obj.myscanf("hello %s"); + Obj.myscanf("hello %s", Str); // expected-warning{{function 'myscanf' is unsafe}} + + Obj.myscanf("hello %d", &X); // expected-warning{{function 'myscanf' is unsafe}} + + Obj.myprintf_default("irrelevant"); // expected-warning{{function 'myprintf_default' is unsafe}} + // expected-note@*{{string argument is not guaranteed to be null-terminated}} + + Obj("hello", Str); + Obj("hello %s", StdStr.c_str()); + Obj("hello %s", Str); // expected-warning{{function 'operator()' is unsafe}} \ + expected-note{{string argument is not guaranteed to be null-terminated}} + Obj["hello"]; + Obj["hello %s"]; + + FormatAttrTestMember2 Obj2; + + Obj2("hello", Str); + Obj2("hello %s", StdStr.c_str()); + Obj2("hello %s", Str); // expected-warning{{function 'operator()' is unsafe}} \ + expected-note{{string argument is not guaranteed to be null-terminated}} +} + +// The second attribute argument, which points the starting index of +// format string arguments, may not be a valid argument index: +void myprintf_arg_idx_oob(const char *) __attribute__((__format__ (__printf__, 1, 2))); + +void test_format_attr_invalid_arg_idx(char * Str, std::string StdStr) { + myprintf_arg_idx_oob("hello"); + myprintf_arg_idx_oob(Str); // expected-warning{{function 'myprintf_arg_idx_oob' is unsafe}} expected-note{{string argument is not guaranteed to be null-terminated}} + myprintf_arg_idx_oob(StdStr.c_str()); + myprintf("hello"); + myprintf(Str); // expected-warning{{function 'myprintf' is unsafe}} expected-note{{string argument is not guaranteed to be null-terminated}} + myprintf(StdStr.c_str()); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
