[analyzer] Add new checker for unchecked return value.

[balazske]

Summary

Add a checker that checks if a function is used without examining
its return value. The set of functions is taken from CERT rule
ERR33-C "Detect and handle standard library errors". This is a
simplified check for the problem.

[balazske]

This is a very simplified form of the check that is implemented in D72705 but still may be useful in practical cases. This check could be a clang-tidy check but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

42

Please note that the CallExpr does not necessarily stands alone. It may be wrapped into an ExprWithCleanUps. We should consider these CallExprs as unchecked too.

86

Hmm, why StringMap<>? Why not CallDescriptionMap<>?

clang/test/Analysis/unchecked-return-value.cpp

11

Please use some valid format here. E.g. scanf("%*c");

17

Please add such simple test case for all the functions we try to check. (One call is enough for every such function, either in std:: or on the top namespace.)

[balazske]

Small fixes in test.

[balazske]

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

42

It looks like that the matcher finds these occurrences too. A test was added for it.

86

CallDescriptionMap is only usable with CallEvent that is not used in this checker. Or it can be extended with lookup from FunctionDecl?

[balazske]

Maybe it is better to have this check for every system function that has non-void return value (except specific functions)? This check is applicable to CERT rule ERR33-C, POS54-C and EXP12-C too (it is restricted but can find a part of the problems).

[balazske]

Ping

[Szelethus]

Looks like a neat checker! I guess the only question is the function matching, and I don't dislike it in its current state. @martong, do you have any thoughts on this?

On another note, have you checked this on some projects yet? I expect that we might need to tone down on some of the functions if this ends up being too noisy.

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

66–67

I suppose we need this to compensate for the fact that we can't reliably match on standard C functions. Unfortunately, using CallDescription wouldn't necessarily solve this: D81745.

86

Good point. @martong, you have used FunctionDecl based checking, do you have any thoughts here?

[vsavchenko]

Great job!
I'd like to see more tests with the following cases:

• more expressions with function calls - ternary expression, arithmetic, array literals, initializer lists, etc.
• using/not using it in if statement with initializer
• using/not using it in different parts of GNU statement expressions
• calls in different parts of exception handling (both C++ and Obj-C)
• calls in lambdas/blocks/coroutines
• calls in different parts of C++ range-for loop
• calls in different parts of Obj-C for loop over collections
• calls under labels
• calls in attributed statements (e.g. [[clang::nomerge]] foo())
• calls in goto statements
• calls in Obj-C @synchronized and @autoreleasepool statements

[NoQ]

but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked

This is literally __attribute__((warn_unused_result)) for which there already is a compiler warning.

[balazske]

There is a clang-tidy check bugprone-unused-return-value that may be confusing in relation to this checker. It does similar job for a selected set of functions and with different reason.

clang/docs/analyzer/checkers.rst

 Warn on using a floating point value as a loop counter (CERT: FLP30-C, FLP30-CPP).
 
 .. code-block:: c
 
  void test() {
    for (float x = 0.1f; x <= 1.0f; x += 0.1f) {} // warn
  }
 
+security.UncheckedReturnValue
+"""""""""""""""""""""""""""""
+Warn on uses of functions whose return values must be checked (CERT: ERR33-C).
+The detection is limited to the case when the return value of the function
+is completely ignored. The detection is applied in C++ code too.
+List of checked functions is:
+FIXME: Fill the list.
+
+.. code-block:: c
+
+ void f();
+
+ void test() {
+   atexit(f); // warn: return value of atexit should be checked
+ }
+
 .. _security-insecureAPI-UncheckedReturn:
 
 security.insecureAPI.UncheckedReturn (C)
 """"""""""""""""""""""""""""""""""""""""
 Warn on uses of functions whose return values must be always checked.
