Merge pull request #3681 from haoNoQ/static-analyzer-cherrypicks-24

Static analyzer cherrypicks 24

Author: haoNoQ

clang/cmake/modules/AddClang.cmake

@@ -100,7 +100,12 @@ macro(add_clang_library name)
       # The Xcode generator doesn't handle object libraries correctly.
       list(APPEND LIBTYPE OBJECT)
     endif()
-    set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
+    if (NOT EXCLUDE_FROM_ALL)
+      # Only include libraries that don't have EXCLUDE_FROM_ALL set. This
+      # ensure that the clang static analyzer libraries are not compiled
+      # as part of clang-shlib if CLANG_ENABLE_STATIC_ANALYZER=OFF.
+      set_property(GLOBAL APPEND PROPERTY CLANG_STATIC_LIBS ${name})
+    endif()
   endif()
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
@@ -110,8 +115,11 @@ macro(add_clang_library name)
   endif()
 
   foreach(lib ${libs})
-    if(TARGET ${lib})
+   if(TARGET ${lib})
       target_link_libraries(${lib} INTERFACE ${LLVM_COMMON_LIBS})
+      if (EXCLUDE_FROM_ALL)
+        continue()
+      endif()
 
       if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ARG_INSTALL_WITH_TOOLCHAIN)
         get_target_export_arg(${name} Clang export_to_clangtargets UMBRELLA clang-libraries)

clang/docs/analyzer/checkers.rst

@@ -2074,10 +2074,63 @@ Finds calls to the ``putenv`` function which pass a pointer to an automatic vari
     if (retval < 0 || (size_t)retval >= sizeof(env)) {
         /* Handle error */
     }
- 
+
     return putenv(env); // putenv function should not be called with auto variables
   }
