[WIP][Analyzer] PtrToIntegCastLibcChecker

[jkorous]

Summary

Checker for memory address being passed to libc functions via parameters for which such value doesn't make sense.

I have a bunch more functions that I was considering but I feel that those might be used in some funky pointer arithmetics - e. g. Artem mentioned pointer hashing.

arg 0
   cos
   sin
   tan
   acos
   asin
   atan
   atan2
   cosh
   sinh
   tanh
   acosh 
   asinh 
   atanh 
   exp
   frexp
   log
   log10
   modf
   exp2 
   expm1 
   ilogb 
   log1p 
   log2 
   logb 
   pow
   sqrt
   cbrt
   erf
   erfc
   tgmamma
   lgamma

 arg 0, 1
   ldexp
   scalbn
   scalbln
   hypot
   remainder
   remquo
   copysign

[jkorous]

clang/lib/StaticAnalyzer/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp

115

Actually, maybe this is too opinionated. Maybe there is a use-case for writing a single char via putc function family?

[NoQ]

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

I also wouldn't mind taking the opposite approach: assume by default that all standard library functions should be warned about, and then add exceptions as needed.

Also what's your excuse for using visitors instead of ASTMatchers this time? :) I think even a simple matcher callExpr().bind("result") that delegates all the work to the imperative match-callback may make the code a lot more concise (by the virtue of ditching the visitor). Argument type checks may look pretty nice in matcher form as well.

[NoQ]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

25

core is a good package because this checker does in fact affect everybody but i don't want to put the checker here because we have a rule that checkers in core cannot be disabled. For that reason it has no path-insensitive checkers (because they don't interact with each other and therefore there's no technical reason to prevent the users from disabling them).

It looks like unix is the traditional place to put warnings about C standard library functions (eg., MallocChecker and StreamChecker belong to unix). It probably shouldn't be this way. I wish we had a better package for C standard library related checkers. Name suggestions anybody? I guess libc is a nice name. c is too short which might cause it to interact badly with our dreams about package-granular generic warning suppression mechanisms.

Our package system is generally pretty messy and prone to legacy / backwards compatibility concerns.

clang/lib/StaticAnalyzer/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp

115

This checker is by design a bug-prone / red flag pattern detector; it doesn't promise to find 100% bugs. It has a natural suppression ("put the casted pointer into a variable"). I wouldn't mind being warning liberally until we see actual intentional use-cases in practice.

clang/test/Analysis/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.c

11

Wait, does this actually work? I thought you have to use expected-warning-re and an extra pair of double braces to get regexps working, otherwise it looks for a literal .*.

[Szelethus]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

25

libc sounds nice! Hopefully we can migrate to a better labeling system sooner rather than later -- its a plus that everyone agrees that we should, and I personally don't foresee any large obstacles in the way of that.

[Szelethus]

@martong has been working hard on libc support for the analyzer, I'll add him :)

[baloghadamsoftware]

In D88332#2297374, @NoQ wrote:

Also what's your excuse for using visitors instead of ASTMatchers this time? :) I think even a simple matcher callExpr().bind("result") that delegates all the work to the imperative match-callback may make the code a lot more concise (by the virtue of ditching the visitor). Argument type checks may look pretty nice in matcher form as well.

+1

I would even go further. Why Clang Static Analyzer? I see nothing path-sensitive here. IMHO a better place to implement this functionality is Clang-Tidy.

clang/test/Analysis/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.c

11

Whether it works or not, we never do that actually. Please write out the warning we should get here.

[jkorous]

In D88332#2297374, @NoQ wrote:

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

Sounds interesting but I lack context to imagine the details. Would the map be still initialized by some ASTVisitor? Do you think there'd be any noticeable performance hit?

I also wouldn't mind taking the opposite approach: assume by default that all standard library functions should be warned about, and then add exceptions as needed.

How about I add the functions I mentioned above to alpha variation of this checker?

