[analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

[gamesh411]

Summary

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.

[gamesh411]

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

99–102

The state modelling is refined to model the env region coming from the main function and the getenv calls.

clang/test/Analysis/cert/env34-c.c

6

This test file is incomplete.
I would welcome suggestions here as to how to test this.
Should a new file be created for the config option with different test cases, or is this file to be extended?

[donat.nagy]

The commit looks good in general, I have a few minor suggestions and a serious question about the state transition bureaucracy.

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

74–75

Nitpick: "practical false positives" sounds strange for me, consider writing
[...but] "in practice does not cause problems (in the commonly used environments)"
or something similar.

131–136

Perhaps add a comment that clarifies that passing a nullptr as the ExplodedNode to addTransition is equivalent to specifying the current node. I remember this because I was studying its implementation recently, but I would've been surprised and suspicious otherwise.

223

I fear that this state transition will go "sideways" and the later state transitions (which add the note tags) will branch off instead of building onto this. IIUC calling CheckerContext::addTransition registers the transition without updating the "current ExplodedNode" field of CheckerContext, so you need to explicitly store and pass around the ExplodedNode returned by it if you want to build on it.

This is an ugly and counter-intuitive API, and I also ran into a very similar issue a few weeks ago (@Szelethus helped me).

clang/test/Analysis/cert/env34-c.c

6

Personally I'd prefer putting those cases into a separate files, because this test file is already 340 lines long and it'd be difficult to understand if it was filled with conditional checks.

[steakhal]

I'm sorry starting the review of this one only now, but I'm quite booked.
Is it still relevant? If so, I'll continue.

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

120–121

FunctionName and Message will dangle inside the NoteTag.

131–136

If nullptr is equivalent with C.getPredecessor() inside addTransition(), why not simply initialize it to that value instead of to nullptr?

219

We should hoist this into a field, to only construct it once.

221–222

What ensures that Call.getReturnValue().getAsRegion() is not null?

223

I think the usage here is correct.

[gamesh411]

Add tests for checker option
Remove unnecessary const_cast
Only model a getenv call if there is a value to model
Use getPredecessor to better indicate what happens during EG building
Hoist GetEnvCall variable
Fix dangling strings in note generation

[gamesh411]

rebased and squashed

[gamesh411]

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

44

Reworded the message here

48

Hoisted here

120–121

Good catch, thanks! Fixed this with a lambda capture initializer.

131–136

I ended up using C.getPredecessor() instead of explaining; this seems a bit more intuitive (if such a thing even exists in CSA).

223

(the line number of this comment desync-ed)
I agree, that the addTransition API is easy to misuse, and I would welcome a more streamlined approach.
I tried to pay attention to "build" the state and the Exploded Graph by always providing the Exploded Node (second parameter), and this seems fine.

clang/test/Analysis/invalid-ptr-checker.c

52

This gives 2 warnings. One for subexpression envp, and one for the whole statement *envp. This is the current behaviour ( check clang/test/Analysis/cert/env31-c.c ), and this patch does not change it.
However, I would like to devise a solution for this in a different patch.
One option would be to make the error of this checker Fatal, so only one would appear, or refine the checkLocation callback to only consider one of these 2 cases for reporting.

[gamesh411]

This comment was removed by gamesh411.

[gamesh411]

In D154603#4609809, @gamesh411 wrote:

In D154603#4580609, @steakhal wrote:

I'm sorry starting the review of this one only now, but I'm quite booked.
Is it still relevant? If so, I'll continue.

Yes, thanks for the effort!

I would like to go through with this option, and then I would like to fix the following issues with this checker as well:

• the previous function call notes could be more streamlined
• the notes of this checker are also shown when another checker hits those nodes with its report
  • for example taint checker giving a warning to getenv usage would also trigger the display of the 'previous function call was here' note here), this I would like to filter with bug category filters
  • code examples for this filtering are below
• try to consolidate the multiple warnings coming from this checker's checkLocation callback

category based filtering ( example from lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:167 ):