-  
+
+alpha.security.cert.env
+^^^^^^^^^^^^^^^^^^^^^^^
+
+SEI CERT checkers of `POSIX C coding rules <https://wiki.sei.cmu.edu/confluence/x/JdcxBQ>`_.
+
+.. _alpha-security-cert-env-InvalidPtr:
+
+alpha.security.cert.env.InvalidPtr
+"""""""""""""""""""""""""""
+
+Corresponds to SEI CERT Rules ENV31-C and ENV34-C.
+
+ENV31-C:
+Rule is about the possible problem with `main` function's third argument, environment pointer,
+"envp". When enviornment array is modified using some modification function
+such as putenv, setenv or others, It may happen that memory is reallocated,
+however "envp" is not updated to reflect the changes and points to old memory
+region.
+
+ENV34-C:
+Some functions return a pointer to a statically allocated buffer.
+Consequently, subsequent call of these functions will invalidate previous
+pointer. These functions include: getenv, localeconv, asctime, setlocale, strerror
+
+.. code-block:: c
+
+  int main(int argc, const char *argv[], const char *envp[]) {
+    if (setenv("MY_NEW_VAR", "new_value", 1) != 0) {
+      // setenv call may invalidate 'envp'
+      /* Handle error */
+    }
+    if (envp != NULL) {
+      for (size_t i = 0; envp[i] != NULL; ++i) {
+        puts(envp[i]);
+        // envp may no longer point to the current environment
+        // this program has unanticipated behavior, since envp
+        // does not reflect changes made by setenv function.
+      }
+    }
+    return 0;
+  }
+
+  void previous_call_invalidation() {
+    char *p, *pp;
+
+    p = getenv("VAR");
+    pp = getenv("VAR2");
+    // subsequent call to 'getenv' invalidated previous one
+
+    *p;
+    // dereferencing invalid pointer
+  }
+
 .. _alpha-security-ArrayBound:
 
 alpha.security.ArrayBound (C)

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

@@ -73,6 +73,7 @@ def Taint : Package<"taint">, ParentPackage<SecurityAlpha>;
 
 def CERT : Package<"cert">, ParentPackage<SecurityAlpha>;
 def POS : Package<"pos">, ParentPackage<CERT>;
+def ENV : Package<"env">, ParentPackage<CERT>;
 
 def Unix : Package<"unix">;
 def UnixAlpha : Package<"unix">, ParentPackage<Alpha>;
@@ -485,7 +486,17 @@ def DynamicMemoryModeling: Checker<"DynamicMemoryModeling">,
                   "allocating and deallocating functions are annotated with "
                   "ownership_holds, ownership_takes and ownership_returns.",
                   "false",
-                  InAlpha>
+                  InAlpha>,
+    CmdLineOption<Boolean,
+                  "AddNoOwnershipChangeNotes",
+                  "Add an additional note to the bug report for leak-like "
+                  "bugs. Dynamically allocated objects passed to functions "
+                  "that neither deallocated it, or have taken responsibility "
+                  "of the ownership are noted, similarly to "
+                  "NoStoreFuncVisitor.",
+                  "true",
+                  Released,
+                  Hide>
   ]>,
   Dependencies<[CStringModeling]>,
   Documentation<NotDocumented>,
@@ -937,6 +948,14 @@ let ParentPackage = POS in {
 
 } // end "alpha.cert.pos"
 
+let ParentPackage = ENV in {
+
+  def InvalidPtrChecker : Checker<"InvalidPtr">,
+  HelpText<"Finds usages of possibly invalidated pointers">,
+  Documentation<HasDocumentation>;
+
+} // end "alpha.cert.env"
+
 let ParentPackage = SecurityAlpha in {
 
 def ArrayBoundChecker : Checker<"ArrayBound">,

clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def

@@ -320,6 +320,22 @@ ANALYZER_OPTION(bool, ShouldDisplayCheckerNameForText, "display-checker-name",
                 "Display the checker name for textual outputs",
                 true)
 
+ANALYZER_OPTION(
+    bool, ShouldConsiderSingleElementArraysAsFlexibleArrayMembers,
+    "consider-single-element-arrays-as-flexible-array-members",
+    "Consider single element arrays as flexible array member candidates. "
+    "This will prevent the analyzer from assuming that a single element array "
+    "holds a single element.",
+    false)
+
+ANALYZER_OPTION(
+    bool, ShouldAssumeControlledEnvironment, "assume-controlled-environment",
+    "Whether the analyzed application runs in a controlled environment. "
+    "We will assume that environment variables exist in queries and they hold "
+    "no malicious data. For instance, if this option is enabled, 'getenv()' "
+    "might be modeled by the analyzer to never return NULL.",
+    false)
+
 //===----------------------------------------------------------------------===//
 // Unsigned analyzer options.
 //===----------------------------------------------------------------------===//

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

@@ -21,6 +21,7 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include <list>
 #include <memory>
@@ -622,8 +623,118 @@ class TagVisitor : public BugReporterVisitor {
                                    PathSensitiveBugReport &R) override;
 };
 
-} // namespace ento
+class ObjCMethodCall;
+class CXXConstructorCall;
+
+/// Put a diagnostic on return statement (or on } in its absence) of all inlined
+/// functions for which some property remained unchanged.
+/// Resulting diagnostics may read such as "Returning without writing to X".
+///
+/// Descendants can define what a "state change is", like a change of value
+/// to a memory region, liveness, etc. For function calls where the state did
+/// not change as defined, a custom note may be constructed.
+///
+/// For a minimal example, check out
+/// clang/unittests/StaticAnalyzer/NoStateChangeFuncVisitorTest.cpp.
+class NoStateChangeFuncVisitor : public BugReporterVisitor {
+private:
+  /// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
+  /// This visitor generates a note only if a function does *not* change the
+  /// state that way. This information is not immediately available
+  /// by looking at the node associated with the exit from the function
+  /// (usually the return statement). To avoid recomputing the same information
+  /// many times (going up the path for each node and checking whether the
+  /// region was written into) we instead lazily compute the stack frames
+  /// along the path.
+  // TODO: Can't we just use a map instead? This is likely not as cheap as it
+  // makes the code difficult to read.
+  llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
+  llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
+
+  /// Check and lazily calculate whether the state is modified in the stack
+  /// frame to which \p CallExitBeginN belongs.
+  /// The calculation is cached in FramesModifying.
+  bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
+
+  void markFrameAsModifying(const StackFrameContext *SCtx);
+
+  /// Write to \c FramesModifying all stack frames along the path in the current
+  /// stack frame which modifies the state.
+  void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
 
+protected:
+  bugreporter::TrackingKind TKind;
+
+  /// \return Whether the state was modified from the current node, \p CurrN, to
+  /// the end of the stack frame, at \p CallExitBeginN. \p CurrN and
+  /// \p CallExitBeginN are always in the same stack frame.
+  /// Clients should override this callback when a state change is important
+  /// not only on the entire function call, but inside of it as well.
+  /// Example: we may want to leave a note about the lack of locking/unlocking
+  /// on a particular mutex, but not if inside the function its state was
+  /// changed, but also restored. wasModifiedInFunction() wouldn't know of this
+  /// change.
+  virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+                                         const ExplodedNode *CallExitBeginN) {
+    return false;
+  }
+
+  /// \return Whether the state was modified in the inlined function call in
+  /// between \p CallEnterN and \p CallExitEndN. Mind that the stack frame
+  /// retrieved from a CallEnterN and CallExitEndN is the *caller's* stack
+  /// frame! The inlined function's stack should be retrieved from either the
+  /// immediate successor to \p CallEnterN or immediate predecessor to
+  /// \p CallExitEndN.
+  /// Clients should override this function if a state changes local to the
+  /// inlined function are not interesting, only the change occuring as a
+  /// result of it.
+  /// Example: we want to leave a not about a leaked resource object not being
+  /// deallocated / its ownership changed inside a function, and we don't care
+  /// if it was assigned to a local variable (its change in ownership is
+  /// inconsequential).
+  virtual bool wasModifiedInFunction(const ExplodedNode *CallEnterN,
+                                     const ExplodedNode *CallExitEndN) {
+    return false;
+  }
+
+  /// Consume the information on the non-modifying stack frame in order to
+  /// either emit a note or not. May suppress the report entirely.
+  /// \return Diagnostics piece for the unmodified state in the current
+  /// function, if it decides to emit one. A good description might start with
+  /// "Returning without...".
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+                           const ObjCMethodCall &Call,
+                           const ExplodedNode *N) = 0;
+
+  /// Consume the information on the non-modifying stack frame in order to
+  /// either emit a note or not. May suppress the report entirely.
+  /// \return Diagnostics piece for the unmodified state in the current
+  /// function, if it decides to emit one. A good description might start with
+  /// "Returning without...".
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+                          const CXXConstructorCall &Call,
+                          const ExplodedNode *N) = 0;
+
+  /// Consume the information on the non-modifying stack frame in order to
+  /// either emit a note or not. May suppress the report entirely.
+  /// \return Diagnostics piece for the unmodified state in the current
+  /// function, if it decides to emit one. A good description might start with
+  /// "Returning without...".
+  virtual PathDiagnosticPieceRef
+  maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+                             const ExplodedNode *N) = 0;
+
+public:
+  NoStateChangeFuncVisitor(bugreporter::TrackingKind TKind) : TKind(TKind) {}
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                   BugReporterContext &BR,
+                                   PathSensitiveBugReport &R) override final;
+};
+
+} // namespace ento
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_BUGREPORTERVISITORS_H

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

@@ -48,6 +48,7 @@ class Range {
     ID.AddPointer(&To());
   }
   void dump(raw_ostream &OS) const;
+  void dump() const;
 
   // In order to keep non-overlapping ranges sorted, we can compare only From
   // points.
@@ -282,6 +283,7 @@ class RangeSet {
   bool contains(llvm::APSInt Point) const { return containsImpl(Point); }
 
   void dump(raw_ostream &OS) const;
+  void dump() const;
 
   bool operator==(const RangeSet &Other) const { return *Impl == *Other.Impl; }
   bool operator!=(const RangeSet &Other) const { return !(*this == Other); }

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

@@ -33,6 +33,7 @@
 
 namespace clang {
 
+class AnalyzerOptions;
 class BlockDecl;
 class CXXBoolLiteralExpr;
 class CXXMethodDecl;
@@ -66,6 +67,8 @@ class SValBuilder {
 
   ProgramStateManager &StateMgr;
 
+  const AnalyzerOptions &AnOpts;
+
   /// The scalar type to use for array indices.
   const QualType ArrayIndexTy;
 
@@ -96,11 +99,7 @@ class SValBuilder {
 
 public:
   SValBuilder(llvm::BumpPtrAllocator &alloc, ASTContext &context,
-              ProgramStateManager &stateMgr)
-      : Context(context), BasicVals(context, alloc),
-        SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-        StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
-        ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
+              ProgramStateManager &stateMgr);
 
   virtual ~SValBuilder() = default;
 
@@ -188,6 +187,8 @@ class SValBuilder {
   MemRegionManager &getRegionManager() { return MemMgr; }
   const MemRegionManager &getRegionManager() const { return MemMgr; }
 
+  const AnalyzerOptions &getAnalyzerOptions() const { return AnOpts; }
+
   // Forwarding methods to SymbolManager.
 
   const SymbolConjured* conjureSymbol(const Stmt *stmt,

clang/lib/Sema/SemaStmt.cpp

@@ -3489,7 +3489,8 @@ VerifyInitializationSequenceCXX98(const Sema &S,
 ExprResult Sema::PerformMoveOrCopyInitialization(
     const InitializedEntity &Entity, const NamedReturnInfo &NRInfo, Expr *Value,
     bool SupressSimplerImplicitMoves) {
-  if ((!getLangOpts().CPlusPlus2b || SupressSimplerImplicitMoves) &&
+  if (getLangOpts().CPlusPlus &&
+      (!getLangOpts().CPlusPlus2b || SupressSimplerImplicitMoves) &&
       NRInfo.isMoveEligible()) {
     ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                               CK_NoOp, Value, VK_XValue, FPOptionsOverride());

clang/lib/StaticAnalyzer/CMakeLists.txt

@@ -1,3 +1,10 @@
+# These directories can significantly impact build time, only build
+# them if anything depends on the clangStaticAnalyzer* libraries.
+if(NOT CLANG_ENABLE_STATIC_ANALYZER)
+  set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)
+  set(EXCLUDE_FROM_ALL ON)
+endif()
+
 add_subdirectory(Core)
 add_subdirectory(Checkers)
 add_subdirectory(Frontend)

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -49,6 +49,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   IdenticalExprChecker.cpp
   InnerPointerChecker.cpp
   InvalidatedIteratorChecker.cpp
+  cert/InvalidPtrChecker.cpp
   Iterator.cpp
   IteratorModeling.cpp
   IteratorRangeChecker.cpp