[analyzer] Fix format attribute handling in GenericTaintChecker by NagyDonat · Pull Request #132765 · llvm/llvm-project (original) (raw)

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

Previously optin.taint.GenericTaint misinterpreted the parameter indices and produced false positives in situations when a format attribute is applied on a non-static method. This commit fixes this bug

Full diff: https://github.com/llvm/llvm-project/pull/132765.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp (+16)
• (modified) clang/test/Analysis/taint-generic.cpp (+42)

diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index b89a6e2588c98..1b61370a580d9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -1080,7 +1080,23 @@ static bool getPrintfFormatArgumentNum(const CallEvent &Call, const ArgIdxTy CallNumArgs = fromArgumentCount(Call.getNumArgs());

for (const auto *Format : FDecl->specific_attrs()) {

• // The format attribute uses 1-based parameter indexing, for example
• // plain printf(const char *fmt, ...) would be annotated with
• // __format__(__printf__, 1, 2), so we need to subtract 1 to get a
• // 0-based index. (This checker uses 0-based parameter indices.) ArgNum = Format->getFormatIdx() - 1;
• // The format attribute also counts the implicit this parameter of
• // methods, so e.g. in SomeClass::method(const char *fmt, ...) could be
• // annotated with __format__(__printf__, 2, 3). This checker doesn't
• // count the implicit this parameter, so in this case we need to subtract
• // one again.
• // FIXME: Apparently the implementation of the format attribute doesn't
• // support methods with an explicit object parameter, so we cannot
• // implement proper support for that rare case either.
• const CXXMethodDecl *MDecl = dyn_cast(FDecl);
• if (MDecl && !MDecl->isStatic())

•  ArgNum--;

• if ((Format->getType()->getName() == "printf") && CallNumArgs > ArgNum)
    return true;

  } diff --git a/clang/test/Analysis/taint-generic.cpp b/clang/test/Analysis/taint-generic.cpp index 8836e1d3d2d98..3bf3ca3a75099 100644 --- a/clang/test/Analysis/taint-generic.cpp +++ b/clang/test/Analysis/taint-generic.cpp @@ -161,3 +161,45 @@ void top() { clang_analyzer_isTainted(A.data); // expected-warning {{YES}} } } // namespace gh114270
• +namespace format_attribute { +attribute((format (printf, 1, 2))) +void log_nonmethod(const char *fmt, ...);
• +void test_format_attribute_nonmethod() {
• int n;
• fscanf(stdin, "%d", &n); // Get a tainted value.
• log_nonmethod("This number is suspicious: %d\n", n); // no-warning +}
• +struct Foo {
• // When the format attribute is applied to a method, argumet '1' is the
• // implicit this, so e.g. in this case argument '2' specifies fmt.
• // Specifying '1' instead of '2' would produce a compilation error:
• // "format attribute cannot specify the implicit this argument as the format string"
• attribute((format (printf, 2, 3)))
• void log_method(const char *fmt, ...);
• void test_format_attribute_method() {
• int n;
• fscanf(stdin, "%d", &n); // Get a tainted value.
• // The analyzer used to misinterpret the parameter indices in the format
• // attribute when the format attribute is applied to a method.
• log_method("This number is suspicious: %d\n", n); // no-warning
• }
• attribute((format (printf, 1, 2)))
• static void log_static_method(const char *fmt, ...);
• void test_format_attribute_static_method() {
• int n;
• fscanf(stdin, "%d", &n); // Get a tainted value.
• log_static_method("This number is suspicious: %d\n", n); // no-warning
• } +};
• +} // namespace format_attribute