+This check detects ignored return value at calls to
+'setuid', 'setgid', 'seteuid', 'setegid', 'setreuid', 'setregid'.
 
 .. code-block:: c
 
  void test() {
    setuid(1); // warn
  }
 
 .. _security-insecureAPI-bcmp:

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

 let ParentPackage = Security in {
 
 def FloatLoopCounter : Checker<"FloatLoopCounter">,
   HelpText<"Warn on using a floating point value as a loop counter (CERT: "
            "FLP30-C, FLP30-CPP)">,
   Dependencies<[SecuritySyntaxChecker]>,
   Documentation<HasDocumentation>;
 
+def UncheckedReturnValueChecker : Checker<"UncheckedReturnValue">,
+  HelpText<"Warn on uses of functions whose return values must be always "
+           "checked.">,
+  Documentation<HasDocumentation>;
+
 } // end "security"
 
 let ParentPackage = POS in {
 
   def PutenvWithAuto : Checker<"34c">,
   HelpText<"Finds calls to the 'putenv' function which pass a pointer to "
            "an automatic variable as the argument.">,
   Documentation<HasDocumentation>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   StdLibraryFunctionsChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
+  UncheckedReturnValueChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
   UninitializedObject/UninitializedObjectChecker.cpp
   UninitializedObject/UninitializedPointee.cpp
   UnixAPIChecker.cpp

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

+//===- ReturnValueChecker - Applies guaranteed return values ----*- 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines UncheckedReturnValueChecker, which checks for calls to functions
+// whose return value has to be observed by the program but is ignored.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+
+static bool isInNoNamespace(const FunctionDecl *FD) {
+  const DeclContext *DC = FD->getDeclContext();
+  while (DC && DC->isTransparentContext())
+    DC = DC->getParent();
+  assert(DC && "Function should have a declaration context.");
+  return DC->isTranslationUnit();
+}
+
+class UncheckedReturnValueChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
+                        BugReporter &BR) const {
+    auto FoundCall = callExpr().bind("call");
+    auto CallInCompound = compoundStmt(forEach(FoundCall));
+    auto CallInIf = ifStmt(anyOf(hasThen(FoundCall), hasElse(FoundCall)));
+    auto CallInFor = forStmt(anyOf(hasBody(FoundCall), hasLoopInit(FoundCall),
+                                   hasIncrement(FoundCall)));
+    auto CallInForRange = cxxForRangeStmt(hasBody(FoundCall));
+    auto CallInWhile = whileStmt(hasBody(FoundCall));
+    auto CallInDo = doStmt(hasBody(FoundCall));
+    auto CallInCase = caseStmt(has(FoundCall));
+    auto CallInAnyStmt =
+        stmt(anyOf(CallInCompound, CallInIf, CallInFor, CallInForRange,
+                   CallInWhile, CallInDo, CallInCase));
+    auto Matches =
+        match(stmt(findAll(CallInAnyStmt)), *D->getBody(), AM.getASTContext());
+
+    for (BoundNodes Match : Matches) {
+      const auto *CE = Match.getNodeAs<CallExpr>("call");
+      assert(CE);
+      const FunctionDecl *FD = CE->getDirectCallee();
+      if (!FD)
+        continue;
+      FD = FD->getCanonicalDecl();
+
+      if (!AM.getSourceManager().isInSystemHeader(FD->getLocation()))
+        continue;
+      if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
+        continue;
+
+      if (isFunctionToCheck(FD)) {
+        // Issue a warning.
+        SmallString<512> buf;
+        llvm::raw_svector_ostream os(buf);
+        os << "Return value is not checked in call to '" << *FD << "'.";
+
+        PathDiagnosticLocation CELoc = PathDiagnosticLocation::createBegin(
+            CE, BR.getSourceManager(), AM.getAnalysisDeclContext(D));
+
+        BR.EmitBasicReport(D, getCheckerName(), "Unchecked return value",
+                           categories::SecurityError, os.str(), CELoc);
+      }
+    }
+  }
+
+private:
+  llvm::StringMap<int> FunctionsToCheck = {
+      {"aligned_alloc", 2}, {"asctime_s", 3},     {"at_quick_exit", 1},
+      {"atexit", 1},        {"bsearch", 5},       {"bsearch_s", 6},
+      {"btowc", 1},         {"c16rtomb", 3},      {"c32rtomb", 3},
+      {"calloc", 2},        {"clock", 0},         {"cnd_broadcast", 1},
+      {"cnd_init", 1},      {"cnd_signal", 1},    {"cnd_timedwait", 3},
+      {"cnd_wait", 2},      {"ctime_s", 3},       {"fclose", 1},
+      {"fflush", 1},        {"fgetc", 1},         {"fgetpos", 2},
+      {"fgets", 3},         {"fgetwc", 1},        {"fopen", 2},
+      {"fopen_s", 3},       {"fprintf", -1},      {"fprintf_s", -1},
+      {"fputc", 2},         {"fputs", 2},         {"fputwc", 2},
+      {"fputws", 2},        {"fread", 4},         {"freopen", 3},
+      {"freopen_s", 4},     {"fscanf", -1},       {"fscanf_s", -1},
+      {"fseek", 3},         {"fsetpos", 2},       {"ftell", 1},
+      {"fwprintf", -1},     {"fwprintf_s", -1},   {"fwrite", 4},
+      {"fwscanf", -1},      {"fwscanf_s", -1},    {"getc", 1},
+      {"getchar", 0},       {"getenv", 1},        {"getenv_s", 4},
+      {"gets_s", 2},        {"getwc", 1},         {"getwchar", 0},
+      {"gmtime", 1},        {"gmtime_s", 2},      {"localtime", 1},
+      {"localtime_s", 2},   {"malloc", 1},        {"mblen", 2},
+      {"mbrlen", 3},        {"mbrtoc16", 4},      {"mbrtoc32", 4},
+      {"mbrtowc", 4},       {"mbsrtowcs", 4},     {"mbsrtowcs_s", 6},
+      {"mbstowcs", 3},      {"mbstowcs_s", 5},    {"mbtowc", 3},
+      {"memchr", 3},        {"mktime", 1},        {"mtx_init", 2},
+      {"mtx_lock", 1},      {"mtx_timedlock", 2}, {"mtx_trylock", 1},
+      {"mtx_unlock", 1},    {"printf_s", -1},     {"putc", 2},
+      {"putwc", 2},         {"raise", 1},         {"realloc", 2},
+      {"remove", 1},        {"rename", 2},        {"setlocale", 2},
+      {"setvbuf", 4},       {"scanf", -1},        {"scanf_s", -1},
+      {"signal", 2},        {"snprintf", -1},     {"snprintf_s", -1},
+      {"snwprintf_s", -1},  {"sprintf", -1},      {"sprintf_s", -1},
+      {"sscanf", -1},       {"sscanf_s", -1},     {"strchr", 2},
+      {"strerror_s", 3},    {"strftime", 4},      {"strpbrk", 2},
+      {"strrchr", 2},       {"strstr", 2},        {"strtod", 2},
+      {"strtof", 2},        {"strtoimax", 3},     {"strtok", 2},
+      {"strtok_s", 4},      {"strtol", 3},        {"strtold", 2},
+      {"strtoll", 3},       {"strtoumax", 3},     {"strtoul", 3},
+      {"strtoull", 3},      {"strxfrm", 3},       {"swprintf", -1},
+      {"swprintf_s", -1},   {"swscanf", -1},      {"swscanf_s", -1},
+      {"thrd_create", 3},   {"thrd_detach", 1},   {"thrd_join", 2},
+      {"thrd_sleep", 2},    {"time", 1},          {"timespec_get", 2},
+      {"tmpfile", 0},       {"tmpfile_s", 1},     {"tmpnam", 1},
+      {"tmpnam_s", 2},      {"tss_create", 2},    {"tss_get", 1},
+      {"tss_set", 2},       {"ungetc", 2},        {"ungetwc", 2},
+      {"vfprintf", 3},      {"vfprintf_s", 3},    {"vfscanf", 3},
+      {"vfscanf_s", 3},     {"vfwprintf", 3},     {"vfwprintf_s", 3},
+      {"vfwscanf", 3},      {"vfwscanf_s", 3},    {"vprintf", 2},
+      {"vprintf_s", 2},     {"vscanf", 2},        {"vscanf_s", 2},
+      {"vsnprintf", 4},     {"vsnprintf_s", 4},   {"vsnwprintf_s", 4},
+      {"vsprintf", 3},      {"vsprintf_s", 4},    {"vsscanf", 3},
+      {"vsscanf_s", 3},     {"vswprintf", 4},     {"vswprintf_s", 4},
+      {"vswscanf", 3},      {"vswscanf_s", 3},    {"vwprintf_s", 2},
+      {"vwscanf", 2},       {"vwscanf_s", 2},     {"wcrtomb", 3},
+      {"wcrtomb_s", 5},     {"wcschr", 2},        {"wcsftime", 4},
+      {"wcspbrk", 2},       {"wcsrchr", 2},       {"wcsrtombs", 4},
+      {"wcsrtombs_s", 5},   {"wcsstr", 2},        {"wcstod", 2},
+      {"wcstof", 2},        {"wcstoimax", 3},     {"wcstok", 3},
+      {"wcstok_s", 4},      {"wcstol", 3},        {"wcstold", 2},
+      {"wcstoll", 3},       {"wcstombs", 3},      {"wcstombs_s", 5},
+      {"wcstoumax", 3},     {"wcstoul", 3},       {"wcstoull", 3},
+      {"wcsxfrm", 3},       {"wctob", 1},         {"wctomb", 2},
+      {"wctomb_s", 4},      {"wctrans", 1},       {"wctype", 1},
+      {"wmemchr", 3},       {"wprintf_s", -1},    {"wscanf", -1},
+      {"wscanf_s", -1}};
+
+  bool isFunctionToCheck(const FunctionDecl *FD) const {
+    // Ignore functions without a name.
+    const IdentifierInfo *II = FD->getIdentifier();
+    if (!II)
+      return false;
+
+    auto FoundFunction = FunctionsToCheck.find(II->getName());
+    if (FoundFunction == FunctionsToCheck.end())
+      return false;
+
+    // Value -1 indicates that the function should be variadic.
+    if (FoundFunction->second == -1)
+      return FD->isVariadic();
+
+    // Check parameter count.
+    return FD->getNumParams() == static_cast<unsigned>(FoundFunction->second);
+  }
+};
+
+} // namespace
+
+void ento::registerUncheckedReturnValueChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UncheckedReturnValueChecker>();
+}
+
+bool ento::shouldRegisterUncheckedReturnValueChecker(
+    const CheckerManager &mgr) {
+  return true;
+}

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

