[analyzer] Correctly measure array size in security.insecureAPI.strcpy

[leanil]

Summary

This will handle those platforms that don't have 8-bit chars.
This is a follow up fix to review D41384, which has been committed since.

[leanil]

getQuantity() returns a signed type

[xazax.hun]

LGTM!

[NoQ]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517

I suggest not using auto here, because it makes it harder to understand integer promotions (potential overflows or sign extensions) in the comparison.

[leanil]

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517

That's true. Should it be CharUnits::QuantityType then?

[NoQ]

Sorry, i completely forgot about this one :(

I think this patch needs lit tests, eg. tell the analyzer to analyze a simple strcpy() call on any -target with non-8-bit chars and see if it's still crashes or behaves incorrectly.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

517

I think you should still getQuantity(), and then explicitly cast it to a type that's compatible with String->getLength(), so that there were no implicit integer casts around >=.

lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp

void WalkAST::checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) {
 
   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;
+      uint64_t ArraySize = BR.getContext().getTypeSizeInChars(Array).getQuantity();
       if (const auto *String = dyn_cast<StringLiteral>(Source)) {
         if (ArraySize >= String->getLength() + 1)
           return;
       }
     }
 
   // Issue a warning.
   PathDiagnosticLocation CELoc =