"ErrorReturn checker" WIP review

[balazske]

Summary

This is a testing version for the checker that is implemented in
D72705. It is extended to make it usable on large projects.

This change it not intended to land, it is here only for
pre-evaluating the code.

[balazske]

Here are the results with this checker on project 'emacs`.
Still more evaluation is needed.

[balazske]

Results for 'duckdb': link

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp

505

This still causes more harm then good, I'd propose to remove it, even for evaluation/debug purposes.

clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp

 //===-- ErrorReturnChecker.cpp ------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 //

 /// Interface and abstraction for various kinds of error return value and way of
 /// checking it. The "test" functions are called if a corresponding construct in
 /// the code is found. The functions should return true the code is found to be
 /// acceptable as error check.
 class ErrorReturnCheckKind {
 public:
   /// Test if an encountered binary operator where the return value is involved
   /// is a valid check statement. The return value appears in one side of the
-  /// operator (`ChildIsLHS` indicates if it is on the LHS). If the other side
-  /// contains a known (mostly constant) value, it is already calculated in
-  /// `KnownValue`. `RetTy` is the type of the return value (return type of the
-  /// function call in the code to check).
+  /// operator (`ChildIsLHS` indicates if it is on the LHS). The other side is
+  /// the value to test against, its "known value" is passed in
+  /// `ValueToTestAgainst. `RetTy` is the return type of the function.
+  // testAppearanceInBinOp
   virtual bool testBinOpForCheckStatement(BasicValueFactory &BVF,
                                           const BinaryOperator *BinOp,
-                                          const llvm::APSInt *KnownValue,
+                                          const llvm::APSInt *ValueToTestAgainst,
                                           QualType RetTy,
                                           bool ChildIsLHS) const = 0;
+
+  virtual bool testUnaryOp(BasicValueFactory &BVF,
+                                          const UnaryOperator *UnOp,
+                                          QualType RetTy) const = 0;
+
+  /// Tell if the bare appearance of the return value in place of a condition (for example 'X' in 'if (X)') can be taken as a check for error return value.
+  // testAppearanceInCondition
+  virtual bool testConditionForCheckStatement() const = 0;
 };
 
 /// Error return is a -1 or any negative value (both is accepted).
 /// More precise, the check for error return value should be comparison to -1
 /// or relational comparison to 0.
 /// This is to be used with signed types only.
 class EOFOrNegativeErrorReturn : public ErrorReturnCheckKind {
 public:
   bool testBinOpForCheckStatement(BasicValueFactory &BVF,
                                   const BinaryOperator *BinOp,
-                                  const llvm::APSInt *KnownValue,
+                                  const llvm::APSInt *ValueToTestAgainst,
                                   QualType RetTy,
                                   bool ChildIsLHS) const override {
-    if (!KnownValue)
-      return false;
+    if (!ValueToTestAgainst)
+      return true;
 
-    bool KnownNull = KnownValue->isNullValue();
-    bool KnownEOF = ((*KnownValue) == BVF.getValue(-1, RetTy));
+    bool NullFound = ValueToTestAgainst->isNullValue();
+    bool EOFFound = ((*ValueToTestAgainst) == BVF.getValue(-1, RetTy));
 
     if (ChildIsLHS) {
       switch (BinOp->getOpcode()) {
       case BO_EQ: // 'X == -1'
       case BO_NE: // 'X != -1'
-        return KnownEOF;
+        return EOFFound;
       case BO_LT: // 'X < 0'
-        return KnownNull;
+        return NullFound;
       case BO_GE: // 'X >= 0'
-        return KnownNull;
+        return NullFound;
       case BO_LE: // 'X <= -1'
-        return KnownEOF;
+        return EOFFound;
       case BO_GT: // 'X > -1'
-        return KnownEOF;
+        return EOFFound;
       default:
         return false;
       }
     } else {
       switch (BinOp->getOpcode()) {
       case BO_EQ: // '-1 == X'
       case BO_NE: // '-1 != X'
-        return KnownEOF;
+        return EOFFound;
       case BO_GT: // '0 > X'
-        return KnownNull;
+        return NullFound;
       case BO_LE: // '0 <= X'
-        return KnownNull;
+        return NullFound;
       case BO_GE: // '-1 >= X'
-        return KnownEOF;
+        return EOFFound;
       case BO_LT: // '-1 < X'
-        return KnownEOF;
+        return EOFFound;
       default:
         return false;
       }
     }
     return false;
   }
+
+  bool testUnaryOp(BasicValueFactory &BVF, const UnaryOperator *UnOp,
+                   QualType RetTy) const override {
+    return false;
+  }
+
+  bool testConditionForCheckStatement() const override { return false; }
+};
+
+class NullErrorReturn : public ErrorReturnCheckKind {
+public:
+  bool testBinOpForCheckStatement(BasicValueFactory &BVF,
+                                  const BinaryOperator *BinOp,
+                                  const llvm::APSInt *ValueToTestAgainst,
+                                  QualType RetTy,
+                                  bool ChildIsLHS) const override {
+    clang::BinaryOperator::Opcode C = BinOp->getOpcode();
+
+    if (C == BO_LAnd || C == BO_LOr)
+      return true;
+
+    if (!ValueToTestAgainst)
+      return false;
+    bool NullFound = ValueToTestAgainst->isNullValue();
+
+    if (C == BO_EQ || C == BO_NE)
+      return NullFound;
+    
+    /*if (ChildIsLHS) {
+      switch (BinOp->getOpcode()) {
+      case BO_EQ: // 'X == 0'
+      case BO_NE: // 'X != 0'
+        return KnownNull;
+      default:
+        return false;
+      }
+    } else {
+      switch (BinOp->getOpcode()) {
+      case BO_EQ: // '0 == X'
+      case BO_NE: // '0 != X'
+        return KnownNull;
+      default:
+        return false;
+      }
+    }*/
+    return false;
+  }
+
+  bool testUnaryOp(BasicValueFactory &BVF, const UnaryOperator *UnOp,
+                   QualType RetTy) const override {
+    if (UnOp->getOpcode() == UnaryOperator::Opcode::UO_LNot)
+      return true;
+    return false;
+  }
+
+  bool testConditionForCheckStatement() const override { return true; }
 };
 
 /// Description of an API function to check.
 struct FnInfo {
   /// Error return check kind for the function.
   ErrorReturnCheckKind *ErrorReturnKind;
 
   /// Is an unchecked use of return value error.
struct CalledFunctionData {
   }
 };
 
 class ErrorReturnChecker
     : public Checker<check::PostCall, check::Location,
                      check::PointerEscape> {
   mutable std::unique_ptr<BuiltinBug> BT_UncheckedUse;
 
-  void checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS,
+  ExplodedNode *checkAccess(CheckerContext &C, ProgramStateRef State, const Stmt *LoadS,
                    SymbolRef CallSym, const CalledFunctionData *CFD) const;
   ProgramStateRef processEscapedParams(CheckerContext &C, const CallEvent &Call,
                                        ProgramStateRef State) const;
   const FnInfo *findFunctionToCheck(CheckerContext &C,
                                     const CallEvent &Call) const;
 
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkLocation(SVal L, bool IsLoad, const Stmt *S,
                      CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const;
 
 private:
   EOFOrNegativeErrorReturn CheckForEOFOrNegative;
+  NullErrorReturn CheckForNull;
+  NullErrorReturn CheckForZero;
 
   CallDescriptionMap<FnInfo> CheckedFunctions = {
       {{"fputs", 2}, FnInfo{&CheckForEOFOrNegative}},
       {{"fputws", 2}, FnInfo{&CheckForEOFOrNegative}},
+
+      {{"aligned_alloc", 2}, FnInfo{&CheckForNull}},
+      {{"bsearch", 5}, FnInfo{&CheckForNull}},
+      {{"bsearch_s", 6}, FnInfo{&CheckForNull}},
+      {{"calloc", 2}, FnInfo{&CheckForNull}},
+      {{"fgets", 3}, FnInfo{&CheckForNull}},
+      {{"fopen", 2}, FnInfo{&CheckForNull}},
+      {{"freopen", 3}, FnInfo{&CheckForNull}},
+      {{"getenv", 1}, FnInfo{&CheckForNull}},
+      {{"getenv_s", 4}, FnInfo{&CheckForNull}},
+      {{"gets_s", 2}, FnInfo{&CheckForNull}},
+      {{"gmtime", 1}, FnInfo{&CheckForNull}},
+      {{"gmtime_s", 2}, FnInfo{&CheckForNull}},
+      {{"localtime", 1}, FnInfo{&CheckForNull}},
+      {{"localtime_s", 2}, FnInfo{&CheckForNull}},
+      {{"malloc", 1}, FnInfo{&CheckForNull}},
+      {{"memchr", 3}, FnInfo{&CheckForNull}},
+      {{"realloc", 2}, FnInfo{&CheckForNull}},
+      {{"setlocale", 2}, FnInfo{&CheckForNull}},
+      {{"strchr", 2}, FnInfo{&CheckForNull}},
+      {{"strpbrk", 2}, FnInfo{&CheckForNull}},
+      {{"strrchr", 2}, FnInfo{&CheckForNull}},
+      {{"strstr", 2}, FnInfo{&CheckForNull}},
+      {{"strtok", 2}, FnInfo{&CheckForNull}},
+      {{"strtok_s", 4}, FnInfo{&CheckForNull}},
+      {{"tmpfile", 0}, FnInfo{&CheckForNull}},
+      {{"tmpnam", 1}, FnInfo{&CheckForNull}},
+      {{"wcschr", 2}, FnInfo{&CheckForNull}},
+      {{"wcspbrk", 2}, FnInfo{&CheckForNull}},
+      {{"wcsrchr", 2}, FnInfo{&CheckForNull}},
+      {{"wcsstr", 2}, FnInfo{&CheckForNull}},
+      {{"wcstok", 3}, FnInfo{&CheckForNull}},
+      {{"wcstok_s", 4}, FnInfo{&CheckForNull}},
+      {{"wmemchr", 3}, FnInfo{&CheckForNull}},
+
+      {{"asctime_s", 3}, FnInfo{&CheckForZero}},
+      {{"at_quick_exit", 1}, FnInfo{&CheckForZero}},
+      {{"atexit", 1}, FnInfo{&CheckForZero}},
+      {{"ctime_s", 3}, FnInfo{&CheckForZero}},
+      {{"fgetpos", 2}, FnInfo{&CheckForZero}},
+      {{"fopen_s", 3}, FnInfo{&CheckForZero}},
+      {{"freopen_s", 4}, FnInfo{&CheckForZero}},
+      {{"fsetpos", 2}, FnInfo{&CheckForZero}},
+      {{"raise", 1}, FnInfo{&CheckForZero}},
+      {{"remove", 1}, FnInfo{&CheckForZero}},
+      {{"rename", 2}, FnInfo{&CheckForZero}},
+      {{"setvbuf", 4}, FnInfo{&CheckForZero}},
+      {{"strerror_s", 3}, FnInfo{&CheckForZero}},
+      {{"strftime", 4}, FnInfo{&CheckForZero}},
+      {{"timespec_get", 2}, FnInfo{&CheckForZero}},
+      {{"tmpfile_s", 1}, FnInfo{&CheckForZero}},
+      {{"tmpnam_s", 2}, FnInfo{&CheckForZero}},
+      {{"wctrans", 1}, FnInfo{&CheckForZero}},
+      {{"wctype", 1}, FnInfo{&CheckForZero}}
   };
+
 };
 
 /// Result of the ErrorCheckTestStmtVisitor.
 enum VisitResult {
   // Value use found that is not an error check.
   NoCheckUseFound,
   // An error check was found.
   CheckFound,
if (CFD->Info->ErrorReturnKind->testBinOpForCheckStatement(
             C.getSValBuilder().getBasicValueFactory(), BO,
             getKnownConstantVal(OtherS), CFD->Info->RetTy,
             OtherS == BO->getRHS()))
       return CheckFound;
     // Value appears in binary operator in other way than error check.
     return NoCheckUseFound;
   }
 
+  VisitResult VisitUnaryOperator(const UnaryOperator *UO) {
+    if (CFD->Info->ErrorReturnKind->testUnaryOp(
+            C.getSValBuilder().getBasicValueFactory(), UO,
+            CFD->Info->RetTy))
+      return CheckFound;
+    return NoCheckUseFound;
+  }
+
+  VisitResult VisitAbstractConditionalOperator(const AbstractConditionalOperator *ACO) {
+    if (isChildOf(ACO->getCond()))
+      return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound;
+    return ContinueAtParentStmt;
+  }
+
   VisitResult VisitCallExpr(const CallExpr *CE) {
     const FunctionDecl *CalledF = C.getCalleeDecl(CE);
     SourceLocation Loc = CalledF->getLocation();
     // Check if system function is called.
     // It is assumed that any system function does not accept value that is
     // an error return value from any other function.
     // In other words error check is required before system function is called
     // with the value.
public:
 
   VisitResult VisitDeclStmt(const DeclStmt *Decl) {
     // Value is used at initialization.
     return StopExamineNoError;
   }
 
   VisitResult VisitIfStmt(const IfStmt *S) {
     if (isChildOf(S->getCond()))
-      return NoCheckUseFound;
+      return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound;
     return StopExamineNoError;
   }
 
   VisitResult VisitForStmt(const ForStmt *S) {
-    if (isChildOf(S->getInit()) || isChildOf(S->getCond()) ||
-        isChildOf(S->getInc()))
-      return NoCheckUseFound;
+//    if (isChildOf(S->getInit()) || isChildOf(S->getInc()))
+//      return 
+    if (isChildOf(S->getCond()))
+      return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound;
     return StopExamineNoError;
   }
 
   VisitResult VisitDoStmt(const DoStmt *S) {
     if (isChildOf(S->getCond()))
-      return NoCheckUseFound;
+      return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound;
     return StopExamineNoError;
   }
 
   VisitResult VisitWhileStmt(const WhileStmt *S) {
     if (isChildOf(S->getCond()))
-      return NoCheckUseFound;
+      return CFD->Info->ErrorReturnKind->testConditionForCheckStatement() ? CheckFound : NoCheckUseFound;
     return StopExamineNoError;
   }
 
   // Value appears in other statement.
   // FIXME: Statements that affects control-flow should be checked separately.
   // For example `Child` may appear as a condition of `if`.
   VisitResult VisitStmt(const Stmt *S) { return StopExamineNoError; }
 };
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(CalledFunctionDataMap, SymbolRef,
                                CalledFunctionData)
 
-void ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State,
+ExplodedNode *ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State,
                                      const Stmt *LoadS, SymbolRef CallSym,
                                      const CalledFunctionData *CFD) const {
   const ParentMap &PM = C.getLocationContext()->getParentMap();
   //llvm::errs()<<"LoadS\n";
   //LoadS->dumpColor();
 
   while (LoadS) {
     const Stmt *ParentS = PM.getParentIgnoreParenCasts(LoadS);
     //llvm::errs()<<"ParentS\n";
     //ParentS->dumpColor();
 
     ErrorCheckTestStmtVisitor FindErrorCheck{C, LoadS, CFD};
     switch (FindErrorCheck.Visit(ParentS)) {
     case NoCheckUseFound: {
       if (!BT_UncheckedUse)
         BT_UncheckedUse.reset(
             new BuiltinBug(this, "Use of unchecked return value",
                           "Use of return value that was not checked for error"));
 
       SourceRange CallLocation = CFD->CallLocation;
       State = State->remove<CalledFunctionDataMap>(CallSym);
 
-      ExplodedNode *N = C.generateNonFatalErrorNode(State);
-      if (!N) {
-        C.addTransition(State);
-        return;
-      }
+      ExplodedNode *ErrN = C.generateNonFatalErrorNode(State);
+      if (!ErrN)
+        return C.addTransition(State);
 
       auto Report = std::make_unique<PathSensitiveBugReport>(
-          *BT_UncheckedUse, BT_UncheckedUse->getDescription(), N);
-      // Report->markInteresting(CallSym);
+          *BT_UncheckedUse, BT_UncheckedUse->getDescription(), ErrN);
+      Report->markInteresting(CallSym);
       Report->addRange(CallLocation);
       C.emitReport(std::move(Report));
       
       //auto Report = std::make_unique<BasicBugReport>(*BT_UncheckedUse, BT_UncheckedUse->getDescription(), PathDiagnosticLocation{LoadS, C.getSourceManager(), C.getLocationContext()});
       //Report->addRange(CallLocation);
       //C.emitReport(std::move(Report));
       
-      return;
+      return nullptr;
     }
     case CheckFound:
       // A correct error check was found, remove from state.
       State = State->remove<CalledFunctionDataMap>(CallSym);
-      C.addTransition(State);
-      return;
+      return C.addTransition(State);
 
     case ContinueAtParentStmt:
       // Continue checking at upper level (check with result of assignment).
       LoadS = ParentS;
       continue;
 
     case StopExamineNoError:
-      C.addTransition(State);
-      return;
+      return C.addTransition(State);
     };
   }
