[analyzer] pr43179: CallDescription: Check number of parameters as well as number of arguments.

[NoQ]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=43179.

Most functions that our checkers react upon are not C-style variadic functions, and therefore they have as many actual arguments as they have formal parameters.

However, it's not impossible to define a variadic function with the same name. This will crash any checker that relies on CallDescription to check the number of arguments but then silently assumes that the number of parameters is the same.

Change CallDescription to check both the number of arguments and the number of parameters by default.

If we're intentionally trying to match variadic functions, allow specifying arguments and parameters separately (possibly omitting any of them). For now we only have one CallDescription which would make use of those, namely __builtin_va_start itself.

[Charusso]

I like the isolation of this kind of stuff. Thanks you!

[NoQ]

For the actual variadic functions i'm thinking of something like {CDF_Variadic, "fprintf", 2} which will match all calls to variadic functions named "fprintf" that have exactly two non-variadic parameters and at least two arguments on the call site [which occupy those non-variadic parameters]. I'm not implementing it yet because we don't need it yet.

I'm not sure if that's the best syntax. I didn't think too deeply about it because i'm waiting for more examples to generalize upon.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

class CallDescription {
   friend CallEvent;
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
   // The list of the qualified names used to identify the specified CallEvent,
   // e.g. "{a, b}" represent the qualified names, like "a::b".
   std::vector<const char *> QualifiedName;
   Optional<unsigned> RequiredArgs;
+  Optional<size_t> RequiredParams;
   int Flags;
 
+  // A constructor helper.
+  static Optional<size_t> readRequiredParams(Optional<unsigned> RequiredArgs,
+                                             Optional<size_t> RequiredParams) {
+    if (RequiredParams)
+      return RequiredParams;
+    if (RequiredArgs)
+      return static_cast<size_t>(*RequiredArgs);
+    return None;
+  }
+
 public:
   /// Constructs a CallDescription object.
   ///
   /// @param QualifiedName The list of the name qualifiers of the function that
   /// will be matched. The user is allowed to skip any of the qualifiers.
   /// For example, {"std", "basic_string", "c_str"} would match both
   /// std::basic_string<...>::c_str() and std::__1::basic_string<...>::c_str().
   ///
   /// @param RequiredArgs The number of arguments that is expected to match a
   /// call. Omit this parameter to match every occurrence of call with a given
   /// name regardless the number of arguments.
   CallDescription(int Flags, ArrayRef<const char *> QualifiedName,
-                  Optional<unsigned> RequiredArgs = None)
+                  Optional<unsigned> RequiredArgs = None,
+                  Optional<size_t> RequiredParams = None)
       : QualifiedName(QualifiedName), RequiredArgs(RequiredArgs),
+        RequiredParams(readRequiredParams(RequiredArgs, RequiredParams)),
         Flags(Flags) {}
 
   /// Construct a CallDescription with default flags.
   CallDescription(ArrayRef<const char *> QualifiedName,
-                  Optional<unsigned> RequiredArgs = None)
-      : CallDescription(0, QualifiedName, RequiredArgs) {}
+                  Optional<unsigned> RequiredArgs = None,
+                  Optional<size_t> RequiredParams = None)
+      : CallDescription(0, QualifiedName, RequiredArgs, RequiredParams) {}
 
   /// Get the name of the function that this object matches.
   StringRef getFunctionName() const { return QualifiedName.back(); }
 };
 
 /// An immutable map from CallDescriptions to arbitrary data. Provides a unified
 /// way for checkers to react on function calls.
 template <typename T> class CallDescriptionMap {

clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

ValistChecker::VAListAccepters = {
         {{"vfwprintf", 3}, 2},
         {{"vfwscanf", 3}, 2},
         {{"vwprintf", 2}, 1},
         {{"vwscanf", 2}, 1},
         {{"vswprintf", 4}, 3},
         // vswprintf is the wide version of vsnprintf,
         // vsprintf has no wide version
         {{"vswscanf", 3}, 2}};
-const CallDescription ValistChecker::VaStart("__builtin_va_start", 2),
+
+const CallDescription
+    ValistChecker::VaStart("__builtin_va_start", /*Args=*/2, /*Params=*/1),
     ValistChecker::VaCopy("__builtin_va_copy", 2),
     ValistChecker::VaEnd("__builtin_va_end", 1);
 } // end anonymous namespace
 
 void ValistChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   if (!Call.isGlobalCFunction())
     return;

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

bool CallEvent::isCalled(const CallDescription &CD) const {
   if (!II)
     return false;
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(getDecl());
   if (!FD)
     return false;
 
   if (CD.Flags & CDF_MaybeBuiltin) {
     return CheckerContext::isCLibraryFunction(FD, CD.getFunctionName()) &&
-           (!CD.RequiredArgs || CD.RequiredArgs <= getNumArgs());
+           (!CD.RequiredArgs || CD.RequiredArgs <= getNumArgs()) &&
+           (!CD.RequiredParams || CD.RequiredParams <= parameters().size());
   }
 
   if (!CD.IsLookupDone) {
     CD.IsLookupDone = true;
     CD.II = &getState()->getStateManager().getContext().Idents.get(
         CD.getFunctionName());
   }
 
for (; Ctx && isa<NamedDecl>(Ctx); Ctx = Ctx->getParent()) {
         continue;
       }
     }
 
     if (NumUnmatched > 0)
       return false;
   }
 
-  return (!CD.RequiredArgs || CD.RequiredArgs == getNumArgs());
+  return (!CD.RequiredArgs || CD.RequiredArgs == getNumArgs()) &&
+         (!CD.RequiredParams || CD.RequiredParams == parameters().size());
 }
 
 SVal CallEvent::getArgSVal(unsigned Index) const {
   const Expr *ArgE = getArgExpr(Index);
   if (!ArgE)
     return UnknownVal();
   return getSVal(ArgE);
 }

clang/test/Analysis/cast-value-weird.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,apiModeling -verify %s
+
+// expected-no-diagnostics
+
+namespace llvm {
+template <typename>
+void cast(...);
+void a() { cast<int>(int()); } // no-crash
+} // namespace llvm