[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 ↗

(On Diff #192255)

Shouldn't we just do this instead?

155–162 ↗

(On Diff #192255)

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

cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

if (!filter.check_DeprecatedOrUnsafeBufferHandling)
     return;
 
   if (!BR.getContext().getLangOpts().C11)
     return;
 
   // Issue a warning. ArgIndex == -1: Deprecated but not unsafe (has size
   // restrictions).
   enum { DEPR_ONLY = -1, UNKNOWN_CALL = -2 };
+
   StringRef Name = FD->getIdentifier()->getName();
+  if (Name.startswith("__builtin_"))
+    Name = Name.substr(10);
+
   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);

cfe/trunk/test/Analysis/security-syntax-checks-no-emit.c

-// RUN: %clang_analyze_cc1 -triple i686-pc-linux-gnu -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
+// RUN: %clang_analyze_cc1 -triple i686-pc-linux-gnu %s -verify \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
 // expected-no-diagnostics
 
 // This file complements 'security-syntax-checks.m', but tests that we omit
 // specific checks on platforms where they don't make sense.
 
 // Omit the 'rand' check since 'arc4random' is not available on Linux.
 int      rand(void);
 double   drand48(void);

cfe/trunk/test/Analysis/security-syntax-checks.c

+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-checker=security.insecureAPI
+
+void builtin_function_call_crash_fixes(char *c) {
+  __builtin_strncpy(c, "", 6); // expected-warning{{Call to function 'strncpy' is insecure as it does not provide security checks introduced in the C11 standard.}}
+  __builtin_memset(c, '\0', (0)); // expected-warning{{Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard.}}
+  __builtin_memcpy(c, c, 0); // expected-warning{{Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard.}}
+}

cfe/trunk/test/Analysis/security-syntax-checks.m

-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DUSE_BUILTINS -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DUSE_BUILTINS -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DUSE_BUILTINS -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi -DUSE_BUILTINS -DVARIANT -analyzer-checker=security.insecureAPI,security.FloatLoopCounter %s -verify
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN:   -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN:   -DVARIANT \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN:   -DUSE_BUILTINS -DVARIANT \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN:   -DUSE_BUILTINS \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN:   -DVARIANT \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
+
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN:   -DUSE_BUILTINS -DVARIANT \
+// RUN:   -analyzer-checker=security.insecureAPI \
+// RUN:   -analyzer-checker=security.FloatLoopCounter
 
 #ifdef USE_BUILTINS
 # define BUILTIN(f) __builtin_ ## f
 #else /* USE_BUILTINS */
 # define BUILTIN(f) f
 #endif /* USE_BUILTINS */
 
 #include "Inputs/system-header-simulator-for-valist.h"