[analyzer] PR41185: Fix regression where __builtin_* functions weren't recognized

[Szelethus]

Summary

For the following code snippet:

void builtin_function_call_crash_fixes(char *c) {
  __builtin_strncpy(c, "", 6);
  __builtin_memset(c, '\0', (0));
  __builtin_memcpy(c, c, 0);
}

security.insecureAPI.DeprecatedOrUnsafeBufferHandling caused a regression, as it didn't recognize functions starting with __builtin_. Fixed exactly that.

I wanted to modify an existing test file, but the two I found didn't seem like perfect candidates. While I was there, I prettified their RUN: lines.

[Szelethus]

Please add a testcase on which the checker crashed, but this patch resolves it.

Also, I think (in this particular case) that removing an assert is not a good idea. It is there for a reason, without it, we wouldn't have discovered this issue either. I think, as I mentioned in the inline, we should just strip `"__builtin_".

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

138–140

Shouldn't we just do this instead?

155–162

This guarantees that if we strip "__builtin_" within WalkAST::checkDeprecatedOrUnsafeBufferHandling, we won't run into an assertation failure.

[alexfh]

Thanks for addressing this! Please add the test cases from https://bugs.llvm.org/show_bug.cgi?id=41185

[alexfh]

Artem, I'd appreciate, if you found time to finish this fix. The bug is causing most failures in our setup. Thanks!

[Szelethus]

Consider it done! In a little while.

[Szelethus]

Implemented the fix I proposed in my earlier comment. While I was there, I prettified some of the other tests, and also made sure that the core package is enabled in their run lines.

[Szelethus]

Uhh, sorry for the spam. Realized that these are not path sensitive checkers. Removed the core packages from the invocations. Actual logic of the patch is untouched.

[alexfh]

Awesome, thanks!
LG

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

void WalkAST::VisitCallExpr(CallExpr *CE) {
 
   if (!FD)
     return;
 
   // Get the name of the callee. If it's a builtin, strip off the prefix.
   IdentifierInfo *II = FD->getIdentifier();
   if (!II)   // if no identifier, not a simple C function
     return;
   StringRef Name = II->getName();
   if (Name.startswith("__builtin_"))
     Name = Name.substr(10);
 
   // Set the evaluation function by switching on the callee name.
   FnCheck evalFunction = llvm::StringSwitch<FnCheck>(Name)
     .Case("bcmp", &WalkAST::checkCall_bcmp)
     .Case("bcopy", &WalkAST::checkCall_bcopy)
     .Case("bzero", &WalkAST::checkCall_bzero)
     .Case("gets", &WalkAST::checkCall_gets)
     .Case("getpw", &WalkAST::checkCall_getpw)
     .Case("mktemp", &WalkAST::checkCall_mktemp)
     .Case("mkstemp", &WalkAST::checkCall_mkstemp)
     .Case("mkdtemp", &WalkAST::checkCall_mkstemp)
     .Case("mkstemps", &WalkAST::checkCall_mkstemp)
     .Cases("strcpy", "__strcpy_chk", &WalkAST::checkCall_strcpy)
     .Cases("strcat", "__strcat_chk", &WalkAST::checkCall_strcat)
     .Cases("sprintf", "vsprintf", "scanf", "wscanf", "fscanf", "fwscanf",
            "vscanf", "vwscanf", "vfscanf", "vfwscanf",
            &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
     .Cases("sscanf", "swscanf", "vsscanf", "vswscanf", "swprintf",
            "snprintf", "vswprintf", "vsnprintf", "memcpy", "memmove",
            &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
     .Cases("strncpy", "strncat", "memset",
            &WalkAST::checkDeprecatedOrUnsafeBufferHandling)
     .Case("drand48", &WalkAST::checkCall_rand)
     .Case("erand48", &WalkAST::checkCall_rand)
     .Case("jrand48", &WalkAST::checkCall_rand)
     .Case("lrand48", &WalkAST::checkCall_rand)
     .Case("mrand48", &WalkAST::checkCall_rand)
     .Case("nrand48", &WalkAST::checkCall_rand)
     .Case("lcong48", &WalkAST::checkCall_rand)
     .Case("rand", &WalkAST::checkCall_rand)
void WalkAST::checkDeprecatedOrUnsafeBufferHandling(const CallExpr *CE,
                                                     const FunctionDecl *FD) {
   if (!filter.check_DeprecatedOrUnsafeBufferHandling)
     return;
 
   if (!BR.getContext().getLangOpts().C11)
     return;
 
   // Issue a warning. ArgIndex == -1: Deprecated but not unsafe (has size
-  // restrictions).
+  // restrictions), ArgIndex == -2: built-in functions, etc.
   enum { DEPR_ONLY = -1, UNKNOWN_CALL = -2 };
   StringRef Name = FD->getIdentifier()->getName();
   int ArgIndex =
       llvm::StringSwitch<int>(Name)
           .Cases("scanf", "wscanf", "vscanf", "vwscanf", 0)
           .Cases("sprintf", "vsprintf", "fscanf", "fwscanf", "vfscanf",
                  "vfwscanf", "sscanf", "swscanf", "vsscanf", "vswscanf", 1)
           .Cases("swprintf", "snprintf", "vswprintf", "vsnprintf", "memcpy",
                  "memmove", "memset", "strncpy", "strncat", DEPR_ONLY)
           .Default(UNKNOWN_CALL);
 
-  assert(ArgIndex != UNKNOWN_CALL && "Unsupported function");
-  bool BoundsProvided = ArgIndex == DEPR_ONLY;
+  bool BoundsProvided = ArgIndex < 0;
 
   if (!BoundsProvided) {
     // Currently we only handle (not wide) string literals. It is possible to do
     // better, either by looking at references to const variables, or by doing
     // real flow analysis.
     auto FormatString =
         dyn_cast<StringLiteral>(CE->getArg(ArgIndex)->IgnoreParenImpCasts());
     if (FormatString &&
Out2 << "Call to function '" << Name
        << "' is insecure as it does not provide ";
 
   if (!BoundsProvided) {
     Out2 << "bounding of the memory buffer or ";
   }
 
   Out2 << "security checks introduced "
           "in the C11 standard. Replace with analogous functions that "
-          "support length arguments or provides boundary checks such as '"
-       << Name << "_s' in case of C11";
+          "support length arguments or provides boundary checks";
+
+  // We know the function has a secure version introduced in C11.
+  if(ArgIndex != UNKNOWN_CALL) {
+    Out2 << " such as '" << Name << "_s' in case of C11";
+  }
 
   PathDiagnosticLocation CELoc =
       PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   BR.EmitBasicReport(AC->getDecl(),
                      filter.checkName_DeprecatedOrUnsafeBufferHandling,
                      Out1.str(), "Security", Out2.str(), CELoc,
                      CE->getCallee()->getSourceRange());
 }