+  return C.addTransition(State);
 }
 
 ProgramStateRef ErrorReturnChecker::processEscapedParams(
     CheckerContext &C, const CallEvent &Call, ProgramStateRef State) const {
   for (unsigned int I = 0, E = Call.getNumArgs(); I < E; ++I) {
     SVal V = Call.getArgSVal(I);
     SymbolRef Sym = V.getAsSymbol();
     if (Sym) {
ErrorReturnChecker::findFunctionToCheck(CheckerContext &C,
 
   // Lazy-init the return type when the function is found.
   if (Fn->RetTy.isNull())
     Fn->RetTy = FD->getReturnType();
 
   return Fn;
 }
 
+  struct NoteFn {
+    const CheckerNameRef CheckerName;
+    SymbolRef StreamSym;
+    std::string Message;
+
+    std::string operator()(PathSensitiveBugReport &BR) const {
+      llvm::errs()<<"NoteFn\n";
+      if (BR.isInteresting(StreamSym) &&
+          CheckerName == BR.getBugType().getCheckerName())
+        return Message;
+
+      return "";
+    }
+  };
+
 void ErrorReturnChecker::checkPostCall(const CallEvent &Call,
                                        CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   State = processEscapedParams(C, Call, State);
 
   const FnInfo *Fn = findFunctionToCheck(C, Call);
   if (!Fn) {
     C.addTransition(State);
     return;
   }
 
   SymbolRef RetSym = Call.getReturnValue().getAsSymbol();
 
   CalledFunctionData CFD{Fn, Call.getSourceRange()};
   State = State->set<CalledFunctionDataMap>(RetSym, CFD);
 
-  checkAccess(C, State, Call.getOriginExpr(), RetSym, &CFD);
+  //SmallString<100> buf;
+  //llvm::raw_svector_ostream os(buf);
+  //os << "Function \'???\' called here without checked return value";
+  ExplodedNode *N = checkAccess(C, State, Call.getOriginExpr(), RetSym, &CFD);
+  if (!N)
+    return;
+  
+  C.addTransition(N->getState(), C.getNoteTag(NoteFn{getCheckerName(), RetSym, "Function \'???\' called here without checked return value"}, N));
 }
 
 void ErrorReturnChecker::checkLocation(SVal L, bool IsLoad, const Stmt *S,
                                        CheckerContext &C) const {
   if (!IsLoad)
     return;
   if (L.isUnknownOrUndef())
     return;

clang/test/Analysis/error-return.c

 void test_EOFOrNeg_VarAssign1() {
   int X = fputs("str", file());
   int Y = X;
   if (Y != 0) { // expected-warning{{Use of return value that was not checked}}
   }
 }
 
 void badcheck(int X) {
-  if (X == 0) { } // expected-warning{{Use of return value that was not checked}}
+  if (X == 0) {
+  } // expected-warning{{Use of return value that was not checked}}
 }
 
 void test_EOFOrNeg_Call() {
   int X = fputs("str", file());
   badcheck(X);
 }
 
 void test_EOFOrNeg_Syscall() {