=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/132...@github.com>
llvmbot wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> Previously `optin.taint.GenericTaint` misinterpreted the parameter indices and produced false positives in situations when a [format attribute](https://clang.llvm.org/docs/AttributeReference.html#format) 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 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<FormatAttr>()) { + // 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<CXXMethodDecl>(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 `````````` </details> https://github.com/llvm/llvm-project/pull/132765 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
