[analyzer] Fix format attribute handling in GenericTaintChecker #132765
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently `optin.taint.GenericTaint` can produce false positives if a [format attribute](https://clang.llvm.org/docs/AttributeReference.html#format) is applied on a non-static method. This commit adds a testcase that highlights this buggy behavior.
Previously the `optin.taint.GenericTaint` checker misinterpreted the parameter indices in situations when the format attribute was applied to a (non-static) method. This commit fixes this bug.
NagyDonat
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesPreviously Full diff: https://github.com/llvm/llvm-project/pull/132765.diff 2 Files Affected:
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
|
steakhal
approved these changes
Mar 26, 2025
NagyDonat
commented
Mar 26, 2025
steakhal
approved these changes
Mar 26, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/41/builds/5807 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously
optin.taint.GenericTaintmisinterpreted 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