If (!BR.isInteresting(CallLocation) ||
  BR.getBugType().getCategory() != categories::TaintedData) { //but this would be InvalidPtr BugType's category, namely memory_error
  return "";
}

or checker based filtering ( example from lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:397 )

if (&BR.getBugType() != smartptr::getNullDereferenceBugType() || // this is a comparison of the address of a static bugtype
    !BR.isInteresting(ThisRegion))

This second one gives a more precise filtering, but the implementation-specific detail of storing the bugtype by reference is what seems to make this work, which I find hacky.

[steakhal]

In D154603#4609872, @gamesh411 wrote:

• try to consolidate the multiple warnings coming from this checker's checkLocation callback

category based filtering ( example from lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:167 ):

If (!BR.isInteresting(CallLocation) ||
  BR.getBugType().getCategory() != categories::TaintedData) { //but this would be InvalidPtr BugType's category, namely memory_error
  return "";
}

or checker based filtering ( example from lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:397 )

if (&BR.getBugType() != smartptr::getNullDereferenceBugType() || // this is a comparison of the address of a static bugtype
    !BR.isInteresting(ThisRegion))

This second one gives a more precise filtering, but the implementation-specific detail of storing the bugtype by reference is what seems to make this work, which I find hacky.

If the checker issues a NoteTag, it makes sense in certain situations to make sure that it acts on only reports issued by that checker. The standard way of achieving that is by comparing the tags, as you do in the second example.
There is nothing wrong with this, if that particular checker issued that NoteTag in the first place. It's marginally debatable, if it was not issued by the given checker but rather something else. That would suggest to me some logic flaw, or coupling issue. For cross-checker cases, I think the bug category would be the better option, but I would still need to think about that, so not set in stone :D

FYI I haven't looked at the patch yet, but I wanted to answer your question.

[donat.nagy]

The change looks promising, I only have minor remarks.

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

121–122

Minor nitpick: in situations like this, when we want to save an already composed string, std::string is better than SmallString because it doesn't occupy more memory than the actual length of the string. (OTOH SmallString is better for buffer variables, I've seen code that creates a SmallString, composes the message in it, then converts it to a std::string for longer-term storage.)

Of course these are all just inconsequential micro-optimization details...

131–136

if such a thing even exists in CSA

😆

clang/test/Analysis/invalid-ptr-checker.c

11

Use -verify=expected,pedantic here and then you can eliminate the pedantic-warning lines that duplicate the messages of expected-warnings.

[gamesh411]

• use std::string
• simplify tests

[gamesh411]

@steakhal gentle ping

[steakhal]

Thanks for the ping. I have some concerns inline.

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

1000–1008

I think we should mention this flag in the docs, and an example.

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

115

I'd prefer an explicit out parameter instead of capturing &State here.
This way we only capture "immutable" stuff.

120–121

On second thought, I'm wrong. It won't dangle, because the StringRef(FunctionName) is owned by the identifier of the function, and thus lives as long as the ASTContext.
But Message would dangle :D

124

To me, it feels like all the messages we emit from this NoteTag, are specific to this particular checker.
if that's true, checking interestingness is not enough, and we should also make sure that the BugType is from this checker.
Otherwise, this note could appear for any other reason when the region is marked as interesting.

I also have the feeling that it should mark uninteresting the region once it puts a message there - which should stop other notes placed for the same reason for other - basically unrelated env invalidations.
Could you verify this with a test?

138–141

I'd prefer if we wouldn't put N separate NoteTags, but rather iterate over this set of regions inside the NoteTag.
That way here you don't need the loop and play with Pred nodes.

219–225

[gamesh411]

Moving this to GitHub as Phabricator is shutting down. Relevant PR here: https://github.com/llvm/llvm-project/pull/67663

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