Also what's your excuse for using visitors instead of ASTMatchers this time? :) I think even a simple matcher callExpr().bind("result") that delegates all the work to the imperative match-callback may make the code a lot more concise (by the virtue of ditching the visitor). Argument type checks may look pretty nice in matcher form as well.

Busted... I just reached for the tool I am used to. I think you're right - let me use a matcher here.

clang/lib/StaticAnalyzer/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp

115

Ok.

FWIW I was intentionally pretty conservative to not deviate too much from the general expectation I assume people have of the analyzer - not too much false positives.
Here, putc is probably fine but otherwise I won't feel comfortable producing too much noise - again it's just my assumption but I feel that once someone decides that a particular checker produces too much false positives (esp. at callsites) we lose that user for the foreseable future.

WDYT? Am I overthinking it?

clang/test/Analysis/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.c

11

For sure! Sorry, I never intended to land it as it is (this definitely falls under the "WIP"). I just somewhat expected that name/package might change and given that expectation - I was "lazy" and just added the TODO below.

@NoQ You're right - this doesn't work. It's just a half-baked draft of tests based on my ad-hoc test file.

Since you are here - I was actually more wondering about what test coverage should I aim for. Is a somewhat random trio of libc functions with somewhat diverse parameters good enough here?

[jkorous]

In D88332#2298301, @baloghadamsoftware wrote:

I would even go further. Why Clang Static Analyzer? I see nothing path-sensitive here. IMHO a better place to implement this functionality is Clang-Tidy.

This concern has been brought up before. I hope you won't mind me pasting @NoQ 's response as it nicely summarizes the situation.

... Also these tools are not a drop-in replacement of each other: you cannot use clang-tidy in all the places where you can use the static analyzer (the opposite is partially true due to libStaticAnalyzer integration into clang-tidy but UI degrades dramatically when such integration is used) therefore many authors simply do not have a choice where to put the check (i'm slowly working on improving this situation so that most path-insensitive checks could indeed be migrated into clang-tidy but we're not there yet). So as of today we have no choice but to let our contributors decide freely where they want their checks to be.

[NoQ]

In D88332#2299912, @jkorous wrote:

In D88332#2297374, @NoQ wrote:

Hmm, if we could add an interface to CallDescriptionMap to accept AnyCall instead of CallEvent then we could use it for path-insensitive checkers as well. That'd have saved some lines of code.

Sounds interesting but I lack context to imagine the details. Would the map be still initialized by some ASTVisitor?

Nah, it's brace-initialized with names as usual.

In D88332#2299912, @jkorous wrote:

Do you think there'd be any noticeable performance hit?

Your code is already linear. As of now CallDescriptionMap is linear too, but if we are to make any improvements to that, i'd rather do them once in CallDescriptionMap than duplicate them in every checker. The problem of identifying functions in the AST is so ridiculously common that we absolutely have to have a unified solution to it. It really hurts when 100 checkers are making the same mistakes over and over again (not accounting for builtins, not accounting for inline namespaces, crashing on accessing a non-existing argument of a call, etc.).

clang/lib/StaticAnalyzer/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp

115

I mean, like, as part of our testing we should take a look at the warnings and see how bad they are when they're wrong and use best judgement (:

For putc specifically i wouldn't be shocked that some code would actively use that to print pointers. But i'd be shocked if that's actually a lot of warnings on a single project, rather than "that one lonely function in which it actually makes sense".

