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.fu...@sigmatechnology.se> Date: Fri, 8 Sep 2023 14:20:00 +0200 Subject: [PATCH 01/11] [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@-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; @@ -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@-1{{use of invalidated pointer 'tmpvar' in a function call}} + // pedantic-warning@-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@-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}} +} From 2f8bc383acfd2d8e514884441c8fa74d1798ece4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fu...@sigmatechnology.se> Date: Thu, 21 Sep 2023 16:03:46 +0200 Subject: [PATCH 02/11] 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.fu...@sigmatechnology.se> Date: Thu, 21 Sep 2023 16:04:13 +0200 Subject: [PATCH 03/11] 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.fu...@sigmatechnology.se> Date: Thu, 21 Sep 2023 16:36:06 +0200 Subject: [PATCH 04/11] 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.fu...@sigmatechnology.se> Date: Mon, 25 Sep 2023 10:01:31 +0200 Subject: [PATCH 05/11] 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.fu...@sigmatechnology.se> Date: Tue, 26 Sep 2023 11:00:06 +0200 Subject: [PATCH 06/11] 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.fu...@sigmatechnology.se> Date: Mon, 2 Oct 2023 10:29:53 +0200 Subject: [PATCH 07/11] 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.fu...@sigmatechnology.se> Date: Tue, 3 Oct 2023 12:07:53 +0200 Subject: [PATCH 08/11] 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@-1{{'setenv' call may invalidate the environment returned by getenv}} + // 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}} From d15e570f37f6fb321daf3742231408585f577137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fu...@sigmatechnology.se> Date: Mon, 9 Oct 2023 10:00:36 +0200 Subject: [PATCH 09/11] Add test to ensure no extra notes are emitted for multiple invalidation points --- .../Checkers/cert/InvalidPtrChecker.cpp | 4 +++- clang/test/Analysis/invalid-ptr-checker.c | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp index 543282fabd3793c..9b9019ed50f7b44 100644 --- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp @@ -142,7 +142,9 @@ const NoteTag *InvalidPtrChecker::createEnvInvalidationNote( Out << ", and " << InvalidLocationNames[1]; // Mark all regions that were interesting before as NOT interesting now - // to avoid extra notes coming from other checkers. + // to avoid extra notes coming from invalidation points higher up the + // bugpath. This ensures, that only the last invalidation point is marked + // with a note tag. if (IsInterestingForInvalidation(MainRegion)) BR.markNotInteresting(MainRegion); for (const MemRegion *GetenvRegion : GetenvRegions) diff --git a/clang/test/Analysis/invalid-ptr-checker.c b/clang/test/Analysis/invalid-ptr-checker.c index 4b257f9e78003fc..e8ffee7fb479dc9 100644 --- a/clang/test/Analysis/invalid-ptr-checker.c +++ b/clang/test/Analysis/invalid-ptr-checker.c @@ -48,3 +48,16 @@ int main(int argc, const char *argv[], const char *envp[]) { // expected-warning@-1 2 {{dereferencing an invalid pointer}} // expected-note@-2 2 {{dereferencing an invalid pointer}} } + +void multiple_invalidation_no_duplicate_notes(void) { + char *v1 = getenv("VAR1"); + + setenv("VAR2", "...", 1); // no note here + + setenv("VAR3", "...", 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}} +} From 39ed2178944539aaa8e221ac11a8d6c7ec93675e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fu...@sigmatechnology.se> Date: Mon, 9 Oct 2023 10:15:03 +0200 Subject: [PATCH 10/11] Make testcase less verbose by using '-verify=expected,pedantic' --- clang/test/Analysis/cert/env34-c-cert-examples.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/test/Analysis/cert/env34-c-cert-examples.c b/clang/test/Analysis/cert/env34-c-cert-examples.c index 7e5b9be3cba6bb0..6d6659e55d86b93 100644 --- a/clang/test/Analysis/cert/env34-c-cert-examples.c +++ b/clang/test/Analysis/cert/env34-c-cert-examples.c @@ -13,7 +13,7 @@ // 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 +// RUN: -verify=expected,pedantic -Wno-unused %s #include "../Inputs/system-header-simulator.h" char *getenv(const char *name); @@ -39,7 +39,6 @@ void incorrect_usage_setenv_getenv_invalidation(void) { 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}} } } From 18eb3e8ca7e6f0edb5c2c703ca6d12acf7629dcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Endre=20F=C3=BCl=C3=B6p?= <endre.fu...@sigmatechnology.se> Date: Tue, 17 Oct 2023 14:26:04 +0200 Subject: [PATCH 11/11] Refactor lambda, inline interesting handling --- .../Checkers/cert/InvalidPtrChecker.cpp | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp index 9b9019ed50f7b44..f00360f7e43b5d4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp @@ -123,33 +123,37 @@ const NoteTag *InvalidPtrChecker::createEnvInvalidationNote( 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); - }; + // Only handle the BugType of this checker. + if (&BR.getBugType() != &InvalidPtrBugType) + return; - // Craft the note tag message. + // Mark all regions that were interesting before as NOT interesting now + // to avoid extra notes coming from invalidation points higher up the + // bugpath. This ensures, that only the last invalidation point is marked + // with a note tag. llvm::SmallVector<std::string, 2> InvalidLocationNames; - if (IsInterestingForInvalidation(MainRegion)) { + if (BR.isInteresting(MainRegion)) { + BR.markNotInteresting(MainRegion); InvalidLocationNames.push_back("the environment parameter of 'main'"); } - if (llvm::any_of(GetenvRegions, IsInterestingForInvalidation)) - InvalidLocationNames.push_back("the environment returned by 'getenv'"); + bool InterestingGetenvFound = false; + for (const MemRegion *MR : GetenvRegions) { + if (BR.isInteresting(MR)) { + BR.markNotInteresting(MR); + if (!InterestingGetenvFound) { + InterestingGetenvFound = true; + InvalidLocationNames.push_back( + "the environment returned by 'getenv'"); + } + } + } + // Emit note tag message. 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 invalidation points higher up the - // bugpath. This ensures, that only the last invalidation point is marked - // with a note tag. - if (IsInterestingForInvalidation(MainRegion)) - BR.markNotInteresting(MainRegion); - for (const MemRegion *GetenvRegion : GetenvRegions) - if (IsInterestingForInvalidation(GetenvRegion)) - BR.markNotInteresting(GetenvRegion); }); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
