[clang] [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker (PR #67663)

Endre Fülöp via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 3 23:43:00 PDT 2023


https://github.com/gamesh411 updated https://github.com/llvm/llvm-project/pull/67663

>From 9aea93ddeb70245a07984188aa98577d54e8e560 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Fri, 8 Sep 2023 14:20:00 +0200
Subject: [PATCH 1/8] [analyzer][clangsa] Add new option to
 alpha.security.cert.InvalidPtrChecker

The invalidation of pointer pointers returned by subsequent calls to genenv is
suggested by the POSIX standard, but is too strict from a practical point of
view. A new checker option 'InvalidatingGetEnv' is introduced, and is set to a
more lax default value, which does not consider consecutive getenv calls
invalidating.
The handling of the main function's possible specification where an environment
pointer is also pecified as a third parameter is also considered now.

Differential Revision: https://reviews.llvm.org/D154603
---
 .../clang/StaticAnalyzer/Checkers/Checkers.td |  9 ++
 .../Checkers/cert/InvalidPtrChecker.cpp       | 86 ++++++++++++++-----
 clang/test/Analysis/analyzer-config.c         |  1 +
 .../Analysis/cert/env34-c-cert-examples.c     | 40 ++++++++-
 clang/test/Analysis/cert/env34-c.c            |  1 +
 clang/test/Analysis/invalid-ptr-checker.c     | 50 +++++++++++
 6 files changed, 163 insertions(+), 24 deletions(-)
 create mode 100644 clang/test/Analysis/invalid-ptr-checker.c

diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 65c1595eb6245dd..b4f65c934bf483b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -997,6 +997,15 @@ let ParentPackage = ENV in {
 
   def InvalidPtrChecker : Checker<"InvalidPtr">,
   HelpText<"Finds usages of possibly invalidated pointers">,
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "InvalidatingGetEnv",
+                  "Regard getenv as an invalidating call (as per POSIX "
+                  "standard), which can lead to false positives depending on "
+                  "implementation.",
+                  "false",
+                  InAlpha>,
+  ]>,
   Documentation<HasDocumentation>;
 
 } // end "alpha.cert.env"
diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index aae1a17bc0ae53e..8849eb1148564b7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -38,6 +38,15 @@ class InvalidPtrChecker
                                                 CheckerContext &C) const;
 
   // SEI CERT ENV31-C
+
+  // If set to true, consider getenv calls as invalidating operations on the
+  // environment variable buffer. This is implied in the standard, but in
+  // practice does not cause problems (in the commonly used environments).
+  bool InvalidatingGetEnv = false;
+
+  // GetEnv can be treated invalidating and non-invalidating as well.
+  const CallDescription GetEnvCall{{"getenv"}, 1};
+
   const CallDescriptionMap<HandlerFn> EnvpInvalidatingFunctions = {
       {{{"setenv"}, 3}, &InvalidPtrChecker::EnvpInvalidatingCall},
       {{{"unsetenv"}, 1}, &InvalidPtrChecker::EnvpInvalidatingCall},
@@ -51,7 +60,6 @@ class InvalidPtrChecker
 
   // SEI CERT ENV34-C
   const CallDescriptionMap<HandlerFn> PreviousCallInvalidatingFunctions = {
-      {{{"getenv"}, 1}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"setlocale"}, 2},
        &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"strerror"}, 1},
@@ -62,6 +70,10 @@ class InvalidPtrChecker
        &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
   };
 
+  // The private members of this checker corresponding to commandline options
+  // are set in this function.
+  friend void ento::registerInvalidPtrChecker(CheckerManager &);
+
 public:
   // Obtain the environment pointer from 'main()' (if present).
   void checkBeginFunction(CheckerContext &C) const;
@@ -84,7 +96,10 @@ class InvalidPtrChecker
 REGISTER_SET_WITH_PROGRAMSTATE(InvalidMemoryRegions, const MemRegion *)
 
 // Stores the region of the environment pointer of 'main' (if present).
-REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const MemRegion *)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(MainEnvPtrRegion, const MemRegion *)
+
+// Stores the regions of environments returned by getenv calls.
+REGISTER_SET_WITH_PROGRAMSTATE(GetenvEnvPtrRegions, const MemRegion *)
 
 // Stores key-value pairs, where key is function declaration and value is
 // pointer to memory region returned by previous call of this function
