[analyzer] [WIP] Checker for pointers-written-into and then not read from.

[george.karpenkov]

Summary

Emits a warning if a called function writes into a (heuristically determined) error out-parameter, and then the caller does not read from it.

Work-in-progress dump of the code I had previously written,
so that it does not get lost.

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

 def DynamicTypeChecker : Checker<"DynamicTypeChecker">,
   HelpText<"Check for cases where the dynamic and the static type of an object are unrelated.">,
   DescFile<"DynamicTypeChecker.cpp">;
 
 def StackAddrAsyncEscapeChecker : Checker<"StackAddressAsyncEscape">,
   HelpText<"Check that addresses to stack memory do not escape the function">,
   DescFile<"StackAddrEscapeChecker.cpp">;
 
+def PointerParamIgnoredChecker : Checker<"PointerParamIgnored">,
+  HelpText<"Check that the pointer written into inside the function is not ignored">,
+  DescFile<"PointerParamIgnoredChecker.cpp">;
+
 } // end "alpha.core"
 
 let ParentPackage = Nullability in {
 
 def NullPassedToNonnullChecker : Checker<"NullPassedToNonnull">,
   HelpText<"Warns when a null pointer is passed to a pointer which has a _Nonnull type.">,
   DescFile<"NullabilityChecker.cpp">;
 

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCPropertyChecker.cpp
   ObjCSelfInitChecker.cpp
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   PaddingChecker.cpp
   PointerArithChecker.cpp
+  PointerParamIgnoredChecker.cpp
   PointerSubChecker.cpp
   PthreadLockChecker.cpp
   RetainCountChecker.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
   SimpleStreamChecker.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp

clang/lib/StaticAnalyzer/Checkers/PointerParamIgnoredChecker.cpp

+//==- PointerParamIgnoredChecker.cpp ------------------------------*- C++ -*--//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Matches:
+//
+// variable set to non-zero
+//   -> passed to a function with the name “error” or similar
+//   -> not written into
+//   -> not checked
+//
+//
+// variable set to zero
+//   -> passed to a function with the name “error” or similar
+//   -> written into
+//   -> not checked
+//
+//===----------------------------------------------------------------------===//
+//
+// TODO: can do checkBranch condition
+// TODO: OK how do I match condition
+
+#include "ClangSACheckers.h"
+#include "llvm/ADT/FoldingSet.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+const char* ERR_PARAMETER_MATCHER = "err";
+
+struct PassedPtrInfo {
+  /// Initial value written into.
+  llvm::APSInt ValueWrittenInto;
+
+  /// Store the stack on where the first call with this location was
+  /// registered. 
+  const StackFrameContext* StackOriginated;
+
+  bool operator==(const PassedPtrInfo &Other) const {
+    return ValueWrittenInto == Other.ValueWrittenInto
+      && StackOriginated == Other.StackOriginated;
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.Add(ValueWrittenInto);
+    ID.Add(StackOriginated);
+  }
+
+  PassedPtrInfo(llvm::APSInt V, const StackFrameContext *StackOriginated)
+      : ValueWrittenInto(V), StackOriginated(StackOriginated) {}
+};
+
+typedef llvm::ImmutableMap<const MemRegion *, PassedPtrInfo> ErrorOutFlag;
+
+} // end anonymous namespace
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ParamInfo, ErrorOutFlag)
+
+namespace {
+
+class PointerParamIgnoredChecker : public Checker<check::Location,
+                                                  check::PreCall,
+                                                  check::PostCall,
+                                                  check::DeadSymbols> {
+private:
+  std::unique_ptr<BugType> PointerIgnoredBugType;
+public:
+  PointerParamIgnoredChecker()
+      : PointerIgnoredBugType(llvm::make_unique<BugType>(
+            this, "Error pointer not checked", "Security")) {}
+
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+    ProgramStateRef State = C.getState();
+    const LocationContext* LC = C.getLocationContext();
+    for (unsigned i=0; i<Call.getNumArgs(); i++) {
+      SVal P = Call.getArgSVal(i);
+      ParmVarDecl *PVD = Call.parameters()[i];
+      StringRef ParamName = PVD->getName();
+      if (!ParamName.contains_lower(ERR_PARAMETER_MATCHER))
+        continue;
+
+      if (auto L = P.getAs<Loc>()) {
+        SVal D = State->getSVal(*L);
+
+        // Note: do not re-add already stored parameter information.
+        if (State->get<ParamInfo>(L->getAsRegion()))
+          continue;
+
+        if (auto N = D.getAs<nonloc::ConcreteInt>()) {
+          llvm::APSInt Val = N->getValue();
+
+          State = State->set<ParamInfo>(
+              L->getAsRegion(), PassedPtrInfo(Val, LC->getCurrentStackFrame()));
+        }
+      }
+    }
+
+    C.addTransition(State);
+
+  }
+
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+    ProgramStateRef State = C.getState();
+    for (unsigned i=0; i<Call.getNumArgs(); i++) {
+      // TODO: refactor the duplication in the block below.
+      SVal P = Call.getArgSVal(i);
+      ParmVarDecl *PVD = Call.parameters()[i];
+      StringRef ParamName = PVD->getName();
+      if (!ParamName.contains_lower(ERR_PARAMETER_MATCHER))
+        continue;
+
+      if (auto L = P.getAs<Loc>()) {
+        const PassedPtrInfo* Data = State->get<ParamInfo>(L->getAsRegion());
+        if (!Data)
+          continue;
+
+        // check that the value has changed.
+        SVal D = State->getSVal(*L);
+        auto N = D.getAs<nonloc::ConcreteInt>();
+        // Value is no longer concrete or has definitely changed.
+        if (!N || N->getValue() != Data->ValueWrittenInto)
+          State = State->remove<ParamInfo>(L->getAsRegion());
+      }
+    }
+    C.addTransition(State);
+  }
+
+  void checkLocation(SVal P, bool IsLoad, const Stmt *S,
+                     CheckerContext &C) const {
+    const LocationContext* LC = C.getLocationContext();
+    const StackFrameContext* SC = LC->getCurrentStackFrame();
+    ProgramStateRef State = C.getState();
+    if (!IsLoad)
+      return;
+    auto L = P.getAs<Loc>();
+    if (!L)
+      return;
+
+    const PassedPtrInfo* Info = State->get<ParamInfo>(L->getAsRegion());
+    if (!Info)
+      return;
+
+    // Value was actually loaded into somewhere!
+    if (Info->StackOriginated == SC)
+      State = State->remove<ParamInfo>(L->getAsRegion());
+
+    C.addTransition(State);
+  }
+
+
+  void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const {
+    ProgramStateRef State = C.getState();
+    BugReporter &BR = C.getBugReporter();
+    Optional<std::string> ErrMsg;
+    for (auto P : State->get<ParamInfo>()) {
+      const MemRegion *MR = P.first;
+      PassedPtrInfo Info = P.second;
+
+      if (!SymReaper.isLiveRegion(MR)) {
+
+        ErrMsg = "Error pointer not checked";
+        State = State->remove<ParamInfo>(MR);
+        return;
+      }
+    }
+
+    if (ErrMsg) {
+        ExplodedNode *N = C.generateNonFatalErrorNode(State);
+        if (!N)
+          return;
+
+        // TODO: a better results messages, handle multiple errors,
+        // variable declaration, etc.
+        BR.emitReport(llvm::make_unique<BugReport>(
+            *PointerIgnoredBugType, "Error pointer not checked", N));
+    } else {
+
+      C.addTransition(State);
+    }
+  }
+
+  // Remove invalidated regions.
+  ProgramStateRef
+  checkRegionChanges(ProgramStateRef State,
+                     const InvalidatedSymbols *Invalidated,
+                     ArrayRef<const MemRegion *> ExplicitRegions,
+                     ArrayRef<const MemRegion *> Regions,
+                     const LocationContext *LCtx, const CallEvent *Call) const {
+    for (const MemRegion *InvalidatedRegion : Regions) {
+      State = State->remove<ParamInfo>(InvalidatedRegion);
+    }
+    return State;
+  }
+
+};
+
+} // namespace
+
+void ento::registerPointerParamIgnoredChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<PointerParamIgnoredChecker>();
+}

clang/test/Analysis/pointerparamignoredchecker-test.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.PointerParamIgnored -verify %s
+
+int errpassed(int *err) {
+  *err = 0;
+  return 1;
+}
+
+int foo() {
+  int z = 0;
+  errpassed(&z);
+  return 1; 
+} // expected-warning{{Error pointer not checked}}
+