This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG744745ae195f: [analyzer] Add failing test case demonstrating
buggy taint propagation (authored by steakhal).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118987/new/
https://reviews.llvm.org/D118987
Files:
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
clang/test/Analysis/taint-checker-callback-order-has-definition.c
clang/test/Analysis/taint-checker-callback-order-without-definition.c
Index: clang/test/Analysis/taint-checker-callback-order-without-definition.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/taint-checker-callback-order-without-definition.c
@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.security.taint \
+// RUN: -mllvm -debug-only=taint-checker \
+// RUN: 2>&1 | FileCheck %s
+
+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
+}
Index: clang/test/Analysis/taint-checker-callback-order-has-definition.c
===================================================================
--- /dev/null
+++ clang/test/Analysis/taint-checker-callback-order-has-definition.c
@@ -0,0 +1,42 @@
+// RUN: %clang_analyze_cc1 %s \
+// RUN: -analyzer-checker=core,alpha.security.taint \
+// RUN: -mllvm -debug-only=taint-checker \
+// RUN: 2>&1 | FileCheck %s
+
+// 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
+}
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ 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 @@
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 @@
/// 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);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
