[analyzer] Suppress false positive warnings form security.insecureAPI.strcpy

[leanil]

Summary

It is safe to copy a string literal to an array which is compile time known to be large enough.
This reduces the number of false positives, while (hopefully) not introducing any false negatives.

[xazax.hun]

In the tests there are multiple variants of the strcpy function guarded by macros. Maybe we should run the tests multiple times to test all variants with different defines?

[leanil]

Move negative test next to the positive ones, because the necessary macros and run-lines are already defined there.

[NoQ]

This patch makes a totally valid point :)

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

513 ↗

(On Diff #127739)

You might want to use a wider integer type because 64-bit arrays may have 2³¹ or more elements (not sure about string literals).

[leanil]

Change result types to match the query return types.

[george.karpenkov]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

522 ↗

(On Diff #129465)

Nesting this if- inside the previous one would simplify the outer scope and let you assign to ArraySize at declaration size.

526 ↗

(On Diff #129465)

Why not put this if-expression into the one above where StrLen is found?
That would eliminate StrLenFound and remove the potential error surface of uninitialized read from StrLen (the declaration for which should probably be inside this block as well)

test/Analysis/security-syntax-checks.m

151 ↗

(On Diff #129465)

I think it would make sense to add another test case where the warning is expected, even though string length and array size are known at compile time.

[leanil]

Nest condition checking. Add positive test.

[leanil]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

526 ↗

(On Diff #129465)

Good point. This makes StrLen itself redundant as well.

[NoQ]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517 ↗

(On Diff #129508)

This can be simplified to const auto *Array = DeclRef->getType()->getAs<ConstantArrayType>().
.getTypePtr() is almost always redundant because of the fancy operator->() on QualType.

518 ↗

(On Diff #129508)

Hmm, actually, ArraySize is the number of elements in the array, not its byte size, so you may want (if you like) to also suppress the warning when the array is not a char array (even if it's a weird code anyway) by instead taking ASTContext.getTypeSize() here instead.

Also i guess it's nice to use uint64_t because that's the return type of .getLimitedValue() and that's what we usually use all over the place when we have to deal with those values.

[NoQ]

Do you have commit access or should someone else commit it for you?

[leanil]

Measure array size in bytes instead of elements.

[leanil]

In D41384#973851, @NoQ wrote:

Do you have commit access or should someone else commit it for you?

I don't have, please commit it.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517 ↗

(On Diff #129508)

Using getAs yielded:

error: static assertion failed: ArrayType cannot be used with getAs!

[NoQ]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517 ↗

(On Diff #129508)

Whoops, yeah, right, array types are the rare exception. It should be ASTContext.getAsConstantArrayType(), see the docs for getAsArrayType() for some explanation of why it was made this way. I guess we don't really care about these aspects, so your code is fine :)

[dcoughlin]

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

517

Rather than dividing by '8', I suggest using ASTContext's getTypeSizeInChars(). This will make sure we handle those annoying platforms that don't have 8-bit chars.

[leanil]

Use getTypeSizeInChars to get array size.

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

 //===----------------------------------------------------------------------===//
 void WalkAST::checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) {
   if (!filter.check_strcpy)
     return;
 
   if (!checkCall_strCommon(CE, FD))
     return;
 
+  const auto *Target = CE->getArg(0)->IgnoreImpCasts(),
+             *Source = CE->getArg(1)->IgnoreImpCasts();
+  if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Target))
+    if (const auto *Array = dyn_cast<ConstantArrayType>(DeclRef->getType())) {
+      uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+      if (const auto *String = dyn_cast<StringLiteral>(Source)) {
+        if (ArraySize >= String->getLength() + 1)
+          return;
+      }
+    }
+
   // Issue a warning.
   PathDiagnosticLocation CELoc =
     PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   BR.EmitBasicReport(AC->getDecl(), filter.checkName_strcpy,
                      "Potential insecure memory buffer bounds restriction in "
                      "call 'strcpy'",
                      "Security",
                      "Call to function 'strcpy' is insecure as it does not "

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

 
 void test_strcpy() {
   char x[4];
   char *y;
 
   strcpy(x, y); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
 }
 
+void test_strcpy_2() {
+  char x[4];
+  strcpy(x, "abcd"); //expected-warning{{Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119}}
+}
+
+void test_strcpy_safe() {
+  char x[5];
+  strcpy(x, "abcd");
+}
+
 //===----------------------------------------------------------------------===
 // strcat()
 //===----------------------------------------------------------------------===
 #ifdef VARIANT
 
 #define __strcat_chk BUILTIN(__strcat_chk)
 char *__strcat_chk(char *restrict s1, const char *restrict s2, size_t destlen);