@@ -95,22 +110,35 @@ void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
                                              CheckerContext &C) const {
   StringRef FunctionName = Call.getCalleeIdentifier()->getName();
   ProgramStateRef State = C.getState();
-  const MemRegion *SymbolicEnvPtrRegion = State->get<EnvPtrRegion>();
-  if (!SymbolicEnvPtrRegion)
-    return;
 
-  State = State->add<InvalidMemoryRegions>(SymbolicEnvPtrRegion);
+  auto PlaceInvalidationNote = [&C, FunctionName,
+                                &State](const MemRegion *Region,
+                                        StringRef Message, ExplodedNode *Pred) {
+    State = State->add<InvalidMemoryRegions>(Region);
+
+    // Make copy of string data for the time when notes are *actually* created.
+    const NoteTag *Note =
+        C.getNoteTag([Region, FunctionName = std::string{FunctionName},
+                      Message = std::string{Message}](
+                         PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+          if (!BR.isInteresting(Region))
+            return;
+          Out << '\'' << FunctionName << "' " << Message;
+        });
+    return C.addTransition(State, Pred, Note);
+  };
 
-  const NoteTag *Note =
-      C.getNoteTag([SymbolicEnvPtrRegion, FunctionName](
-                       PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-        if (!BR.isInteresting(SymbolicEnvPtrRegion))
-          return;
-        Out << '\'' << FunctionName
-            << "' call may invalidate the environment parameter of 'main'";
-      });
+  ExplodedNode *CurrentChainEnd = C.getPredecessor();
+
+  if (const MemRegion *MainEnvPtr = State->get<MainEnvPtrRegion>())
+    CurrentChainEnd = PlaceInvalidationNote(
+        MainEnvPtr, "call may invalidate the environment parameter of 'main'",
+        CurrentChainEnd);
 
-  C.addTransition(State, Note);
+  for (const MemRegion *EnvPtr : State->get<GetenvEnvPtrRegions>())
+    CurrentChainEnd = PlaceInvalidationNote(
+        EnvPtr, "call may invalidate the environment returned by getenv",
+        CurrentChainEnd);
 }
 
 void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
@@ -146,8 +174,7 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
 
   // Remember to this region.
   const auto *SymRegOfRetVal = cast<SymbolicRegion>(RetVal.getAsRegion());
-  const MemRegion *MR =
-      const_cast<MemRegion *>(SymRegOfRetVal->getBaseRegion());
+  const MemRegion *MR = SymRegOfRetVal->getBaseRegion();
   State = State->set<PreviousCallResultMap>(FD, MR);
 
   ExplodedNode *Node = C.addTransition(State, Note);
