[clang] fa0a80e - Revert "Revert "[analyzer] Add failing test case demonstrating buggy taint propagation""
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 23 01:48:30 PST 2022
Author: Balazs Benics
Date: 2022-02-23T10:48:06+01:00
New Revision: fa0a80e017ebd58a71bdb4e4493bb022f80fe791
URL: https://github.com/llvm/llvm-project/commit/fa0a80e017ebd58a71bdb4e4493bb022f80fe791
DIFF: https://github.com/llvm/llvm-project/commit/fa0a80e017ebd58a71bdb4e4493bb022f80fe791.diff
LOG: Revert "Revert "[analyzer] Add failing test case demonstrating buggy taint propagation""
This reverts commit b8ae323cca61dc1edcd36e9ae18c7e4c3d76d52e.
Let's try `REQUIRES: asserts`.
Added:
clang/test/Analysis/taint-checker-callback-order-has-definition.c
clang/test/Analysis/taint-checker-callback-order-without-definition.c
Modified:
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index e2209e3debfde..428778e6cfaa6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -32,6 +32,8 @@
#include <memory>
#include <utility>
+#define DEBUG_TYPE "taint-checker"
+
using namespace clang;
using namespace ento;
using namespace taint;
@@ -691,6 +693,13 @@ void GenericTaintChecker::checkPostCall(const CallEvent &Call,
if (TaintArgs.isEmpty())
return;
+ LLVM_DEBUG(for (ArgIdxTy I
+ : TaintArgs) {
+ llvm::dbgs() << "PostCall<";
+ Call.dump(llvm::dbgs());
+ llvm::dbgs() << "> actually wants to taint arg index: " << I << '\n';
+ });
+
for (ArgIdxTy ArgNum : TaintArgs) {
// Special handling for the tainted return value.
if (ArgNum == ReturnValueIndex) {
@@ -768,15 +777,25 @@ void GenericTaintRule::process(const GenericTaintChecker &Checker,
/// Propagate taint where it is necessary.
ForEachCallArg(
- [this, &State, WouldEscape](ArgIdxTy I, const Expr *E, SVal V) {
- if (PropDstArgs.contains(I))
+ [this, &State, WouldEscape, &Call](ArgIdxTy I, const Expr *E, SVal V) {
+ if (PropDstArgs.contains(I)) {
+ LLVM_DEBUG(llvm::dbgs() << "PreCall<"; Call.dump(llvm::dbgs());
+ llvm::dbgs()
+ << "> prepares tainting arg index: " << I << '\n';);
State = State->add<TaintArgsOnPostVisit>(I);
+ }
// TODO: We should traverse all reachable memory regions via the
// escaping parameter. Instead of doing that we simply mark only the
// referred memory region as tainted.
- if (WouldEscape(V, E->getType()))
+ if (WouldEscape(V, E->getType())) {
+ LLVM_DEBUG(if (!State->contains<TaintArgsOnPostVisit>(I)) {
+ llvm::dbgs() << "PreCall<";
+ Call.dump(llvm::dbgs());
+ llvm::dbgs() << "> prepares tainting arg index: " << I << '\n';
+ });
State = State->add<TaintArgsOnPostVisit>(I);
+ }
});
C.addTransition(State);
diff --git a/clang/test/Analysis/taint-checker-callback-order-has-definition.c b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
new file mode 100644
index 0000000000000..82943ad46fbd8
--- /dev/null
+++ b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.security.taint \
+// RUN: -mllvm -debug-only=taint-checker \
+// RUN: 2>&1 | FileCheck %s
+
+// REQUIRES: asserts
+// FIXME: We should not crash.
+// XFAIL: *
+
+struct _IO_FILE;
+typedef struct _IO_FILE FILE;
+FILE *fopen(const char *fname, const char *mode);
+
+void nested_call(void) {}
+
+char *fgets(char *s, int n, FILE *fp) {
+ nested_call(); // no-crash: we should not try adding taint to a non-existent argument.
+ return (char *)0;
+}
+
+void top(const char *fname, char *buf) {
+ FILE *fp = fopen(fname, "r");
+ // CHECK: PreCall<fopen(fname, "r")> prepares tainting arg index: -1
+ // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
+
+ if (!fp)
+ return;
+
+ (void)fgets(buf, 42, fp); // Trigger taint propagation.
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
+
+ // FIXME: We should propagate taint from PreCall<fgets> -> PostCall<fgets>.
+ // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: -1
+ // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 0
+ // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 1
+ // CHECK-NEXT: PostCall<nested_call()> actually wants to taint arg index: 2
+
+ // FIXME: We should not crash.
+ // CHECK: PLEASE submit a bug report
+}
diff --git a/clang/test/Analysis/taint-checker-callback-order-without-definition.c b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
new file mode 100644
index 0000000000000..dba23f367fd66
--- /dev/null
+++ b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.security.taint \
+// RUN: -mllvm -debug-only=taint-checker \
+// RUN: 2>&1 | FileCheck %s
+
+// REQUIRES: asserts
+
+struct _IO_FILE;
+typedef struct _IO_FILE FILE;
+FILE *fopen(const char *fname, const char *mode);
+
+char *fgets(char *s, int n, FILE *fp); // no-definition
+
+void top(const char *fname, char *buf) {
+ FILE *fp = fopen(fname, "r"); // Introduce taint.
+ // CHECK: PreCall<fopen(fname, "r")> prepares tainting arg index: -1
+ // CHECK-NEXT: PostCall<fopen(fname, "r")> actually wants to taint arg index: -1
+
+ if (!fp)
+ return;
+
+ (void)fgets(buf, 42, fp); // Trigger taint propagation.
+
+ // FIXME: Why is the arg index 1 prepared for taint?
+ // Before the call it wasn't tainted, and it also shouldn't be tainted after the call.
+
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: -1
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 0
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 1
+ // CHECK-NEXT: PreCall<fgets(buf, 42, fp)> prepares tainting arg index: 2
+ //
+ // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: -1
+ // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 0
+ // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 1
+ // CHECK-NEXT: PostCall<fgets(buf, 42, fp)> actually wants to taint arg index: 2
+}
More information about the cfe-commits
mailing list