let ParentPackage = POS in {
   Documentation<HasDocumentation>;
 
 } // end "alpha.cert.pos"
 
 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"
 
 let ParentPackage = SecurityAlpha in {
 
 def ArrayBoundChecker : Checker<"ArrayBound">,
   HelpText<"Warn about buffer overflows (older checker)">,

clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp

 //== InvalidPtrChecker.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
 //
 //===----------------------------------------------------------------------===//
 //
private:
   BugType BT{this, "Use of invalidated pointer", categories::MemoryError};
 
   void EnvpInvalidatingCall(const CallEvent &Call, CheckerContext &C) const;
 
   using HandlerFn = void (InvalidPtrChecker::*)(const CallEvent &Call,
                                                 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},
       {{{"putenv"}, 1}, &InvalidPtrChecker::EnvpInvalidatingCall},
       {{{"_putenv_s"}, 2}, &InvalidPtrChecker::EnvpInvalidatingCall},
       {{{"_wputenv_s"}, 2}, &InvalidPtrChecker::EnvpInvalidatingCall},
   };
 
   void postPreviousReturnInvalidatingCall(const CallEvent &Call,
                                           CheckerContext &C) const;
 
   // SEI CERT ENV34-C
   const CallDescriptionMap<HandlerFn> PreviousCallInvalidatingFunctions = {
-      {{{"getenv"}, 1}, &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"setlocale"}, 2},
        &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"strerror"}, 1},
        &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"localeconv"}, 0},
        &InvalidPtrChecker::postPreviousReturnInvalidatingCall},
       {{{"asctime"}, 1},
        &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;
 
   // Handle functions in EnvpInvalidatingFunctions, that invalidate environment
   // pointer from 'main()'
   // Handle functions in PreviousCallInvalidatingFunctions.
   // Also, check if invalidated region is passed to a
   // conservatively evaluated function call as an argument.
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
 
   // Check if invalidated region is being dereferenced.
   void checkLocation(SVal l, bool isLoad, const Stmt *S,
                      CheckerContext &C) const;
 };
 
 } // namespace
 
 // Set of memory regions that were invalidated
 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
 REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const FunctionDecl *,
                                const MemRegion *)
 
 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([SymbolicEnvPtrRegion, FunctionName](
+        C.getNoteTag([Region, FunctionName = std::string{FunctionName},
+                      Message = std::string{Message}](
                          PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-        if (!BR.isInteresting(SymbolicEnvPtrRegion))
+          if (!BR.isInteresting(Region))
             return;
-        Out << '\'' << FunctionName
-            << "' call may invalidate the environment parameter of 'main'";
+          Out << '\'' << FunctionName << "' " << Message;
         });
+    return C.addTransition(State, Pred, Note);
+  };
 
-  C.addTransition(State, Note);
+  ExplodedNode *CurrentChainEnd = C.getPredecessor();
+
+  if (const MemRegion *MainEnvPtr = State->get<MainEnvPtrRegion>())
+    CurrentChainEnd = PlaceInvalidationNote(
+        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",
+        CurrentChainEnd);
 }
 
 void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
     const CallEvent &Call, CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   const NoteTag *Note = nullptr;
   const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
