From 94f604c8d82d40dac4a329e1b201edd80ccce3b1 Mon Sep 17 00:00:00 2001 From: Balazs Benics Date: Mon, 18 Sep 2023 08:25:28 +0200 Subject: [PATCH] fixup! [analyzer] TaintPropagation checker strlen() should not propagate Leave comments for the future for why these dont propagate taint --- clang/docs/ReleaseNotes.rst | 7 +++++++ .../Checkers/GenericTaintChecker.cpp | 4 ++++ clang/test/Analysis/taint-generic.c | 16 +++++++++++++--- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 3cdad2f7b9f0..414cd7f62e2d 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -409,6 +409,13 @@ Static Analyzer bitwise shift operators produce undefined behavior (because some operand is negative or too large). +- The ``alpha.security.taint.TaintPropagation`` checker no longer propagates + taint on ``strlen`` and ``strnlen`` calls, unless these are marked + explicitly propagators in the user-provided taint configuration file. + This removal empirically reduces the number of false positive reports. + Read the PR for the details. + (`#66086 `_) + .. _release-notes-sanitizers: Sanitizers diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 95a759c251ca..dae8ff0c5c8f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -695,6 +695,10 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const { {{{"strpbrk"}}, TR::Prop({{0}}, {{ReturnValueIndex}})}, {{{"strndup"}}, TR::Prop({{0}}, {{ReturnValueIndex}})}, {{{"strndupa"}}, TR::Prop({{0}}, {{ReturnValueIndex}})}, + + // strlen, wcslen, strnlen and alike intentionally don't propagate taint. + // See the details here: https://github.com/llvm/llvm-project/pull/66086 + {{{"strtol"}}, TR::Prop({{0}}, {{1, ReturnValueIndex}})}, {{{"strtoll"}}, TR::Prop({{0}}, {{1, ReturnValueIndex}})}, {{{"strtoul"}}, TR::Prop({{0}}, {{1, ReturnValueIndex}})}, diff --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c index 359a7a264c8c..10a52cb44259 100644 --- a/clang/test/Analysis/taint-generic.c +++ b/clang/test/Analysis/taint-generic.c @@ -443,7 +443,11 @@ int testSprintf_propagates_taint(char *buf, char *msg) { return 1 / x; // expected-warning {{Division by a tainted value, possibly zero}} } -void test_wchar_apis_propagate(const char *path) { +void test_wchar_apis_dont_propagate(const char *path) { + // strlen, wcslen, strnlen and alike intentionally don't propagate taint. + // See the details here: https://github.com/llvm/llvm-project/pull/66086 + // This isn't ideal, but this is only what we have now. + FILE *f = fopen(path, "r"); clang_analyzer_isTainted_charp((char*)f); // expected-warning {{YES}} wchar_t wbuf[10]; @@ -948,7 +952,10 @@ void testStrndupa(size_t n) { } size_t strlen(const char *s); -void testStrlen() { +void testStrlen_dont_propagate() { + // strlen, wcslen, strnlen and alike intentionally don't propagate taint. + // See the details here: https://github.com/llvm/llvm-project/pull/66086 + // This isn't ideal, but this is only what we have now. char s[10]; scanf("%9s", s); @@ -958,7 +965,10 @@ void testStrlen() { } size_t strnlen(const char *s, size_t maxlen); -void testStrnlen(size_t maxlen) { +void testStrnlen_dont_propagate(size_t maxlen) { + // strlen, wcslen, strnlen and alike intentionally don't propagate taint. + // See the details here: https://github.com/llvm/llvm-project/pull/66086 + // This isn't ideal, but this is only what we have now. char s[10]; scanf("%9s", s); size_t result = strnlen(s, maxlen); -- 2.42.0