[llvm-branch-commits] [clang] Backport taint analysis slowdown regression fix (PR #105516)
Tobias Hieta via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sun Sep 1 01:00:56 PDT 2024
https://github.com/tru updated https://github.com/llvm/llvm-project/pull/105516
>From f565cbd194c54a3fec64236330ade3760f473965 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Wed, 21 Aug 2024 14:24:56 +0200
Subject: [PATCH 1/2] [analyzer] Limit `isTainted()` by skipping complicated
symbols (#105493)
As discussed in
https://discourse.llvm.org/t/rfc-make-istainted-and-complex-symbols-friends/79570/10
Some `isTainted()` queries can blow up the analysis times, and
effectively halt the analysis under specific workloads.
We don't really have the time now to do a caching re-implementation of
`isTainted()`, so we need to workaround the case.
The workaround with the smallest blast radius was to limit what symbols
`isTainted()` does the query (by walking the SymExpr). So far, the
threshold 10 worked for us, but this value can be overridden using the
"max-tainted-symbol-complexity" config value.
This new option is "deprecated" from the getgo, as I expect this issue
to be fixed within the next few months and I don't want users to
override this value anyways. If they do, this message will let them know
that they are on their own, and the next release may break them (as we
no longer recognize this option if we drop it).
Mitigates #89720
CPP-5414
(cherry picked from commit 848658955a9d2d42ea3e319d191e2dcd5d76c837)
---
.../StaticAnalyzer/Core/AnalyzerOptions.def | 5 ++
clang/lib/StaticAnalyzer/Checkers/Taint.cpp | 7 +++
clang/test/Analysis/analyzer-config.c | 1 +
clang/test/Analysis/taint-generic.c | 49 ++++++++++++++++++-
4 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index 29aa6a3b8a16e7..737bc8e86cfb6a 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -407,6 +407,11 @@ ANALYZER_OPTION(
ANALYZER_OPTION(unsigned, MaxSymbolComplexity, "max-symbol-complexity",
"The maximum complexity of symbolic constraint.", 35)
+// HACK:https://discourse.llvm.org/t/rfc-make-istainted-and-complex-symbols-friends/79570
+// Ideally, we should get rid of this option soon.
+ANALYZER_OPTION(unsigned, MaxTaintedSymbolComplexity, "max-tainted-symbol-complexity",
+ "[DEPRECATED] The maximum complexity of a symbol to carry taint", 9)
+
ANALYZER_OPTION(unsigned, MaxTimesInlineLarge, "max-times-inline-large",
"The maximum times a large function could be inlined.", 32)
diff --git a/clang/lib/StaticAnalyzer/Checkers/Taint.cpp b/clang/lib/StaticAnalyzer/Checkers/Taint.cpp
index 6362c82b009d72..0bb5739db4b756 100644
--- a/clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -12,6 +12,7 @@
#include "clang/StaticAnalyzer/Checkers/Taint.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include <optional>
@@ -256,6 +257,12 @@ std::vector<SymbolRef> taint::getTaintedSymbolsImpl(ProgramStateRef State,
if (!Sym)
return TaintedSymbols;
+ // HACK:https://discourse.llvm.org/t/rfc-make-istainted-and-complex-symbols-friends/79570
+ if (const auto &Opts = State->getAnalysisManager().getAnalyzerOptions();
+ Sym->computeComplexity() > Opts.MaxTaintedSymbolComplexity) {
+ return {};
+ }
+
// Traverse all the symbols this symbol depends on to see if any are tainted.
for (SymbolRef SubSym : Sym->symbols()) {
if (!isa<SymbolData>(SubSym))
diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index 2a4c40005a4dc0..1ee0d8e4eecebd 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -96,6 +96,7 @@
// CHECK-NEXT: max-inlinable-size = 100
// CHECK-NEXT: max-nodes = 225000
// CHECK-NEXT: max-symbol-complexity = 35
+// CHECK-NEXT: max-tainted-symbol-complexity = 9
// CHECK-NEXT: max-times-inline-large = 32
// CHECK-NEXT: min-cfg-size-treat-functions-as-large = 14
// CHECK-NEXT: mode = deep
diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index b0df85f237298d..1c139312734bca 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -63,6 +63,7 @@ void clang_analyzer_isTainted_char(char);
void clang_analyzer_isTainted_wchar(wchar_t);
void clang_analyzer_isTainted_charp(char*);
void clang_analyzer_isTainted_int(int);
+void clang_analyzer_dump_int(int);
int coin();
@@ -459,7 +460,53 @@ unsigned radar11369570_hanging(const unsigned char *arr, int l) {
longcmp(a, t, c);
l -= 12;
}
- return 5/a; // expected-warning {{Division by a tainted value, possibly zero}}
+ return 5/a; // FIXME: Should be a "div by tainted" warning here.
+}
+
+// This computation used to take a very long time.
+void complex_taint_queries(const int *p) {
+ int tainted = 0;
+ scanf("%d", &tainted);
+
+ // Make "tmp" tainted.
+ int tmp = tainted + tainted;
+ clang_analyzer_isTainted_int(tmp); // expected-warning{{YES}}
+
+ // Make "tmp" SymExpr a lot more complicated by applying computation.
+ // This should balloon the symbol complexity.
+ tmp += p[0] + p[0];
+ tmp += p[1] + p[1];
+ tmp += p[2] + p[2];
+ clang_analyzer_dump_int(tmp); // expected-warning{{((((conj_}} symbol complexity: 8
+ clang_analyzer_isTainted_int(tmp); // expected-warning{{YES}}
+
+ tmp += p[3] + p[3];
+ clang_analyzer_dump_int(tmp); // expected-warning{{(((((conj_}} symbol complexity: 10
+ clang_analyzer_isTainted_int(tmp); // expected-warning{{NO}} 10 is already too complex to be traversed
+
+ tmp += p[4] + p[4];
+ tmp += p[5] + p[5];
+ tmp += p[6] + p[6];
+ tmp += p[7] + p[7];
+ tmp += p[8] + p[8];
+ tmp += p[9] + p[9];
+ tmp += p[10] + p[10];
+ tmp += p[11] + p[11];
+ tmp += p[12] + p[12];
+ tmp += p[13] + p[13];
+ tmp += p[14] + p[14];
+ tmp += p[15] + p[15];
+
+ // The SymExpr still holds the full history of the computation, yet, "isTainted" doesn't traverse the tree as the complexity is over the threshold.
+ clang_analyzer_dump_int(tmp);
+ // expected-warning at -1{{(((((((((((((((((conj_}} symbol complexity: 34
+ clang_analyzer_isTainted_int(tmp); // expected-warning{{NO}} FIXME: Ideally, this should still result in "tainted".
+
+ // By making it even one step more complex, then it would hit the "max-symbol-complexity"
+ // threshold and the engine would cut the SymExpr and replace it by a new conjured symbol.
+ tmp += p[16];
+ clang_analyzer_dump_int(tmp); // expected-warning{{conj_}} symbol complexity: 1
+ clang_analyzer_isTainted_int(tmp); // expected-warning{{NO}}
}
// Check that we do not assert of the following code.
>From a475b2bb4d4bd762189e5ce7b25adfe2688484bf Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Wed, 21 Aug 2024 14:48:57 +0200
Subject: [PATCH 2/2] Mention the fix in the release notes
---
clang/docs/ReleaseNotes.rst | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b68b823ae6761d..ef5a2fda1ea771 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1407,6 +1407,11 @@ Crash and bug fixes
- Z3 crosschecking (aka. Z3 refutation) is now bounded, and can't consume
more total time than the eymbolic execution itself. (#GH97298)
+- In clang-18, we regressed in terms of analysis time for projects having many
+ nested loops with buffer indexing or shifting or other binary operations.
+ For example, functions computing different hash values. Some of this slowdown
+ was attributed to taint analysis, which is fixed now. (#GH105493)
+
- ``std::addressof``, ``std::as_const``, ``std::forward``,
``std::forward_like``, ``std::move``, ``std::move_if_noexcept``, are now
modeled just like their builtin counterpart. (#GH94193)
More information about the llvm-branch-commits
mailing list