The opposite thing we could do would be to only warn on functions that do inherently introduce some source of confusion. Say, you could mix up the two arguments in strncpy() and accidentally swap the pointer and the size (this partially overlaps with clang-tidy's bugprone-swapped-arguments check).

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // hierarchy checkers would have had if they were truly at the top level.
 // (For example, a Cocoa-specific checker that is alpha should be in
 // alpha.osx.cocoa).
 def Alpha : Package<"alpha">;
 
 def Core : Package<"core">;
 def CoreBuiltin : Package<"builtin">, ParentPackage<Core>, Hidden;
 def CoreUninitialized  : Package<"uninitialized">, ParentPackage<Core>;
+def CorePointerToIntegralTypeCast  : Package<"PointerToIntegralTypeCast">, ParentPackage<Core>;
 def CoreAlpha : Package<"core">, ParentPackage<Alpha>;
 
 // The OptIn package is for checkers that are not alpha and that would normally
 // be on by default but where the driver does not have enough information to
 // determine when they are applicable. For example, localizability checkers fit
 // this criterion because the driver cannot determine whether a project is
 // localized or not -- this is best determined at the IDE or build-system level.
 //
def UndefCapturedBlockVarChecker : Checker<"CapturedBlockVariable">,
   Documentation<NotDocumented>;
 
 def ReturnUndefChecker : Checker<"UndefReturn">,
   HelpText<"Check for uninitialized values being returned to the caller">,
   Documentation<HasDocumentation>;
 
 } // end "core.uninitialized"
 
+let ParentPackage = CorePointerToIntegralTypeCast in {
+
+def PtrToIntegCastLibcChecker : Checker<"LibcAPI">,
+  HelpText<"Check for misuse of libc APIs by passing a pointer casted to its integral value">,
+  Documentation<HasDocumentation>;
+} // end "core.PointerToIntegralTypeCast"
+
 //===----------------------------------------------------------------------===//
 // Unix API checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CString in {
 
 def CStringModeling : Checker<"CStringModeling">,
   HelpText<"The base of several CString related checkers. On it's own it emits "

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   OSObjectCStyleCast.cpp
   PaddingChecker.cpp
   PointerArithChecker.cpp
   PointerIterationChecker.cpp
   PointerSortingChecker.cpp
   PointerSubChecker.cpp
+  PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp
   PthreadLockChecker.cpp
   cert/PutenvWithAutoChecker.cpp
   RetainCountChecker/RetainCountChecker.cpp
   RetainCountChecker/RetainCountDiagnostics.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp

clang/lib/StaticAnalyzer/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.cpp