@@ -185,6 +212,18 @@ static const MemRegion *findInvalidatedSymbolicBase(ProgramStateRef State,
 // function call as an argument.
 void InvalidPtrChecker::checkPostCall(const CallEvent &Call,
                                       CheckerContext &C) const {
+
+  ProgramStateRef State = C.getState();
+
+  // Model 'getenv' calls
+  if (GetEnvCall.matches(Call)) {
+    const MemRegion *Region = Call.getReturnValue().getAsRegion();
+    if (Region) {
+      State = State->add<GetenvEnvPtrRegions>(Region);
+      C.addTransition(State);
+    }
+  }
+
   // Check if function invalidates 'envp' argument of 'main'
   if (const auto *Handler = EnvpInvalidatingFunctions.lookup(Call))
     (this->**Handler)(Call, C);
@@ -193,14 +232,16 @@ void InvalidPtrChecker::checkPostCall(const CallEvent &Call,
   if (const auto *Handler = PreviousCallInvalidatingFunctions.lookup(Call))
     (this->**Handler)(Call, C);
 
+  // If pedantic mode is on, regard 'getenv' calls invalidating as well
+  if (InvalidatingGetEnv && GetEnvCall.matches(Call))
+    postPreviousReturnInvalidatingCall(Call, C);
+
   // Check if one of the arguments of the function call is invalidated
 
   // If call was inlined, don't report invalidated argument
   if (C.wasInlined)
     return;
 
-  ProgramStateRef State = C.getState();
-
   for (unsigned I = 0, NumArgs = Call.getNumArgs(); I < NumArgs; ++I) {
 
     if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(
@@ -243,7 +284,7 @@ void InvalidPtrChecker::checkBeginFunction(CheckerContext &C) const {
 
   // Save the memory region pointed by the environment pointer parameter of
   // 'main'.
-  C.addTransition(State->set<EnvPtrRegion>(EnvpReg));
+  C.addTransition(State->set<MainEnvPtrRegion>(EnvpReg));
 }
 
 // Check if invalidated region is being dereferenced.
@@ -268,7 +309,10 @@ void InvalidPtrChecker::checkLocation(SVal Loc, bool isLoad, const Stmt *S,
 }
 
 void ento::registerInvalidPtrChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<InvalidPtrChecker>();
+  auto *Checker = Mgr.registerChecker<InvalidPtrChecker>();
+  Checker->InvalidatingGetEnv =
+      Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker,
+                                                       "InvalidatingGetEnv");
 }
 
 bool ento::shouldRegisterInvalidPtrChecker(const CheckerManager &) {
diff --git a/clang/test/Analysis/analyzer-config.c b/clang/test/Analysis/analyzer-config.c
index d86ca5d19219c64..22b68cd1fd4c1c0 100644
--- a/clang/test/Analysis/analyzer-config.c
+++ b/clang/test/Analysis/analyzer-config.c
@@ -11,6 +11,7 @@
 // CHECK-NEXT: alpha.osx.cocoa.DirectIvarAssignment:AnnotatedFunctions = false
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
+// CHECK-NEXT: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv = false
 // CHECK-NEXT: alpha.security.taint.TaintPropagation:Config = ""
 // CHECK-NEXT: alpha.unix.Errno:AllowErrnoReadOutsideConditionExpressions = true
 // CHECK-NEXT: alpha.unix.StdCLibraryFunctions:DisplayLoadedSummaries = false
diff --git a/clang/test/Analysis/cert/env34-c-cert-examples.c b/clang/test/Analysis/cert/env34-c-cert-examples.c
index 19ca73f34c7ee5e..7e5b9be3cba6bb0 100644
--- a/clang/test/Analysis/cert/env34-c-cert-examples.c
+++ b/clang/test/Analysis/cert/env34-c-cert-examples.c
@@ -1,15 +1,49 @@
+// Default options.
 // RUN: %clang_analyze_cc1                                         \
 // RUN:  -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
 // RUN:  -verify -Wno-unused %s
+//
+// Test the laxer handling of getenv function (this is the default).
+// RUN: %clang_analyze_cc1                                         \
+// RUN:  -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
+// RUN:  -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
+// RUN:  -verify -Wno-unused %s
+//
+// Test the stricter handling of getenv function.
+// RUN: %clang_analyze_cc1                                         \
+// RUN:  -analyzer-checker=core,alpha.security.cert.env.InvalidPtr \
+// RUN:  -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
+// RUN:  -verify=pedantic -Wno-unused %s
 
 #include "../Inputs/system-header-simulator.h"
 char *getenv(const char *name);
+int setenv(const char *name, const char *value, int overwrite);
 int strcmp(const char*, const char*);
 char *strdup(const char*);
 void free(void *memblock);
 void *malloc(size_t size);
 
-void incorrect_usage(void) {
+void incorrect_usage_setenv_getenv_invalidation(void) {
+  char *tmpvar;
+  char *tempvar;
+
+  tmpvar = getenv("TMP");
+
+  if (!tmpvar)
+    return;
+
+  setenv("TEMP", "", 1); //setenv can invalidate env
+
+  if (!tmpvar)
+    return;
+
+  if (strcmp(tmpvar, "") == 0) { // body of strcmp is unknown
+    // expected-warning at -1{{use of invalidated pointer 'tmpvar' in a function call}}
+    // pedantic-warning at -2{{use of invalidated pointer 'tmpvar' in a function call}}
+  }
+}
+
+void incorrect_usage_double_getenv_invalidation(void) {
   char *tmpvar;
   char *tempvar;
 
@@ -18,13 +52,13 @@ void incorrect_usage(void) {
   if (!tmpvar)
     return;
 
-  tempvar = getenv("TEMP");
+  tempvar = getenv("TEMP"); //getenv should not invalidate env in non-pedantic mode
 
   if (!tempvar)
     return;
 
   if (strcmp(tmpvar, tempvar) == 0) { // body of strcmp is unknown
-    // expected-warning at -1{{use of invalidated pointer 'tmpvar' in a function call}}
+    // pedantic-warning at -1{{use of invalidated pointer 'tmpvar' in a function call}}
   }
 }
 
diff --git a/clang/test/Analysis/cert/env34-c.c b/clang/test/Analysis/cert/env34-c.c
index dc7b0340c311ed5..94bc2cf95ccc9b0 100644
--- a/clang/test/Analysis/cert/env34-c.c
+++ b/clang/test/Analysis/cert/env34-c.c
@@ -1,5 +1,6 @@
 // RUN: %clang_analyze_cc1 \
 // RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr\
+// RUN:  -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
 // RUN:  -analyzer-output=text -verify -Wno-unused %s
 
 #include "../Inputs/system-header-simulator.h"
diff --git a/clang/test/Analysis/invalid-ptr-checker.c b/clang/test/Analysis/invalid-ptr-checker.c
new file mode 100644
index 000000000000000..d2250e03f3d3d2f
--- /dev/null
+++ b/clang/test/Analysis/invalid-ptr-checker.c
@@ -0,0 +1,50 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr \
+// RUN:  -analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=false \
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
+//
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=alpha.security.cert.env.InvalidPtr \
+// RUN:  -analyzer-config \
+// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
+// RUN: -analyzer-output=text -verify=expected,pedantic -Wno-unused %s
+
+#include "Inputs/system-header-simulator.h"
+
+char *getenv(const char *name);
+int setenv(const char *name, const char *value, int overwrite);
+int strcmp(const char *, const char *);
+
+int custom_env_handler(const char **envp);
+
+void getenv_after_getenv(void) {
+  char *v1 = getenv("V1");
+  // pedantic-note at -1{{previous function call was here}}
+
+  char *v2 = getenv("V2");
+  // pedantic-note at -1{{'getenv' call may invalidate the result of the previous 'getenv'}}
+
+  strcmp(v1, v2);
+  // pedantic-warning at -1{{use of invalidated pointer 'v1' in a function call}}
+  // pedantic-note at -2{{use of invalidated pointer 'v1' in a function call}}
+}
+
+void setenv_after_getenv(void) {
+  char *v1 = getenv("VAR1");
+
+  setenv("VAR2", "...", 1);
+  // expected-note at -1{{'setenv' call may invalidate the environment returned by getenv}}
+
+  strcmp(v1, "");
+  // expected-warning at -1{{use of invalidated pointer 'v1' in a function call}}
+  // expected-note at -2{{use of invalidated pointer 'v1' in a function call}}
+}
+
+int main(int argc, const char *argv[], const char *envp[]) {
+  setenv("VAR", "...", 0);
+  // expected-note at -1 2 {{'setenv' call may invalidate the environment parameter of 'main'}}
+
+  *envp;
+  // expected-warning at -1 2 {{dereferencing an invalid pointer}}
+  // expected-note at -2 2 {{dereferencing an invalid pointer}}
+}

>From 2f8bc383acfd2d8e514884441c8fa74d1798ece4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Thu, 21 Sep 2023 16:03:46 +0200
Subject: [PATCH 2/8] Add explicit State parameter

---
 .../Checkers/cert/InvalidPtrChecker.cpp            | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index 8849eb1148564b7..5b576ace1d547d3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -109,11 +109,11 @@ REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const FunctionDecl *,
 void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
                                              CheckerContext &C) const {
   StringRef FunctionName = Call.getCalleeIdentifier()->getName();
-  ProgramStateRef State = C.getState();
 
-  auto PlaceInvalidationNote = [&C, FunctionName,
-                                &State](const MemRegion *Region,
-                                        StringRef Message, ExplodedNode *Pred) {
+  auto PlaceInvalidationNote = [&C, FunctionName](ProgramStateRef State,
+                                                  const MemRegion *Region,
+                                                  StringRef Message,
+                                                  ExplodedNode *Pred) {
     State = State->add<InvalidMemoryRegions>(Region);
 
     // Make copy of string data for the time when notes are *actually* created.
@@ -128,16 +128,18 @@ void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
     return C.addTransition(State, Pred, Note);
   };
 
+  ProgramStateRef State = C.getState();
   ExplodedNode *CurrentChainEnd = C.getPredecessor();
 
   if (const MemRegion *MainEnvPtr = State->get<MainEnvPtrRegion>())
     CurrentChainEnd = PlaceInvalidationNote(
-        MainEnvPtr, "call may invalidate the environment parameter of 'main'",
+        State, MainEnvPtr,
+        "call may invalidate the environment parameter of 'main'",
         CurrentChainEnd);
 
   for (const MemRegion *EnvPtr : State->get<GetenvEnvPtrRegions>())
     CurrentChainEnd = PlaceInvalidationNote(
-        EnvPtr, "call may invalidate the environment returned by getenv",
+        State, EnvPtr, "call may invalidate the environment returned by getenv",
         CurrentChainEnd);
 }
 

>From 32e6697271c44ef35e1c7a4f00b459aed186757a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Thu, 21 Sep 2023 16:04:13 +0200
Subject: [PATCH 3/8] Add NoteTag filtering based on exact BugType* equality

---
 .../Checkers/cert/InvalidPtrChecker.cpp       | 26 ++++++++++++++-----
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index 5b576ace1d547d3..4a8e71743f3a845 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -25,12 +25,20 @@
 using namespace clang;
 using namespace ento;
 
+
 namespace {
 
+
 class InvalidPtrChecker
     : public Checker<check::Location, check::BeginFunction, check::PostCall> {
 private:
-  BugType BT{this, "Use of invalidated pointer", categories::MemoryError};
+  static const BugType *InvalidPtrBugType;
+  // For accurate emission of NoteTags, the BugType of this checker should have
+  // a unique address.
+  void InitInvalidPtrBugType() {
+    InvalidPtrBugType = new BugType(this, "Use of invalidated pointer",
+                                    categories::MemoryError);
+  }
 
   void EnvpInvalidatingCall(const CallEvent &Call, CheckerContext &C) const;
 
@@ -90,6 +98,8 @@ class InvalidPtrChecker
                      CheckerContext &C) const;
 };
 
+const BugType *InvalidPtrChecker::InvalidPtrBugType;
+
 } // namespace
 
 // Set of memory regions that were invalidated
@@ -121,7 +131,8 @@ void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
         C.getNoteTag([Region, FunctionName = std::string{FunctionName},
                       Message = std::string{Message}](
                          PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-          if (!BR.isInteresting(Region))
+          if (!BR.isInteresting(Region) ||
+              &BR.getBugType() != InvalidPtrBugType)
             return;
           Out << '\'' << FunctionName << "' " << Message;
         });
@@ -156,7 +167,7 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
     State = State->add<InvalidMemoryRegions>(PrevReg);
     Note = C.getNoteTag([PrevReg, FD](PathSensitiveBugReport &BR,
                                       llvm::raw_ostream &Out) {
-      if (!BR.isInteresting(PrevReg))
+      if (!BR.isInteresting(PrevReg) || &BR.getBugType() != InvalidPtrBugType)
         return;
       Out << '\'';
       FD->getNameForDiagnostic(Out, FD->getASTContext().getLangOpts(), true);
@@ -182,7 +193,7 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
   ExplodedNode *Node = C.addTransition(State, Note);
   const NoteTag *PreviousCallNote =
       C.getNoteTag([MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-        if (!BR.isInteresting(MR))
+        if (!BR.isInteresting(MR) || &BR.getBugType() != InvalidPtrBugType)
           return;
         Out << '\'' << "'previous function call was here" << '\'';
       });
@@ -261,8 +272,8 @@ void InvalidPtrChecker::checkPostCall(const CallEvent &Call,
                                         C.getASTContext().getPrintingPolicy());
         Out << "' in a function call";
 
-        auto Report =
-            std::make_unique<PathSensitiveBugReport>(BT, Out.str(), ErrorNode);
+        auto Report = std::make_unique<PathSensitiveBugReport>(
+            *InvalidPtrBugType, Out.str(), ErrorNode);
         Report->markInteresting(InvalidatedSymbolicBase);
         Report->addRange(Call.getArgSourceRange(I));
         C.emitReport(std::move(Report));
@@ -305,7 +316,7 @@ void InvalidPtrChecker::checkLocation(SVal Loc, bool isLoad, const Stmt *S,
     return;
 
   auto Report = std::make_unique<PathSensitiveBugReport>(
-      BT, "dereferencing an invalid pointer", ErrorNode);
+      *InvalidPtrBugType, "dereferencing an invalid pointer", ErrorNode);
   Report->markInteresting(InvalidatedSymbolicBase);
   C.emitReport(std::move(Report));
 }
@@ -315,6 +326,7 @@ void ento::registerInvalidPtrChecker(CheckerManager &Mgr) {
   Checker->InvalidatingGetEnv =
       Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker,
                                                        "InvalidatingGetEnv");
+  Checker->InitInvalidPtrBugType();
 }
 
 bool ento::shouldRegisterInvalidPtrChecker(const CheckerManager &) {

>From fde714a51611e3e025f8eecd001391a76d754a44 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Thu, 21 Sep 2023 16:36:06 +0200
Subject: [PATCH 4/8] Mark region uninteresing after placing Note

---
 clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index 4a8e71743f3a845..e394705cee85561 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -135,6 +135,7 @@ void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
               &BR.getBugType() != InvalidPtrBugType)
             return;
           Out << '\'' << FunctionName << "' " << Message;
+          BR.markNotInteresting(Region);
         });
     return C.addTransition(State, Pred, Note);
   };

>From 3377755f5cd03dbdf3b958b60f2df1291f39d603 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Mon, 25 Sep 2023 10:01:31 +0200
Subject: [PATCH 5/8] Add documentation and example of the option

---
 clang/docs/analyzer/checkers.rst | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index dbd6d7787823530..b73a213db789de0 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2399,13 +2399,34 @@ pointer. These functions include: getenv, localeconv, asctime, setlocale, strerr
     char *p, *pp;
 
     p = getenv("VAR");
-    pp = getenv("VAR2");
-    // subsequent call to 'getenv' invalidated previous one
+    setenv("SOMEVAR", "VALUE", /*overwrite = */1);
+    // call to 'setenv' may invalidate p
 
     *p;
     // dereferencing invalid pointer
   }
 
+
+The ``InvalidatingGetEnv`` option is available for treating getenv calls as
+invalidating. When enabled, the checker issues a warning if getenv is called
+multiple times and their results are used without first creating a copy.
+This level of strictness might be considered overly pedantic for the commonly
+used getenv implementations.
+
+To enable this option, use:
+``-analyzer-config alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true``.
+
+By default, this option is set to *false*.
+
+When this option is enabled, warnings will be generated for scenarios like the
+following:
+
+.. code-block:: c
+
+  char* p = getenv("VAR");
+  char* pp = getenv("VAR2"); // assumes this call can invalidate `env`
+  strlen(p); // warns about accessing invalid ptr
+
 alpha.security.taint
 ^^^^^^^^^^^^^^^^^^^^
 

>From 9be2f319a1a43185338ccea3d7c0480a18594659 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Tue, 26 Sep 2023 11:00:06 +0200
Subject: [PATCH 6/8] Remove extra quote in note tag message

---
 clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index e394705cee85561..b233d2197c3a3a4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -196,7 +196,7 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
       C.getNoteTag([MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
         if (!BR.isInteresting(MR) || &BR.getBugType() != InvalidPtrBugType)
           return;
-        Out << '\'' << "'previous function call was here" << '\'';
+        Out << "previous function call was here";
       });
 
   C.addTransition(State, Node, PreviousCallNote);

>From 2b6d75e9e0c2a2f2b9b0e03e38977b19b61b808a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Mon, 2 Oct 2023 10:29:53 +0200
Subject: [PATCH 7/8] Return to member variable BugType, as suggested by
 @DonatNagyE

---
 .../Checkers/cert/InvalidPtrChecker.cpp       | 40 ++++++++-----------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index b233d2197c3a3a4..be6657bb28381fc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -25,20 +25,15 @@
 using namespace clang;
 using namespace ento;
 
-
 namespace {
 
-
 class InvalidPtrChecker
     : public Checker<check::Location, check::BeginFunction, check::PostCall> {
 private:
-  static const BugType *InvalidPtrBugType;
   // For accurate emission of NoteTags, the BugType of this checker should have
   // a unique address.
-  void InitInvalidPtrBugType() {
-    InvalidPtrBugType = new BugType(this, "Use of invalidated pointer",
-                                    categories::MemoryError);
-  }
+  BugType InvalidPtrBugType{this, "Use of invalidated pointer",
+                            categories::MemoryError};
 
   void EnvpInvalidatingCall(const CallEvent &Call, CheckerContext &C) const;
 
@@ -98,8 +93,6 @@ class InvalidPtrChecker
                      CheckerContext &C) const;
 };
 
-const BugType *InvalidPtrChecker::InvalidPtrBugType;
-
 } // namespace
 
 // Set of memory regions that were invalidated
