[analyzer] Fix sensitive argument logic in GenericTaintChecker

[gamesh411]

Summary

The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.

[steakhal]

E.g. execl() and execlp functions are actually variadic. You should also account for them.
I would rather map directly to a full-fledged Propagation rule instead of to a sensitive arg index list. That way you could express this property.
Demonstrate this in a test.

Actually, the CallDescriptionFlags::CDF_MaybeBuiltin CallDescriptions will use the CCtx::isCLibraryFunction() for matching the call.
By using that the code would get significantly shorter and more readable as well.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

951–957

These algorithms are not supposed to have side-effects. Even though we technically could have side-effects, I would still apply it separately.
That being said, this seems to be a copy-paste version of the previous hunk, like 3rd time.

[martong]

The semantics of taint sinks is that if ANY of the arguments is tainted, a
warning should be emmitted. Before this change, if there were multiple
arguments that are sensitive, and if the first arg is not tainted, but any of
the noninitial are tainted, a warning is not emitted. After this change the
correct semantics is reflected in code.

It worked well with custom sinks, didn't it? So, as I see, this patch is about system calls and subscript indices. Could you please update the description to have this information?

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

951–957

That being said, this seems to be a copy-paste version of the previous hunk, like 3rd time.

+1, to put it into a named function

[gamesh411]

This is superseded by D116025.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

