[analyzer] Consolidate the va_list checkers #156682
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the analyzer had an undocumented top-level checker group called `valist` which offered several checkers to detect use of uninitialized `va_list` objects and leaks of `va_list`s. As the responsibilities of these checkers were messily intertwined and `va_list` is a rarely used language feature, this commit simplifies the situation by consolidating these checkers into a single checker which will be called `security.VAList`. Note that I'm choosing the capitalization `VAList` to be consistent with the example of the AST node type `VAArgExpr`. I updated many variable names to ensure that `ValistChecker.cpp` uses this spelling everywhere (in CamelCased names). I'm planning to rename `ValistChecker.cpp` to `VAListChecker.cpp` in a follow-up commit. This commit also adds documentation for this checker in checkers.rst. Among the test files I preserved the existing separation but I eliminated some duplicated cases now that there is no way to separately enable the old sub-checkers. For the background of this change see also the discourse thread https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) ChangesPreviously the analyzer had an undocumented top-level checker group called As the responsibilities of these checkers were messily intertwined and Note that I'm choosing the capitalization This commit also adds documentation for this checker in checkers.rst. Among the test files I preserved the existing separation but I eliminated some duplicated cases now that there is no way to separately enable the old sub-checkers. For the background of this change see also the discourse thread https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3 Patch is 24.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/156682.diff 7 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index b2effadacf9f1..ab2bbfaa43738 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1859,6 +1859,32 @@ this) and always check the return value of these calls.
This check corresponds to SEI CERT Rule `POS36-C <https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges>`_.
+.. _security-VAList:
+
+security.VAList (C, C++)
+""""""""""""""""""""""""
+Reports use of uninitialized (or already released) ``va_list`` objects and
+situations where a ``va_start`` call is not followed by ``va_end``.
+
+Report out of bounds access to memory that is before the start or after the end
+of the accessed region (array, heap-allocated region, string literal etc.).
+This usually means incorrect indexing, but the checker also detects access via
+the operators ``*`` and ``->``.
+
+.. code-block:: c
+
+ int test_use_after_release(int x, ...) {
+ va_list va;
+ va_start(va, x);
+ va_end(va, x);
+ return va_arg(va, int); // warn: va is uninitialized
+ }
+
+ int test_leak(int x, ...) {
+ va_list va;
+ va_start(va, x);
+ } // warn: va is never released
+
.. _unix-checkers:
unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 73f702de581d9..e7468772b7885 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -64,8 +64,6 @@ def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, ParentPackage<Alpha>;
def CplusplusOptIn : Package<"cplusplus">, ParentPackage<OptIn>;
-def Valist : Package<"valist">;
-
def DeadCode : Package<"deadcode">;
def DeadCodeAlpha : Package<"deadcode">, ParentPackage<Alpha>;
@@ -803,35 +801,6 @@ def SmartPtrChecker: Checker<"SmartPtr">,
} // end: "alpha.cplusplus"
-
-//===----------------------------------------------------------------------===//
-// Valist checkers.
-//===----------------------------------------------------------------------===//
-
-let ParentPackage = Valist in {
-
-def ValistBase : Checker<"ValistBase">,
- HelpText<"Gathers information about va_lists.">,
- Documentation<NotDocumented>,
- Hidden;
-
-def UninitializedChecker : Checker<"Uninitialized">,
- HelpText<"Check for usages of uninitialized (or already released) va_lists.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-def UnterminatedChecker : Checker<"Unterminated">,
- HelpText<"Check for va_lists which are not released by a va_end call.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-def CopyToSelfChecker : Checker<"CopyToSelf">,
- HelpText<"Check for va_lists which are copied onto itself.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-} // end : "valist"
-
//===----------------------------------------------------------------------===//
// Deadcode checkers.
//===----------------------------------------------------------------------===//
@@ -1006,6 +975,10 @@ let ParentPackage = Security in {
"'setuid(getuid())' (CERT: POS36-C)">,
Documentation<HasDocumentation>;
+ def VAListChecker : Checker<"VAList">,
+ HelpText<"Warn on misuse of va_list objects">,
+ Documentation<HasDocumentation>;
+
} // end "security"
let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
index bd2f88c7b1bcc..2355abdaa060c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -27,28 +27,21 @@ REGISTER_SET_WITH_PROGRAMSTATE(InitializedVALists, const MemRegion *)
namespace {
typedef SmallVector<const MemRegion *, 2> RegionVector;
-class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
+class VAListChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
check::DeadSymbols> {
- mutable std::unique_ptr<BugType> BT_leakedvalist, BT_uninitaccess;
+ const BugType LeakBug{this, "Leaked va_list", categories::MemoryError,
+ /*SuppressOnSink=*/true};
+ const BugType UninitAccessBug{this, "Uninitialized va_list",
+ categories::MemoryError};
struct VAListAccepter {
CallDescription Func;
- int VAListPos;
+ int ParamIndex;
};
static const SmallVector<VAListAccepter, 15> VAListAccepters;
static const CallDescription VaStart, VaEnd, VaCopy;
public:
- enum CheckKind {
- CK_Uninitialized,
- CK_Unterminated,
- CK_CopyToSelf,
- CK_NumCheckKinds
- };
-
- bool ChecksEnabled[CK_NumCheckKinds] = {false};
- CheckerNameRef CheckNames[CK_NumCheckKinds];
-
void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
@@ -61,17 +54,16 @@ class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg,
CheckerContext &C) const;
- void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1,
- StringRef Msg2, CheckerContext &C, ExplodedNode *N,
- bool ReportUninit = false) const;
+ void reportLeaked(const RegionVector &Leaked, StringRef Msg1, StringRef Msg2,
+ CheckerContext &C, ExplodedNode *N) const;
void checkVAListStartCall(const CallEvent &Call, CheckerContext &C,
bool IsCopy) const;
void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const;
- class ValistBugVisitor : public BugReporterVisitor {
+ class VAListBugVisitor : public BugReporterVisitor {
public:
- ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false)
+ VAListBugVisitor(const MemRegion *Reg, bool IsLeak = false)
: Reg(Reg), IsLeak(IsLeak) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int X = 0;
@@ -99,8 +91,8 @@ class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
};
};
-const SmallVector<ValistChecker::VAListAccepter, 15>
- ValistChecker::VAListAccepters = {{{CDM::CLibrary, {"vfprintf"}, 3}, 2},
+const SmallVector<VAListChecker::VAListAccepter, 15>
+ VAListChecker::VAListAccepters = {{{CDM::CLibrary, {"vfprintf"}, 3}, 2},
{{CDM::CLibrary, {"vfscanf"}, 3}, 2},
{{CDM::CLibrary, {"vprintf"}, 2}, 1},
{{CDM::CLibrary, {"vscanf"}, 2}, 1},
@@ -116,14 +108,14 @@ const SmallVector<ValistChecker::VAListAccepter, 15>
// vsnprintf, vsprintf has no wide version
{{CDM::CLibrary, {"vswscanf"}, 3}, 2}};
-const CallDescription ValistChecker::VaStart(CDM::CLibrary,
+const CallDescription VAListChecker::VaStart(CDM::CLibrary,
{"__builtin_va_start"}, /*Args=*/2,
/*Params=*/1),
- ValistChecker::VaCopy(CDM::CLibrary, {"__builtin_va_copy"}, 2),
- ValistChecker::VaEnd(CDM::CLibrary, {"__builtin_va_end"}, 1);
+ VAListChecker::VaCopy(CDM::CLibrary, {"__builtin_va_copy"}, 2),
+ VAListChecker::VaEnd(CDM::CLibrary, {"__builtin_va_end"}, 1);
} // end anonymous namespace
-void ValistChecker::checkPreCall(const CallEvent &Call,
+void VAListChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
if (VaStart.matches(Call))
checkVAListStartCall(Call, C, false);
@@ -137,8 +129,8 @@ void ValistChecker::checkPreCall(const CallEvent &Call,
continue;
bool Symbolic;
const MemRegion *VAList =
- getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos),
- Call.getArgExpr(FuncInfo.VAListPos), Symbolic, C);
+ getVAListAsRegion(Call.getArgSVal(FuncInfo.ParamIndex),
+ Call.getArgExpr(FuncInfo.ParamIndex), Symbolic, C);
if (!VAList)
return;
@@ -159,17 +151,17 @@ void ValistChecker::checkPreCall(const CallEvent &Call,
}
}
-const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E,
+const MemRegion *VAListChecker::getVAListAsRegion(SVal SV, const Expr *E,
bool &IsSymbolic,
CheckerContext &C) const {
const MemRegion *Reg = SV.getAsRegion();
if (!Reg)
return nullptr;
// TODO: In the future this should be abstracted away by the analyzer.
- bool VaListModelledAsArray = false;
+ bool VAListModelledAsArray = false;
if (const auto *Cast = dyn_cast<CastExpr>(E)) {
QualType Ty = Cast->getType();
- VaListModelledAsArray =
+ VAListModelledAsArray =
Ty->isPointerType() && Ty->getPointeeType()->isRecordType();
}
if (const auto *DeclReg = Reg->getAs<DeclRegion>()) {
@@ -179,17 +171,16 @@ const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E,
IsSymbolic = Reg && Reg->getBaseRegion()->getAs<SymbolicRegion>();
// Some VarRegion based VA lists reach here as ElementRegions.
const auto *EReg = dyn_cast_or_null<ElementRegion>(Reg);
- return (EReg && VaListModelledAsArray) ? EReg->getSuperRegion() : Reg;
+ return (EReg && VAListModelledAsArray) ? EReg->getSuperRegion() : Reg;
}
-void ValistChecker::checkPreStmt(const VAArgExpr *VAA,
+void VAListChecker::checkPreStmt(const VAArgExpr *VAA,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
- const Expr *VASubExpr = VAA->getSubExpr();
- SVal VAListSVal = C.getSVal(VASubExpr);
+ const Expr *ArgExpr = VAA->getSubExpr();
+ SVal VAListSVal = C.getSVal(ArgExpr);
bool Symbolic;
- const MemRegion *VAList =
- getVAListAsRegion(VAListSVal, VASubExpr, Symbolic, C);
+ const MemRegion *VAList = getVAListAsRegion(VAListSVal, ArgExpr, Symbolic, C);
if (!VAList)
return;
if (Symbolic)
@@ -199,20 +190,19 @@ void ValistChecker::checkPreStmt(const VAArgExpr *VAA,
VAList, "va_arg() is called on an uninitialized va_list", C);
}
-void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
+void VAListChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
- InitializedVAListsTy TrackedVALists = State->get<InitializedVALists>();
- RegionVector LeakedVALists;
- for (auto Reg : TrackedVALists) {
+ InitializedVAListsTy Tracked = State->get<InitializedVALists>();
+ RegionVector Leaked;
+ for (const MemRegion *Reg : Tracked) {
if (SR.isLiveRegion(Reg))
continue;
- LeakedVALists.push_back(Reg);
+ Leaked.push_back(Reg);
State = State->remove<InitializedVALists>(Reg);
}
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C,
- N);
+ reportLeaked(Leaked, "Initialized va_list", " is leaked", C, N);
}
// This function traverses the exploded graph backwards and finds the node where
@@ -220,7 +210,7 @@ void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
// It is not likely that there are several different va_lists that belongs to
// different stack frames, so that case is not yet handled.
const ExplodedNode *
-ValistChecker::getStartCallSite(const ExplodedNode *N,
+VAListChecker::getStartCallSite(const ExplodedNode *N,
const MemRegion *Reg) const {
const LocationContext *LeakContext = N->getLocationContext();
const ExplodedNode *StartCallNode = N;
@@ -244,42 +234,21 @@ ValistChecker::getStartCallSite(const ExplodedNode *N,
return StartCallNode;
}
-void ValistChecker::reportUninitializedAccess(const MemRegion *VAList,
+void VAListChecker::reportUninitializedAccess(const MemRegion *VAList,
StringRef Msg,
CheckerContext &C) const {
- if (!ChecksEnabled[CK_Uninitialized])
- return;
if (ExplodedNode *N = C.generateErrorNode()) {
- if (!BT_uninitaccess)
- BT_uninitaccess.reset(new BugType(CheckNames[CK_Uninitialized],
- "Uninitialized va_list",
- categories::MemoryError));
- auto R = std::make_unique<PathSensitiveBugReport>(*BT_uninitaccess, Msg, N);
+ auto R = std::make_unique<PathSensitiveBugReport>(UninitAccessBug, Msg, N);
R->markInteresting(VAList);
- R->addVisitor(std::make_unique<ValistBugVisitor>(VAList));
+ R->addVisitor(std::make_unique<VAListBugVisitor>(VAList));
C.emitReport(std::move(R));
}
}
-void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
- StringRef Msg1, StringRef Msg2,
- CheckerContext &C, ExplodedNode *N,
- bool ReportUninit) const {
- if (!(ChecksEnabled[CK_Unterminated] ||
- (ChecksEnabled[CK_Uninitialized] && ReportUninit)))
- return;
- for (auto Reg : LeakedVALists) {
- if (!BT_leakedvalist) {
- // FIXME: maybe creating a new check name for this type of bug is a better
- // solution.
- BT_leakedvalist.reset(new BugType(
- static_cast<StringRef>(CheckNames[CK_Unterminated]).empty()
- ? CheckNames[CK_Uninitialized]
- : CheckNames[CK_Unterminated],
- "Leaked va_list", categories::MemoryError,
- /*SuppressOnSink=*/true));
- }
-
+void VAListChecker::reportLeaked(const RegionVector &Leaked, StringRef Msg1,
+ StringRef Msg2, CheckerContext &C,
+ ExplodedNode *N) const {
+ for (const MemRegion *Reg : Leaked) {
const ExplodedNode *StartNode = getStartCallSite(N, Reg);
PathDiagnosticLocation LocUsedForUniqueing;
@@ -296,15 +265,15 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
OS << Msg2;
auto R = std::make_unique<PathSensitiveBugReport>(
- *BT_leakedvalist, OS.str(), N, LocUsedForUniqueing,
+ LeakBug, OS.str(), N, LocUsedForUniqueing,
StartNode->getLocationContext()->getDecl());
R->markInteresting(Reg);
- R->addVisitor(std::make_unique<ValistBugVisitor>(Reg, true));
+ R->addVisitor(std::make_unique<VAListBugVisitor>(Reg, true));
C.emitReport(std::move(R));
}
}
-void ValistChecker::checkVAListStartCall(const CallEvent &Call,
+void VAListChecker::checkVAListStartCall(const CallEvent &Call,
CheckerContext &C, bool IsCopy) const {
bool Symbolic;
const MemRegion *VAList =
@@ -318,20 +287,19 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
const MemRegion *Arg2 =
getVAListAsRegion(Call.getArgSVal(1), Call.getArgExpr(1), Symbolic, C);
if (Arg2) {
- if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) {
- RegionVector LeakedVALists{VAList};
+ if (VAList == Arg2) {
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "va_list",
- " is copied onto itself", C, N, true);
+ reportLeaked(Leaked, "va_list", " is copied onto itself", C, N);
return;
- } else if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) {
+ }
+ if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) {
if (State->contains<InitializedVALists>(VAList)) {
State = State->remove<InitializedVALists>(VAList);
- RegionVector LeakedVALists{VAList};
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list",
- " is overwritten by an uninitialized one", C, N,
- true);
+ reportLeaked(Leaked, "Initialized va_list",
+ " is overwritten by an uninitialized one", C, N);
} else {
reportUninitializedAccess(Arg2, "Uninitialized va_list is copied", C);
}
@@ -340,10 +308,10 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
}
}
if (State->contains<InitializedVALists>(VAList)) {
- RegionVector LeakedVALists{VAList};
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list",
- " is initialized again", C, N);
+ reportLeaked(Leaked, "Initialized va_list", " is initialized again", C,
+ N);
return;
}
@@ -351,7 +319,7 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
C.addTransition(State);
}
-void ValistChecker::checkVAListEndCall(const CallEvent &Call,
+void VAListChecker::checkVAListEndCall(const CallEvent &Call,
CheckerContext &C) const {
bool Symbolic;
const MemRegion *VAList =
@@ -374,7 +342,7 @@ void ValistChecker::checkVAListEndCall(const CallEvent &Call,
C.addTransition(State);
}
-PathDiagnosticPieceRef ValistChecker::ValistBugVisitor::VisitNode(
+PathDiagnosticPieceRef VAListChecker::VAListBugVisitor::VisitNode(
const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &) {
ProgramStateRef State = N->getState();
ProgramStateRef StatePrev = N->getFirstPred()->getState();
@@ -399,26 +367,8 @@ PathDiagnosticPieceRef ValistChecker::ValistBugVisitor::VisitNode(
return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true);
}
-void ento::registerValistBase(CheckerManager &mgr) {
- mgr.registerChecker<ValistChecker>();
-}
-
-bool ento::shouldRegisterValistBase(const CheckerManager &mgr) {
- return true;
+void ento::registerVAListChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<VAListChecker>();
}
-#define REGISTER_CHECKER(name) \
- void ento::register##name##Checker(CheckerManager &mgr) { \
- ValistChecker *checker = mgr.getChecker<ValistChecker>(); \
- checker->ChecksEnabled[ValistChecker::CK_##name] = true; \
- checker->CheckNames[ValistChecker::CK_##name] = \
- mgr.getCurrentCheckerName(); \
- } \
- \
- bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
- return true; \
- }
-
-REGISTER_CHECKER(Uninitialized)
-REGISTER_CHECKER(Unterminated)
-REGISTER_CHECKER(CopyToSelf)
+bool ento::shouldRegisterVAListChecker(const CheckerManager &) { return true; }
diff --git a/clang/test/Analysis/valist-as-lazycompound.c b/clang/test/Analysis/valist-as-lazycompound.c
index 3218e03a9c657..35a137321a67f 100644
--- a/clang/test/Analysis/valist-as-lazycompound.c
+++ b/clang/test/Analysis/valist-as-lazycompound.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple gcc-linaro-arm-linux-gnueabihf -analyzer-checker=core,valist.Uninitialized,valist.CopyToSelf -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -triple gcc-linaro-arm-linux-gnueabihf -analyzer-checker=core,security.VAList -analyzer-output=text -verify %s
// expected-no-diagnostics
typedef unsigned int size_t;
diff --git a/clang/test/Analysis/valist-uninitialized-no-undef.c b/clang/test/Analysis/valist-uninitialized-no-undef.c
index 48631429be646..b8add29fc4803 100644
--- a/clang/test/Analysis/valist-uninitialized-no-undef.c
+++ b/clang/test/Analysis/valist-uninitialized-no-undef.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesPreviously the analyzer had an undocumented top-level checker group called As the responsibilities of these checkers were messily intertwined and Note that I'm choosing the capitalization This commit also adds documentation for this checker in checkers.rst. Among the test files I preserved the existing separation but I eliminated some duplicated cases now that there is no way to separately enable the old sub-checkers. For the background of this change see also the discourse thread https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3 Patch is 24.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/156682.diff 7 Files Affected:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index b2effadacf9f1..ab2bbfaa43738 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1859,6 +1859,32 @@ this) and always check the return value of these calls.
This check corresponds to SEI CERT Rule `POS36-C <https://wiki.sei.cmu.edu/confluence/display/c/POS36-C.+Observe+correct+revocation+order+while+relinquishing+privileges>`_.
+.. _security-VAList:
+
+security.VAList (C, C++)
+""""""""""""""""""""""""
+Reports use of uninitialized (or already released) ``va_list`` objects and
+situations where a ``va_start`` call is not followed by ``va_end``.
+
+Report out of bounds access to memory that is before the start or after the end
+of the accessed region (array, heap-allocated region, string literal etc.).
+This usually means incorrect indexing, but the checker also detects access via
+the operators ``*`` and ``->``.
+
+.. code-block:: c
+
+ int test_use_after_release(int x, ...) {
+ va_list va;
+ va_start(va, x);
+ va_end(va, x);
+ return va_arg(va, int); // warn: va is uninitialized
+ }
+
+ int test_leak(int x, ...) {
+ va_list va;
+ va_start(va, x);
+ } // warn: va is never released
+
.. _unix-checkers:
unix
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 73f702de581d9..e7468772b7885 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -64,8 +64,6 @@ def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, ParentPackage<Alpha>;
def CplusplusOptIn : Package<"cplusplus">, ParentPackage<OptIn>;
-def Valist : Package<"valist">;
-
def DeadCode : Package<"deadcode">;
def DeadCodeAlpha : Package<"deadcode">, ParentPackage<Alpha>;
@@ -803,35 +801,6 @@ def SmartPtrChecker: Checker<"SmartPtr">,
} // end: "alpha.cplusplus"
-
-//===----------------------------------------------------------------------===//
-// Valist checkers.
-//===----------------------------------------------------------------------===//
-
-let ParentPackage = Valist in {
-
-def ValistBase : Checker<"ValistBase">,
- HelpText<"Gathers information about va_lists.">,
- Documentation<NotDocumented>,
- Hidden;
-
-def UninitializedChecker : Checker<"Uninitialized">,
- HelpText<"Check for usages of uninitialized (or already released) va_lists.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-def UnterminatedChecker : Checker<"Unterminated">,
- HelpText<"Check for va_lists which are not released by a va_end call.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-def CopyToSelfChecker : Checker<"CopyToSelf">,
- HelpText<"Check for va_lists which are copied onto itself.">,
- Dependencies<[ValistBase]>,
- Documentation<NotDocumented>;
-
-} // end : "valist"
-
//===----------------------------------------------------------------------===//
// Deadcode checkers.
//===----------------------------------------------------------------------===//
@@ -1006,6 +975,10 @@ let ParentPackage = Security in {
"'setuid(getuid())' (CERT: POS36-C)">,
Documentation<HasDocumentation>;
+ def VAListChecker : Checker<"VAList">,
+ HelpText<"Warn on misuse of va_list objects">,
+ Documentation<HasDocumentation>;
+
} // end "security"
let ParentPackage = ENV in {
diff --git a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
index bd2f88c7b1bcc..2355abdaa060c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -27,28 +27,21 @@ REGISTER_SET_WITH_PROGRAMSTATE(InitializedVALists, const MemRegion *)
namespace {
typedef SmallVector<const MemRegion *, 2> RegionVector;
-class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
+class VAListChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
check::DeadSymbols> {
- mutable std::unique_ptr<BugType> BT_leakedvalist, BT_uninitaccess;
+ const BugType LeakBug{this, "Leaked va_list", categories::MemoryError,
+ /*SuppressOnSink=*/true};
+ const BugType UninitAccessBug{this, "Uninitialized va_list",
+ categories::MemoryError};
struct VAListAccepter {
CallDescription Func;
- int VAListPos;
+ int ParamIndex;
};
static const SmallVector<VAListAccepter, 15> VAListAccepters;
static const CallDescription VaStart, VaEnd, VaCopy;
public:
- enum CheckKind {
- CK_Uninitialized,
- CK_Unterminated,
- CK_CopyToSelf,
- CK_NumCheckKinds
- };
-
- bool ChecksEnabled[CK_NumCheckKinds] = {false};
- CheckerNameRef CheckNames[CK_NumCheckKinds];
-
void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
@@ -61,17 +54,16 @@ class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg,
CheckerContext &C) const;
- void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1,
- StringRef Msg2, CheckerContext &C, ExplodedNode *N,
- bool ReportUninit = false) const;
+ void reportLeaked(const RegionVector &Leaked, StringRef Msg1, StringRef Msg2,
+ CheckerContext &C, ExplodedNode *N) const;
void checkVAListStartCall(const CallEvent &Call, CheckerContext &C,
bool IsCopy) const;
void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const;
- class ValistBugVisitor : public BugReporterVisitor {
+ class VAListBugVisitor : public BugReporterVisitor {
public:
- ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false)
+ VAListBugVisitor(const MemRegion *Reg, bool IsLeak = false)
: Reg(Reg), IsLeak(IsLeak) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int X = 0;
@@ -99,8 +91,8 @@ class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
};
};
-const SmallVector<ValistChecker::VAListAccepter, 15>
- ValistChecker::VAListAccepters = {{{CDM::CLibrary, {"vfprintf"}, 3}, 2},
+const SmallVector<VAListChecker::VAListAccepter, 15>
+ VAListChecker::VAListAccepters = {{{CDM::CLibrary, {"vfprintf"}, 3}, 2},
{{CDM::CLibrary, {"vfscanf"}, 3}, 2},
{{CDM::CLibrary, {"vprintf"}, 2}, 1},
{{CDM::CLibrary, {"vscanf"}, 2}, 1},
@@ -116,14 +108,14 @@ const SmallVector<ValistChecker::VAListAccepter, 15>
// vsnprintf, vsprintf has no wide version
{{CDM::CLibrary, {"vswscanf"}, 3}, 2}};
-const CallDescription ValistChecker::VaStart(CDM::CLibrary,
+const CallDescription VAListChecker::VaStart(CDM::CLibrary,
{"__builtin_va_start"}, /*Args=*/2,
/*Params=*/1),
- ValistChecker::VaCopy(CDM::CLibrary, {"__builtin_va_copy"}, 2),
- ValistChecker::VaEnd(CDM::CLibrary, {"__builtin_va_end"}, 1);
+ VAListChecker::VaCopy(CDM::CLibrary, {"__builtin_va_copy"}, 2),
+ VAListChecker::VaEnd(CDM::CLibrary, {"__builtin_va_end"}, 1);
} // end anonymous namespace
-void ValistChecker::checkPreCall(const CallEvent &Call,
+void VAListChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
if (VaStart.matches(Call))
checkVAListStartCall(Call, C, false);
@@ -137,8 +129,8 @@ void ValistChecker::checkPreCall(const CallEvent &Call,
continue;
bool Symbolic;
const MemRegion *VAList =
- getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos),
- Call.getArgExpr(FuncInfo.VAListPos), Symbolic, C);
+ getVAListAsRegion(Call.getArgSVal(FuncInfo.ParamIndex),
+ Call.getArgExpr(FuncInfo.ParamIndex), Symbolic, C);
if (!VAList)
return;
@@ -159,17 +151,17 @@ void ValistChecker::checkPreCall(const CallEvent &Call,
}
}
-const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E,
+const MemRegion *VAListChecker::getVAListAsRegion(SVal SV, const Expr *E,
bool &IsSymbolic,
CheckerContext &C) const {
const MemRegion *Reg = SV.getAsRegion();
if (!Reg)
return nullptr;
// TODO: In the future this should be abstracted away by the analyzer.
- bool VaListModelledAsArray = false;
+ bool VAListModelledAsArray = false;
if (const auto *Cast = dyn_cast<CastExpr>(E)) {
QualType Ty = Cast->getType();
- VaListModelledAsArray =
+ VAListModelledAsArray =
Ty->isPointerType() && Ty->getPointeeType()->isRecordType();
}
if (const auto *DeclReg = Reg->getAs<DeclRegion>()) {
@@ -179,17 +171,16 @@ const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E,
IsSymbolic = Reg && Reg->getBaseRegion()->getAs<SymbolicRegion>();
// Some VarRegion based VA lists reach here as ElementRegions.
const auto *EReg = dyn_cast_or_null<ElementRegion>(Reg);
- return (EReg && VaListModelledAsArray) ? EReg->getSuperRegion() : Reg;
+ return (EReg && VAListModelledAsArray) ? EReg->getSuperRegion() : Reg;
}
-void ValistChecker::checkPreStmt(const VAArgExpr *VAA,
+void VAListChecker::checkPreStmt(const VAArgExpr *VAA,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
- const Expr *VASubExpr = VAA->getSubExpr();
- SVal VAListSVal = C.getSVal(VASubExpr);
+ const Expr *ArgExpr = VAA->getSubExpr();
+ SVal VAListSVal = C.getSVal(ArgExpr);
bool Symbolic;
- const MemRegion *VAList =
- getVAListAsRegion(VAListSVal, VASubExpr, Symbolic, C);
+ const MemRegion *VAList = getVAListAsRegion(VAListSVal, ArgExpr, Symbolic, C);
if (!VAList)
return;
if (Symbolic)
@@ -199,20 +190,19 @@ void ValistChecker::checkPreStmt(const VAArgExpr *VAA,
VAList, "va_arg() is called on an uninitialized va_list", C);
}
-void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
+void VAListChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
ProgramStateRef State = C.getState();
- InitializedVAListsTy TrackedVALists = State->get<InitializedVALists>();
- RegionVector LeakedVALists;
- for (auto Reg : TrackedVALists) {
+ InitializedVAListsTy Tracked = State->get<InitializedVALists>();
+ RegionVector Leaked;
+ for (const MemRegion *Reg : Tracked) {
if (SR.isLiveRegion(Reg))
continue;
- LeakedVALists.push_back(Reg);
+ Leaked.push_back(Reg);
State = State->remove<InitializedVALists>(Reg);
}
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C,
- N);
+ reportLeaked(Leaked, "Initialized va_list", " is leaked", C, N);
}
// This function traverses the exploded graph backwards and finds the node where
@@ -220,7 +210,7 @@ void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
// It is not likely that there are several different va_lists that belongs to
// different stack frames, so that case is not yet handled.
const ExplodedNode *
-ValistChecker::getStartCallSite(const ExplodedNode *N,
+VAListChecker::getStartCallSite(const ExplodedNode *N,
const MemRegion *Reg) const {
const LocationContext *LeakContext = N->getLocationContext();
const ExplodedNode *StartCallNode = N;
@@ -244,42 +234,21 @@ ValistChecker::getStartCallSite(const ExplodedNode *N,
return StartCallNode;
}
-void ValistChecker::reportUninitializedAccess(const MemRegion *VAList,
+void VAListChecker::reportUninitializedAccess(const MemRegion *VAList,
StringRef Msg,
CheckerContext &C) const {
- if (!ChecksEnabled[CK_Uninitialized])
- return;
if (ExplodedNode *N = C.generateErrorNode()) {
- if (!BT_uninitaccess)
- BT_uninitaccess.reset(new BugType(CheckNames[CK_Uninitialized],
- "Uninitialized va_list",
- categories::MemoryError));
- auto R = std::make_unique<PathSensitiveBugReport>(*BT_uninitaccess, Msg, N);
+ auto R = std::make_unique<PathSensitiveBugReport>(UninitAccessBug, Msg, N);
R->markInteresting(VAList);
- R->addVisitor(std::make_unique<ValistBugVisitor>(VAList));
+ R->addVisitor(std::make_unique<VAListBugVisitor>(VAList));
C.emitReport(std::move(R));
}
}
-void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
- StringRef Msg1, StringRef Msg2,
- CheckerContext &C, ExplodedNode *N,
- bool ReportUninit) const {
- if (!(ChecksEnabled[CK_Unterminated] ||
- (ChecksEnabled[CK_Uninitialized] && ReportUninit)))
- return;
- for (auto Reg : LeakedVALists) {
- if (!BT_leakedvalist) {
- // FIXME: maybe creating a new check name for this type of bug is a better
- // solution.
- BT_leakedvalist.reset(new BugType(
- static_cast<StringRef>(CheckNames[CK_Unterminated]).empty()
- ? CheckNames[CK_Uninitialized]
- : CheckNames[CK_Unterminated],
- "Leaked va_list", categories::MemoryError,
- /*SuppressOnSink=*/true));
- }
-
+void VAListChecker::reportLeaked(const RegionVector &Leaked, StringRef Msg1,
+ StringRef Msg2, CheckerContext &C,
+ ExplodedNode *N) const {
+ for (const MemRegion *Reg : Leaked) {
const ExplodedNode *StartNode = getStartCallSite(N, Reg);
PathDiagnosticLocation LocUsedForUniqueing;
@@ -296,15 +265,15 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
OS << Msg2;
auto R = std::make_unique<PathSensitiveBugReport>(
- *BT_leakedvalist, OS.str(), N, LocUsedForUniqueing,
+ LeakBug, OS.str(), N, LocUsedForUniqueing,
StartNode->getLocationContext()->getDecl());
R->markInteresting(Reg);
- R->addVisitor(std::make_unique<ValistBugVisitor>(Reg, true));
+ R->addVisitor(std::make_unique<VAListBugVisitor>(Reg, true));
C.emitReport(std::move(R));
}
}
-void ValistChecker::checkVAListStartCall(const CallEvent &Call,
+void VAListChecker::checkVAListStartCall(const CallEvent &Call,
CheckerContext &C, bool IsCopy) const {
bool Symbolic;
const MemRegion *VAList =
@@ -318,20 +287,19 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
const MemRegion *Arg2 =
getVAListAsRegion(Call.getArgSVal(1), Call.getArgExpr(1), Symbolic, C);
if (Arg2) {
- if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) {
- RegionVector LeakedVALists{VAList};
+ if (VAList == Arg2) {
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "va_list",
- " is copied onto itself", C, N, true);
+ reportLeaked(Leaked, "va_list", " is copied onto itself", C, N);
return;
- } else if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) {
+ }
+ if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) {
if (State->contains<InitializedVALists>(VAList)) {
State = State->remove<InitializedVALists>(VAList);
- RegionVector LeakedVALists{VAList};
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list",
- " is overwritten by an uninitialized one", C, N,
- true);
+ reportLeaked(Leaked, "Initialized va_list",
+ " is overwritten by an uninitialized one", C, N);
} else {
reportUninitializedAccess(Arg2, "Uninitialized va_list is copied", C);
}
@@ -340,10 +308,10 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
}
}
if (State->contains<InitializedVALists>(VAList)) {
- RegionVector LeakedVALists{VAList};
+ RegionVector Leaked{VAList};
if (ExplodedNode *N = C.addTransition(State))
- reportLeakedVALists(LeakedVALists, "Initialized va_list",
- " is initialized again", C, N);
+ reportLeaked(Leaked, "Initialized va_list", " is initialized again", C,
+ N);
return;
}
@@ -351,7 +319,7 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call,
C.addTransition(State);
}
-void ValistChecker::checkVAListEndCall(const CallEvent &Call,
+void VAListChecker::checkVAListEndCall(const CallEvent &Call,
CheckerContext &C) const {
bool Symbolic;
const MemRegion *VAList =
@@ -374,7 +342,7 @@ void ValistChecker::checkVAListEndCall(const CallEvent &Call,
C.addTransition(State);
}
-PathDiagnosticPieceRef ValistChecker::ValistBugVisitor::VisitNode(
+PathDiagnosticPieceRef VAListChecker::VAListBugVisitor::VisitNode(
const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &) {
ProgramStateRef State = N->getState();
ProgramStateRef StatePrev = N->getFirstPred()->getState();
@@ -399,26 +367,8 @@ PathDiagnosticPieceRef ValistChecker::ValistBugVisitor::VisitNode(
return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true);
}
-void ento::registerValistBase(CheckerManager &mgr) {
- mgr.registerChecker<ValistChecker>();
-}
-
-bool ento::shouldRegisterValistBase(const CheckerManager &mgr) {
- return true;
+void ento::registerVAListChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<VAListChecker>();
}
-#define REGISTER_CHECKER(name) \
- void ento::register##name##Checker(CheckerManager &mgr) { \
- ValistChecker *checker = mgr.getChecker<ValistChecker>(); \
- checker->ChecksEnabled[ValistChecker::CK_##name] = true; \
- checker->CheckNames[ValistChecker::CK_##name] = \
- mgr.getCurrentCheckerName(); \
- } \
- \
- bool ento::shouldRegister##name##Checker(const CheckerManager &mgr) { \
- return true; \
- }
-
-REGISTER_CHECKER(Uninitialized)
-REGISTER_CHECKER(Unterminated)
-REGISTER_CHECKER(CopyToSelf)
+bool ento::shouldRegisterVAListChecker(const CheckerManager &) { return true; }
diff --git a/clang/test/Analysis/valist-as-lazycompound.c b/clang/test/Analysis/valist-as-lazycompound.c
index 3218e03a9c657..35a137321a67f 100644
--- a/clang/test/Analysis/valist-as-lazycompound.c
+++ b/clang/test/Analysis/valist-as-lazycompound.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple gcc-linaro-arm-linux-gnueabihf -analyzer-checker=core,valist.Uninitialized,valist.CopyToSelf -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -triple gcc-linaro-arm-linux-gnueabihf -analyzer-checker=core,security.VAList -analyzer-output=text -verify %s
// expected-no-diagnostics
typedef unsigned int size_t;
diff --git a/clang/test/Analysis/valist-uninitialized-no-undef.c b/clang/test/Analysis/valist-uninitialized-no-undef.c
index 48631429be646..b8add29fc4803 100644
--- a/clang/test/Analysis/valist-uninitialized-no-undef.c
+++ b/clang/test/Analysis/valist-uninitialized-no-undef.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64...
[truncated]
|
steakhal
approved these changes
Sep 4, 2025
clang/docs/analyzer/checkers.rst
Outdated
| int test_use_after_release(int x, ...) { | ||
| va_list va; | ||
| va_start(va, x); | ||
| va_end(va, x); |
There was a problem hiding this comment.
Suggested change
| va_end(va, x); | |
| va_end(va); |
There was a problem hiding this comment.
Oops, thanks for noticing!
clang/docs/analyzer/checkers.rst
Outdated
| int test_leak(int x, ...) { | ||
| va_list va; | ||
| va_start(va, x); | ||
| } // warn: va is never released |
There was a problem hiding this comment.
The actual diag phrases this as Initialized va_list 'va' is leaked, so uses the phrase leaked instead of released.
There was a problem hiding this comment.
Good point, updated to "va is leaked".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the analyzer had an undocumented top-level checker group called
valistwhich offered several checkers to detect use of uninitializedva_listobjects and leaks ofva_lists.As the responsibilities of these checkers were messily intertwined and
va_listis a rarely used language feature, this commit simplifies the situation by consolidating these checkers into a single checker which will be calledsecurity.VAList.Note that I'm choosing the capitalization
VAListto be consistent with the example of the AST node typeVAArgExpr. I updated many variable names to ensure thatValistChecker.cppuses this spelling everywhere (in CamelCased names). I'm planning to renameValistChecker.cpptoVAListChecker.cppin a follow-up commit.This commit also adds documentation for this checker in checkers.rst.
Among the test files I preserved the existing separation but I eliminated some duplicated cases now that there is no way to separately enable the old sub-checkers.
For the background of this change see also the discourse thread https://discourse.llvm.org/t/clean-up-valist-checkers/85277/3