[clang] 2acead3 - Revert "[analyzer] Fix taint rule of fgets and setproctitle_init"

[Balazs Benics via cfe-commits]

Author: Balazs Benics
Date: 2022-02-14T18:45:46+01:00
New Revision: 2acead35c1289d2b3593a992b0639ca6427e481f

URL: https://github.com/llvm/llvm-project/commit/2acead35c1289d2b3593a992b0639ca6427e481f
DIFF: https://github.com/llvm/llvm-project/commit/2acead35c1289d2b3593a992b0639ca6427e481f.diff

LOG: Revert "[analyzer] Fix taint rule of fgets and setproctitle_init"

This reverts commit bf5963bf19670ea58facdf57492e147c13bb650f.

I'm reverting this since the head of the patch stack caused a build
breakage.

https://lab.llvm.org/buildbot/#/builders/91/builds/3818

Added: 
    

Modified: 
    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
    clang/test/Analysis/taint-generic.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
index d15a4659a96e..66143f78932c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -559,7 +559,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
       {{"atoll"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"fgetc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
       {{"fgetln"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
-      {{"fgets"}, TR::Prop({{2}}, {{0, ReturnValueIndex}})},
+      {{"fgets"}, TR::Prop({{2}}, {{0}, ReturnValueIndex})},
       {{"fscanf"}, TR::Prop({{0}}, {{}, 2})},
       {{"sscanf"}, TR::Prop({{0}}, {{}, 2})},
       {{"getc"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
@@ -632,7 +632,7 @@ void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
   if (TR::UntrustedEnv(C)) {
     // void setproctitle_init(int argc, char *argv[], char *envp[])
     GlobalCRules.push_back(
-        {{{"setproctitle_init"}}, TR::Sink({{1, 2}}, MsgCustomSink)});
+        {{{"setproctitle_init"}}, TR::Sink({{2}}, MsgCustomSink)});
     GlobalCRules.push_back({{"getenv"}, TR::Source({{ReturnValueIndex}})});
   }
 

diff  --git a/clang/test/Analysis/taint-checker-callback-order-has-definition.c b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
index cdf55e3aef41..a070077004fa 100644
--- a/clang/test/Analysis/taint-checker-callback-order-has-definition.c
+++ b/clang/test/Analysis/taint-checker-callback-order-has-definition.c
@@ -25,9 +25,11 @@ void top(const char *fname, char *buf) {
   (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
-  //
+
   // 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
 }

diff  --git a/clang/test/Analysis/taint-checker-callback-order-without-definition.c b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
index f2e070739e97..962e8b259298 100644
--- a/clang/test/Analysis/taint-checker-callback-order-without-definition.c
+++ b/clang/test/Analysis/taint-checker-callback-order-without-definition.c
@@ -19,11 +19,16 @@ void top(const char *fname, char *buf) {
 
   (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
 }

diff  --git a/clang/test/Analysis/taint-generic.c b/clang/test/Analysis/taint-generic.c
index 0612e1b9f98b..6979c0667764 100644
--- a/clang/test/Analysis/taint-generic.c
+++ b/clang/test/Analysis/taint-generic.c
@@ -58,11 +58,9 @@ extern FILE *stdin;
 
 #define bool _Bool
 
-char *getenv(const char *name);
 int fscanf(FILE *restrict stream, const char *restrict format, ...);
 int sprintf(char *str, const char *format, ...);
 void setproctitle(const char *fmt, ...);
-void setproctitle_init(int argc, char *argv[], char *envp[]);
 typedef __typeof(sizeof(int)) size_t;
 
 // Define string functions. Use builtin for some of them. They all default to
@@ -406,20 +404,3 @@ void testConfigurationSinks(void) {
 void testUnknownFunction(void (*foo)(void)) {
   foo(); // no-crash
 }
-
-void testProctitleFalseNegative() {
-  char flag[80];
-  fscanf(stdin, "%79s", flag);
-  char *argv[] = {"myapp", flag};
-  // FIXME: We should have a warning below: Untrusted data passed to sink.
-  setproctitle_init(1, argv, 0);
-}
-
-void testProctitle2(char *real_argv[]) {
-  char *app = getenv("APP_NAME");
-  if (!app)
-    return;
-  char *argv[] = {app, "--foobar"};
-  setproctitle_init(1, argv, 0);         // expected-warning {{Untrusted data is passed to a user-defined sink}}
-  setproctitle_init(1, real_argv, argv); // expected-warning {{Untrusted data is passed to a user-defined sink}}
-}