return generateReportIfTainted(Call.getArgExpr(ArgNum),
                                  MsgUncontrolledFormatString, C);
 }
 
 bool GenericTaintChecker::checkSystemCall(const CallEvent &Call, StringRef Name,
                                           CheckerContext &C) const {
   // TODO: It might make sense to run this check on demand. In some cases,
   // we should check if the environment has been cleansed here. We also might
   // need to know if the user was reset before these calls(seteuid).
-  unsigned ArgNum = llvm::StringSwitch<unsigned>(Name)
-                        .Case("system", 0)
-                        .Case("popen", 0)
-                        .Case("execl", 0)
-                        .Case("execle", 0)
-                        .Case("execlp", 0)
-                        .Case("execv", 0)
-                        .Case("execvp", 0)
-                        .Case("execvP", 0)
-                        .Case("execve", 0)
-                        .Case("dlopen", 0)
-                        .Default(InvalidArgIndex);
-
-  if (ArgNum == InvalidArgIndex || Call.getNumArgs() < (ArgNum + 1))
-    return false;
+  const ArgVector SensitiveArgs = llvm::StringSwitch<ArgVector>(Name)
+                                      .Case("system", {0})
+                                      .Case("popen", {0})
+                                      .Case("execl", {0, 1})
+                                      .Case("execle", {0, 1})
+                                      .Case("execlp", {0, 1})
+                                      .Case("execv", {0, 1})
+                                      .Case("execvp", {0, 1})
+                                      .Case("execvP", {0})
+                                      .Case("execve", {0, 1, 2})
+                                      .Case("dlopen", {0})
+                                      .Default({});
 
-  return generateReportIfTainted(Call.getArgExpr(ArgNum), MsgSanitizeSystemArgs,
-                                 C);
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+           return Idx < Call.getNumArgs() &&
+                  generateReportIfTainted(Call.getArgExpr(Idx),
+                                          MsgSanitizeSystemArgs, C);
+         }) != SensitiveArgs.end();
 }
 
 // TODO: Should this check be a part of the CString checker?
 // If yes, should taint be a global setting?
 bool GenericTaintChecker::checkTaintedBufferSize(const CallEvent &Call,
                                                  CheckerContext &C) const {
   const auto *FDecl = Call.getDecl()->getAsFunction();
   // If the function has a buffer size argument, set ArgNum.
-  unsigned ArgNum = InvalidArgIndex;
+  ArgVector SensitiveArgs = {};
   unsigned BId = 0;
   if ((BId = FDecl->getMemoryFunctionKind())) {
     switch (BId) {
     case Builtin::BImemcpy:
     case Builtin::BImemmove:
     case Builtin::BIstrncpy:
-      ArgNum = 2;
+      SensitiveArgs = {2};
       break;
     case Builtin::BIstrndup:
-      ArgNum = 1;
+      SensitiveArgs = {1};
       break;
     default:
       break;
     }
   }
 
-  if (ArgNum == InvalidArgIndex) {
+  if (SensitiveArgs.empty()) {
     using CCtx = CheckerContext;
     if (CCtx::isCLibraryFunction(FDecl, "malloc") ||
-        CCtx::isCLibraryFunction(FDecl, "calloc") ||
         CCtx::isCLibraryFunction(FDecl, "alloca"))
-      ArgNum = 0;
+      SensitiveArgs = {0};
+    else if (CCtx::isCLibraryFunction(FDecl, "calloc"))
+      SensitiveArgs = {0, 1};
     else if (CCtx::isCLibraryFunction(FDecl, "memccpy"))
-      ArgNum = 3;
+      SensitiveArgs = {3};
     else if (CCtx::isCLibraryFunction(FDecl, "realloc"))
-      ArgNum = 1;
+      SensitiveArgs = {1};
     else if (CCtx::isCLibraryFunction(FDecl, "bcopy"))
-      ArgNum = 2;
+      SensitiveArgs = {2};
   }
 
-  return ArgNum != InvalidArgIndex && Call.getNumArgs() > ArgNum &&
-         generateReportIfTainted(Call.getArgExpr(ArgNum), MsgTaintedBufferSize,
-                                 C);
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+           return Idx < Call.getNumArgs() &&
+                  generateReportIfTainted(Call.getArgExpr(Idx),
+                                          MsgTaintedBufferSize, C);
+         }) != SensitiveArgs.end();
 }
 
 bool GenericTaintChecker::checkCustomSinks(const CallEvent &Call,
                                            const FunctionData &FData,
                                            CheckerContext &C) const {
   auto It = findFunctionInConfig(CustomSinks, FData);
   if (It == CustomSinks.end())
     return false;
 
   const auto &Value = It->second;
-  const GenericTaintChecker::ArgVector &Args = Value.second;
-  for (unsigned ArgNum : Args) {
-    if (ArgNum >= Call.getNumArgs())
-      continue;
+  const ArgVector &SensitiveArgs = Value.second;
 
-    if (generateReportIfTainted(Call.getArgExpr(ArgNum), MsgCustomSink, C))
-      return true;
-  }
-
-  return false;
+  // Find the first argument that receives a tainted value.
+  // The report is emitted as a side-effect.
+  return llvm::find_if(SensitiveArgs, [this, &C, &Call](unsigned Idx) {
+           return Idx < Call.getNumArgs() &&
+                  generateReportIfTainted(Call.getArgExpr(Idx), MsgCustomSink,
+                                          C);
+         }) != SensitiveArgs.end();
 }
 
 void ento::registerGenericTaintChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker<GenericTaintChecker>();
   std::string Option{"Config"};
   StringRef ConfigFile =
       Mgr.getAnalyzerOptions().getCheckerStringOption(Checker, Option);
   llvm::Optional<TaintConfig> Config =
       getConfiguration<TaintConfig>(Mgr, Checker, Option, ConfigFile);
   if (Config)
     Checker->parseConfiguration(Mgr, Option, std::move(Config.getValue()));
 }
 
 bool ento::shouldRegisterGenericTaintChecker(const CheckerManager &mgr) {
   return true;
 }

clang/test/Analysis/taint-generic.c

void testTaintedBufferSize() {
   bcopy(buf1, dst, ts); // expected-warning {{Untrusted data is used to specify the buffer size}}
   __builtin_memcpy(dst, buf1, (ts + 4)*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}
 
   // If both buffers are trusted, do not issue a warning.
   char *dst2 = (char*)malloc(ts*sizeof(char)); // expected-warning {{Untrusted data is used to specify the buffer size}}
   strncat(dst2, dst, ts); // no-warning
 }
 
+void testTaintedBufferSizeNoninitialTaintedArg() {
+  size_t ts;
+  scanf("%zd", &ts);
+
+  const int nontainted_num = 1;
+
+  (void)calloc(nontainted_num, ts); //expected-warning {{Untrusted data is used to specify the buffer size}}
+}
+
 #define AF_UNIX   1   /* local to host (pipes) */
 #define AF_INET   2   /* internetwork: UDP, TCP, etc. */
 #define AF_LOCAL  AF_UNIX   /* backward compatibility */
 #define SOCK_STREAM 1
 int socket(int, int, int);
 size_t read(int, void *, size_t);
 int  execl(const char *, const char *, ...);