public:
   operator()( ForwardIt2 first, ForwardIt2 last ) const;
 };
 
 template <typename> class packaged_task;
 template <typename Ret, typename... Args> class packaged_task<Ret(Args...)> {
   // TODO: Add some actual implementation.
 };
 
+int scanf(const char *format, ...);
+
 } // namespace std
+
+namespace other_ns {
+int scanf(const char *format, ...);
+}

clang/test/Analysis/unchecked-return-value.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=security.UncheckedReturnValue -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+#include "Inputs/system-header-simulator.h"
+
+extern void extern_f(int);
+int btowc(int);
+
+struct S {
+  int getInt() const;
+};
+
+int f1(int X) {
+  scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  std::scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  other_ns::scanf("%*d");
+  {
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X) {
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  if (X)
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  if (scanf("%*d")) {
+  }
+  if (scanf("%*d") > 1) {
+  }
+
+  while (true)
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (scanf("%*d")) {
+  }
+
+  do
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  while (true);
+  do {
+  } while (scanf("%*d"));
+
+  for (int Y = 1; Y < 10; ++Y)
+    scanf("%*d");                         // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  for (int Y = 1; Y < 10; scanf("%*d")) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+  for (int Y = 1; scanf("%*d"); ++Y) {
+  }
+  int Y = 0;
+  for (scanf("%*d"); Y < 10; ++Y) { // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+  }
+
+  std::vector<int> Vec;
+  for (int I : Vec)
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+
+  switch (scanf("%*d")) {
+  }
+  switch (X) {
+  case 1:
+    scanf("%*d"); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+    break;
+  }
+
+  // This call is wrapped in an ExprWithCleanups.
+  scanf("", S().getInt()); // expected-warning {{Return value is not checked in call to 'scanf' [security.UncheckedReturnValue]}}
+
+  int Z1 = scanf("%*d");
+  Z1 = scanf("%*d");
+  extern_f(scanf("%*d"));
+
+  btowc(22); // no warning for non-system function
+
+  return scanf("%*d"); // no warning if value is returned
+}