+//== CStringSyntaxChecker.cpp - CoreFoundation containers API *- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// An AST checker that looks for common pitfalls when using C string APIs.
+//  - Identifies erroneous patterns in the last argument to strncat - the number
+//    of bytes to copy.
+//
+//===----------------------------------------------------------------------===//
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/OperationKinds.h"
+#include "clang/AST/StmtVisitor.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Basic/TypeTraits.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/raw_ostream.h"
+#include <llvm/ADT/StringRef.h>
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class FindMemAddrUsedAsSize: public StmtVisitor<FindMemAddrUsedAsSize> {
+  const CheckerBase *Checker;
+  BugReporter &BR;
+  AnalysisDeclContext* AC;
+public:
+  FindMemAddrUsedAsSize(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
+      : Checker(Checker), BR(BR), AC(AC) {}
+
+  // This is just traversing.
+  void VisitChildren(Stmt *S) {
+   for (Stmt *Child : S->children())
+    if (Child)
+      Visit(Child);
+  }
+  void VisitStmt(Stmt *S) {
+    VisitChildren(S);
+  }
+
+  // The business part.
+  void VisitCallExpr(CallExpr *CE);
+};
+
+void FindMemAddrUsedAsSize::VisitCallExpr(CallExpr *CE) {
+  const FunctionDecl *FD = CE->getDirectCallee();
+  if (!FD)
+    return;
+
+  // FIXME: We can skip some arithmetic expressions too.
+  // FIXME: The only class of arithmetic expression that seems legal is address subtraction (seems like a legal way how to find length of a buffer).
+  // FIXME: + * / seem illegal in 100% cases.
+  // FIXME: Subtraction of a constant also seem illegal. Subtraction of a variable has false positives - e. g. there's an address stored in the variable.
+  // FIXME: A weird (but rare?) false positive would also be nullptr converted to int.
+  auto SkipCastsToPtrToIntCast = [](const Expr *E) -> const CastExpr * {
+    // Example: strlen() address passed as lenght
+    // `-ImplicitCastExpr 'unsigned long (*)(const char *)' <FunctionToPointerDecay> part_of_explicit_cast
+    //   `-DeclRefExpr 'unsigned long (const char *)' Function 'strlen' 'unsigned long (const char *)'
+    while (E) {
+      const CastExpr * Cast = dyn_cast<CastExpr>(E);
+      if (Cast) {
+        if (Cast->getCastKind() == CK_PointerToIntegral) {
+          return Cast;
+        }
+        E = Cast->getSubExpr();
+        continue;
+      }
+      // We don't want to skip any other nodes.
+      break;
+    }
+    return nullptr;
+  };
+
+  auto NameIs = [&FD](llvm::StringRef name) {
+    return CheckerContext::isCLibraryFunction(FD, name);
+  };
+
+  // Parameters of specific functions which semantics won't match memory address value.
+  llvm::DenseSet<const Expr *> ArgsOfInterest;
+  // FIXME: We could further decreate the number of false positives by checking function signature.
+  if (
+    NameIs("feclearexcept")
+    || NameIs("feraiseexcept")
+    || NameIs("fesetround")
+    || NameIs("fetestexcept")
+    || NameIs("setlocale")
+    || NameIs("ceil")
+    || NameIs("floor")
+    || NameIs("trunc")
+    || NameIs("round")
+    || NameIs("lround")
+    || NameIs("llround")
+    || NameIs("rint")
+    || NameIs("lrint")
+    || NameIs("llrint")
+    || NameIs("nearbyint")
+    || NameIs("fdim")
+    || NameIs("fmax")
+    || NameIs("fmin")
+    || NameIs("fabs")
+    || NameIs("abs")
+    || NameIs("fma")
+    || NameIs("fputc")
+    || NameIs("putc")
+    || NameIs("putchar")
+    || NameIs("ungetc")
+    || NameIs("malloc")
+    || NameIs("labs")
+    || NameIs("llabs")
+    || NameIs("fputwc")
+    || NameIs("putwc")
+    || NameIs("putwchar")
+    || NameIs("ungetwc")
+    || NameIs("btowc")
+    || NameIs("iswcntrl")
+    || NameIs("iswdigit")
+    || NameIs("iswgraph")
+    || NameIs("iswlower")
+    || NameIs("iswprint")
+    || NameIs("iswpunct")
+    || NameIs("iswspace")
+    || NameIs("iswupper")
+    || NameIs("iswxdigit")
+    || NameIs("towlower")
+    || NameIs("towupper")
+    || NameIs("iswctype")
+    || NameIs("towctrans")
+  ) {
+    ArgsOfInterest.insert(CE->getArg(0));
+  } else if (
+    NameIs("fegetexceptflag")
+    || NameIs("fesetexceptflag")
+    || NameIs("longjmp")
+    || NameIs("raise")
+    || NameIs("snprintf")
+    || NameIs("vsnprintf")
+    || NameIs("fgets")
+    || NameIs("realloc")
+    || NameIs("mblen")
+    || NameIs("wctomb")
+    || NameIs("strftime")
+    || NameIs("c16rtomb")
+    || NameIs("c32rtomb")
+    || NameIs("fgetws")
+    || NameIs("fwide")
+    || NameIs("swprintf")
+    || NameIs("vswprintf")
+    || NameIs("mbrlen")
+    || NameIs("wcrtomb")
+    || NameIs("wcschr")
+    || NameIs("wcsrchr")
+    || NameIs("wcsftime")
+  ) {
+    ArgsOfInterest.insert(CE->getArg(1));
+  } else if (
+    NameIs("strtol")
+    || NameIs("strtoll")
+    || NameIs("strtoul")
+    || NameIs("strtoull")
+    || NameIs("mbtowc")
+    || NameIs("mbstowcs")
+    || NameIs("wcstombs")
+    || NameIs("memcpy")
+    || NameIs("memmove")
+    || NameIs("strncpy")
+    || NameIs("mbrtoc16")
+    || NameIs("mbrtoc32")
+    || NameIs("wcstol")
+    || NameIs("wcstoll")
+    || NameIs("wcstoul")
+    || NameIs("wcstoull")
+    || NameIs("mbrtowc")
+    || NameIs("mbsrtowcs")
+    || NameIs("wcsrtombs")
+    || NameIs("wcsncat")
+    || NameIs("wcsncmp")
+    || NameIs("wcsncpy")
+    || NameIs("wcsxfrm")
+    || NameIs("wmemcmp")
+    || NameIs("wmemcpy")
+    || NameIs("wmemmove")
+  ) {
+    ArgsOfInterest.insert(CE->getArg(2));
+  } else if (NameIs("calloc")) {
+    ArgsOfInterest.insert(CE->getArg(0));
+    ArgsOfInterest.insert(CE->getArg(1));
+  } else if (
+    NameIs("fread")
+    || NameIs("fwrite")
+    || NameIs("fseek")
+    || NameIs("qsort")
+    || NameIs("wmemchr")
+    || NameIs("wmemset")
+  ) {
+    ArgsOfInterest.insert(CE->getArg(1));
+    ArgsOfInterest.insert(CE->getArg(2));
+  } else if (
+    NameIs("bsearch")
+    || NameIs("setvbuf")
+  ) {
+    ArgsOfInterest.insert(CE->getArg(2));
+    ArgsOfInterest.insert(CE->getArg(3));
+  }
+
+  for (const Expr* Arg : ArgsOfInterest) {
+    const CastExpr *PtrToIntCast = SkipCastsToPtrToIntCast(Arg);
+
+    if (PtrToIntCast) {
+      PathDiagnosticLocation Loc =
+        PathDiagnosticLocation::createBegin(PtrToIntCast, BR.getSourceManager(), AC);
+
+      SmallString<256> S;
+      llvm::raw_svector_ostream os(S);
+      os << "Memory address is used as an integer in context where it's likely to be a bug.";
+      // TODO get name & type of a DeclRefExpr from LenArg (past PtrToIntCast)
+
+      BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+                         "C String API", os.str(), Loc,
+                         Arg->getSourceRange());
+    }
+  }
+
+  // Recurse and check children.
+  VisitStmt(CE);
+}
+
+} // end anonymous namespace
+
+namespace {
+class PtrToIntegCastLibcChecker : public Checker<check::ASTCodeBody> {
+public:
+
+  void checkASTCodeBody(const Decl *D, AnalysisManager& Mgr,
+      BugReporter &BR) const {
+    FindMemAddrUsedAsSize walker(this, BR, Mgr.getAnalysisDeclContext(D));
+    walker.Visit(D->getBody());
+  }
+};
+}
+
+void ento::registerPtrToIntegCastLibcChecker(CheckerManager &mgr) {
+  mgr.registerChecker<PtrToIntegCastLibcChecker>();
+}
+
+bool ento::shouldRegisterPtrToIntegCastLibcChecker(const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/Checkers/PointerToIntegralTypeCast/PtrToIntegCastLibcChecker.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core.PointerToIntegralTypeCast.LibcAPI -verify %s
+
+#define NULL 0
+typedef unsigned long size_t;
+
+typedef void* FILE;
+int putc(int c, FILE *stream);
+void putc_misuse(void) {
+    FILE* file = NULL; 
+    int* a_random_ptr = NULL;
+    putc((size_t) a_random_ptr, file); // expected-warning {{.*}}
+    // TODO proper expected
+}
+
+void *memmove(void *dest, const void *src, size_t n);
+size_t get_size_of_buffer(int* buffer) { /* real impl would be here */ return 0; } 
+void memmove_misuse(void) {
+    int dest[10];
+    int* src = NULL;
+    memmove(dest, src, (size_t) get_size_of_buffer); // expected-warning {{.*}}
+    // TODO proper expected
+}
+
+void *memcpy(void *dest, const void *src, size_t n);
+void memcpy_misuse() {
+    int dest[10];
+    int* src = NULL;
+    size_t buffer_sizes[1];
+    memcpy(dest, src, (size_t) buffer_sizes); // expected-warning {{.*}}
+    // TODO proper expected
+}
 No newline at end of file