void InvalidPtrChecker::postPreviousReturnInvalidatingCall(
 
   // Function call will return a pointer to the new symbolic region.
   DefinedOrUnknownSVal RetVal = C.getSValBuilder().conjureSymbolVal(
       CE, LCtx, CE->getType(), C.blockCount());
   State = State->BindExpr(CE, LCtx, RetVal);
 
   // 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);
   const NoteTag *PreviousCallNote =
       C.getNoteTag([MR](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
         if (!BR.isInteresting(MR))
           return;
         Out << '\'' << "'previous function call was here" << '\'';
static const MemRegion *findInvalidatedSymbolicBase(ProgramStateRef State,
   return nullptr;
 }
 
 // Handle functions in EnvpInvalidatingFunctions, that invalidate environment
 // pointer from 'main()' Also, check if invalidated region is passed to a
 // 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);
 
   // Check if function invalidates the result of previous 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>(
             Call.getArgSVal(I).getAsRegion())) {
       if (const MemRegion *InvalidatedSymbolicBase =
               findInvalidatedSymbolicBase(State, SR)) {
         ExplodedNode *ErrorNode = C.generateNonFatalErrorNode();
         if (!ErrorNode)
if (!FD || FD->param_size() != 3 || !FD->isMain())
     return;
 
   ProgramStateRef State = C.getState();
   const MemRegion *EnvpReg =
       State->getRegion(FD->parameters()[2], C.getLocationContext());
 
   // 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.
 void InvalidPtrChecker::checkLocation(SVal Loc, bool isLoad, const Stmt *S,
                                       CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // Ignore memory operations involving 'non-invalidated' locations.
   const MemRegion *InvalidatedSymbolicBase =
       findInvalidatedSymbolicBase(State, Loc.getAsRegion());
   if (!InvalidatedSymbolicBase)
     return;
 
   ExplodedNode *ErrorNode = C.generateNonFatalErrorNode();
   if (!ErrorNode)
     return;
 
   auto Report = std::make_unique<PathSensitiveBugReport>(
       BT, "dereferencing an invalid pointer", ErrorNode);
   Report->markInteresting(InvalidatedSymbolicBase);
   C.emitReport(std::move(Report));
 }
 
 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 &) {
   return true;
 }

clang/test/Analysis/analyzer-config.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=debug.ConfigDumper > %t 2>&1
 // RUN: FileCheck --input-file=%t %s --match-full-lines
 
 // CHECK:      [config]
 // CHECK-NEXT: add-pop-up-notes = true
 // CHECK-NEXT: aggressive-binary-operation-simplification = false
 // CHECK-NEXT: alpha.clone.CloneChecker:IgnoredFilesPattern = ""
 // CHECK-NEXT: alpha.clone.CloneChecker:MinimumCloneComplexity = 50
 // CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true
 // CHECK-NEXT: alpha.cplusplus.STLAlgorithmModeling:AggressiveStdFindModeling = false
 // 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
 // CHECK-NEXT: alpha.unix.StdCLibraryFunctions:ModelPOSIX = false
 // CHECK-NEXT: apply-fixits = false
 // CHECK-NEXT: assume-controlled-environment = false
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true

clang/test/Analysis/cert/env34-c-cert-examples.c

+// 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@-1{{use of invalidated pointer 'tmpvar' in a function call}}
+    // pedantic-warning@-2{{use of invalidated pointer 'tmpvar' in a function call}}
+  }
+}
+
+void incorrect_usage_double_getenv_invalidation(void) {
   char *tmpvar;
   char *tempvar;
 
   tmpvar = getenv("TMP");
 
   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@-1{{use of invalidated pointer 'tmpvar' in a function call}}
+    // pedantic-warning@-1{{use of invalidated pointer 'tmpvar' in a function call}}
   }
 }
 
 void correct_usage_1(void) {
   char *tmpvar;
   char *tempvar;
 
   const char *temp = getenv("TMP");

clang/test/Analysis/cert/env34-c.c

 // 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"
 char *getenv(const char *name);
 char *setlocale(int category, const char *locale);
 char *strerror(int errnum);
 
 typedef struct {
   char * field;
 } lconv;
 lconv *localeconv(void);

clang/test/Analysis/invalid-ptr-checker.c

+// 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@-1{{previous function call was here}}
+
+  char *v2 = getenv("V2");
+  // pedantic-note@-1{{'getenv' call may invalidate the result of the previous 'getenv'}}
+
+  strcmp(v1, v2);
+  // pedantic-warning@-1{{use of invalidated pointer 'v1' in a function call}}
+  // pedantic-note@-2{{use of invalidated pointer 'v1' in a function call}}
+}
+
+void setenv_after_getenv(void) {
+  char *v1 = getenv("VAR1");
+
+  setenv("VAR2", "...", 1);
+  // expected-note@-1{{'setenv' call may invalidate the environment returned by getenv}}
+
+  strcmp(v1, "");
+  // expected-warning@-1{{use of invalidated pointer 'v1' in a function call}}
+  // expected-note@-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@-1 2 {{'setenv' call may invalidate the environment parameter of 'main'}}
+
+  *envp;
+  // expected-warning@-1 2 {{dereferencing an invalid pointer}}
+  // expected-note@-2 2 {{dereferencing an invalid pointer}}
+}