@@ -120,19 +113,19 @@ void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
                                              CheckerContext &C) const {
   StringRef FunctionName = Call.getCalleeIdentifier()->getName();
 
-  auto PlaceInvalidationNote = [&C, FunctionName](ProgramStateRef State,
-                                                  const MemRegion *Region,
-                                                  StringRef Message,
-                                                  ExplodedNode *Pred) {
+  auto PlaceInvalidationNote = [this, &C, FunctionName](ProgramStateRef State,
+                                                        const MemRegion *Region,
+                                                        StringRef Message,
+                                                        ExplodedNode *Pred) {
     State = State->add<InvalidMemoryRegions>(Region);
 
     // Make copy of string data for the time when notes are *actually* created.
     const NoteTag *Note =
-        C.getNoteTag([Region, FunctionName = std::string{FunctionName},
+        C.getNoteTag([this, Region, FunctionName = std::string{FunctionName},
                       Message = std::string{Message}](
                          PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
           if (!BR.isInteresting(Region) ||
-              &BR.getBugType() != InvalidPtrBugType)
+              &BR.getBugType() != &InvalidPtrBugType)
             return;
           Out << '\'' << FunctionName << "' " << Message;
           BR.markNotInteresting(Region);
@@ -166,9 +159,9 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
   if (const MemRegion *const *Reg = State->get<PreviousCallResultMap>(FD)) {
     const MemRegion *PrevReg = *Reg;
     State = State->add<InvalidMemoryRegions>(PrevReg);
-    Note = C.getNoteTag([PrevReg, FD](PathSensitiveBugReport &BR,
-                                      llvm::raw_ostream &Out) {
-      if (!BR.isInteresting(PrevReg) || &BR.getBugType() != InvalidPtrBugType)
+    Note = C.getNoteTag([this, PrevReg, FD](PathSensitiveBugReport &BR,
+                                            llvm::raw_ostream &Out) {
+      if (!BR.isInteresting(PrevReg) || &BR.getBugType() != &InvalidPtrBugType)
         return;
       Out << '\'';
       FD->getNameForDiagnostic(Out, FD->getASTContext().getLangOpts(), true);
@@ -192,9 +185,9 @@ void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
   State = State->set<PreviousCallResultMap>(FD, MR);
 
   ExplodedNode *Node = C.addTransition(State, Note);
-  const NoteTag *PreviousCallNote =
-      C.getNoteTag([MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-        if (!BR.isInteresting(MR) || &BR.getBugType() != InvalidPtrBugType)
+  const NoteTag *PreviousCallNote = C.getNoteTag(
+      [this, MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+        if (!BR.isInteresting(MR) || &BR.getBugType() != &InvalidPtrBugType)
           return;
         Out << "previous function call was here";
       });
@@ -274,7 +267,7 @@ void InvalidPtrChecker::checkPostCall(const CallEvent &Call,
         Out << "' in a function call";
 
         auto Report = std::make_unique<PathSensitiveBugReport>(
-            *InvalidPtrBugType, Out.str(), ErrorNode);
+            InvalidPtrBugType, Out.str(), ErrorNode);
         Report->markInteresting(InvalidatedSymbolicBase);
         Report->addRange(Call.getArgSourceRange(I));
         C.emitReport(std::move(Report));
@@ -317,7 +310,7 @@ void InvalidPtrChecker::checkLocation(SVal Loc, bool isLoad, const Stmt *S,
     return;
 
   auto Report = std::make_unique<PathSensitiveBugReport>(
-      *InvalidPtrBugType, "dereferencing an invalid pointer", ErrorNode);
+      InvalidPtrBugType, "dereferencing an invalid pointer", ErrorNode);
   Report->markInteresting(InvalidatedSymbolicBase);
   C.emitReport(std::move(Report));
 }
@@ -327,7 +320,6 @@ void ento::registerInvalidPtrChecker(CheckerManager &Mgr) {
   Checker->InvalidatingGetEnv =
       Mgr.getAnalyzerOptions().getCheckerBooleanOption(Checker,
                                                        "InvalidatingGetEnv");
-  Checker->InitInvalidPtrBugType();
 }
 
 bool ento::shouldRegisterInvalidPtrChecker(const CheckerManager &) {

>From 1faca072459898c26d7e19b2ba1fe1315b9e2171 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fulop at sigmatechnology.se>
Date: Tue, 3 Oct 2023 12:07:53 +0200
Subject: [PATCH 8/8] Create only a single NoteTag per invalidation

---
 .../Checkers/cert/InvalidPtrChecker.cpp       | 84 ++++++++++++-------
 clang/test/Analysis/invalid-ptr-checker.c     |  2 +-
 2 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
index be6657bb28381fc..543282fabd3793c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp
@@ -91,6 +91,11 @@ class InvalidPtrChecker
   // Check if invalidated region is being dereferenced.
   void checkLocation(SVal l, bool isLoad, const Stmt *S,
                      CheckerContext &C) const;
+
+private:
+  const NoteTag *createEnvInvalidationNote(CheckerContext &C,
+                                           ProgramStateRef State,
+                                           StringRef FunctionName) const;
 };
 
 } // namespace
@@ -109,43 +114,58 @@ REGISTER_SET_WITH_PROGRAMSTATE(GetenvEnvPtrRegions, const MemRegion *)
 REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const FunctionDecl *,
                                const MemRegion *)
 
-void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
-                                             CheckerContext &C) const {
-  StringRef FunctionName = Call.getCalleeIdentifier()->getName();
+const NoteTag *InvalidPtrChecker::createEnvInvalidationNote(
+    CheckerContext &C, ProgramStateRef State, StringRef FunctionName) const {
 
-  auto PlaceInvalidationNote = [this, &C, FunctionName](ProgramStateRef State,
-                                                        const MemRegion *Region,
-                                                        StringRef Message,
-                                                        ExplodedNode *Pred) {
-    State = State->add<InvalidMemoryRegions>(Region);
-
-    // Make copy of string data for the time when notes are *actually* created.
-    const NoteTag *Note =
-        C.getNoteTag([this, Region, FunctionName = std::string{FunctionName},
-                      Message = std::string{Message}](
-                         PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-          if (!BR.isInteresting(Region) ||
-              &BR.getBugType() != &InvalidPtrBugType)
-            return;
-          Out << '\'' << FunctionName << "' " << Message;
-          BR.markNotInteresting(Region);
-        });
-    return C.addTransition(State, Pred, Note);
-  };
+  const MemRegion *MainRegion = State->get<MainEnvPtrRegion>();
+  const auto GetenvRegions = State->get<GetenvEnvPtrRegions>();
 
-  ProgramStateRef State = C.getState();
-  ExplodedNode *CurrentChainEnd = C.getPredecessor();
+  return C.getNoteTag([this, MainRegion, GetenvRegions,
+                       FunctionName = std::string{FunctionName}](
+                          PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+    auto IsInterestingForInvalidation = [this, &BR](const MemRegion *R) {
+      return R && &BR.getBugType() == &InvalidPtrBugType && BR.isInteresting(R);
+    };
 
-  if (const MemRegion *MainEnvPtr = State->get<MainEnvPtrRegion>())
-    CurrentChainEnd = PlaceInvalidationNote(
-        State, MainEnvPtr,
-        "call may invalidate the environment parameter of 'main'",
-        CurrentChainEnd);
+    // Craft the note tag message.
+    llvm::SmallVector<std::string, 2> InvalidLocationNames;
+    if (IsInterestingForInvalidation(MainRegion)) {
+      InvalidLocationNames.push_back("the environment parameter of 'main'");
+    }
+    if (llvm::any_of(GetenvRegions, IsInterestingForInvalidation))
+      InvalidLocationNames.push_back("the environment returned by 'getenv'");
+
+    if (InvalidLocationNames.size() >= 1)
+      Out << '\'' << FunctionName << "' call may invalidate "
+          << InvalidLocationNames[0];
+    if (InvalidLocationNames.size() == 2)
+      Out << ", and " << InvalidLocationNames[1];
+
+    // Mark all regions that were interesting before as NOT interesting now
+    // to avoid extra notes coming from other checkers.
+    if (IsInterestingForInvalidation(MainRegion))
+      BR.markNotInteresting(MainRegion);
+    for (const MemRegion *GetenvRegion : GetenvRegions)
+      if (IsInterestingForInvalidation(GetenvRegion))
+        BR.markNotInteresting(GetenvRegion);
+  });
+}
 
+void InvalidPtrChecker::EnvpInvalidatingCall(const CallEvent &Call,
+                                             CheckerContext &C) const {
+  // This callevent invalidates all previously generated pointers to the
+  // environment.
+  ProgramStateRef State = C.getState();
+  if (const MemRegion *MainEnvPtr = State->get<MainEnvPtrRegion>())
+    State = State->add<InvalidMemoryRegions>(MainEnvPtr);
   for (const MemRegion *EnvPtr : State->get<GetenvEnvPtrRegions>())
-    CurrentChainEnd = PlaceInvalidationNote(
-        State, EnvPtr, "call may invalidate the environment returned by getenv",
-        CurrentChainEnd);
+    State = State->add<InvalidMemoryRegions>(EnvPtr);
+
+  StringRef FunctionName = Call.getCalleeIdentifier()->getName();
+  const NoteTag *InvalidationNote =
+      createEnvInvalidationNote(C, State, FunctionName);
+
+  C.addTransition(State, InvalidationNote);
 }
 
 void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
diff --git a/clang/test/Analysis/invalid-ptr-checker.c b/clang/test/Analysis/invalid-ptr-checker.c
index d2250e03f3d3d2f..4b257f9e78003fc 100644
--- a/clang/test/Analysis/invalid-ptr-checker.c
+++ b/clang/test/Analysis/invalid-ptr-checker.c
@@ -33,7 +33,7 @@ void setenv_after_getenv(void) {
   char *v1 = getenv("VAR1");
 
   setenv("VAR2", "...", 1);
-  // expected-note at -1{{'setenv' call may invalidate the environment returned by getenv}}
+  // expected-note at -1{{'setenv' call may invalidate the environment returned by 'getenv'}}
 
   strcmp(v1, "");
   // expected-warning at -1{{use of invalidated pointer 'v1' in a function call}}



More information about the cfe-commits mailing list