[clang] [analyzer] Modernize, improve and promote chroot checker (PR #117791)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 28 23:22:00 PST 2024
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/117791
>From ed174c8b52880d4f89415eb3a72da13f355438d7 Mon Sep 17 00:00:00 2001
From: einvbri <vince.a.bridgers at ericsson.com>
Date: Mon, 25 Nov 2024 10:31:57 +0100
Subject: [PATCH 01/20] [analyzer] Modernize, improve and promote chroot
checker
This change modernizes, improves and promotes the chroot checker from
alpha to the Unix family of checkers. This checker covers the POS05
recommendations for use of chroot.
The improvements included modeling of a success or failure from chroot
and not falsely reporting a warning along an error path. This was made
possible through modernizing the checker to be flow sensitive.
---
clang/docs/ReleaseNotes.rst | 3 +
clang/docs/analyzer/checkers.rst | 30 ++--
.../clang/StaticAnalyzer/Checkers/Checkers.td | 8 +-
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 149 ++++++++++++++----
.../test/Analysis/analyzer-enabled-checkers.c | 1 +
clang/test/Analysis/chroot.c | 36 ++++-
clang/test/Analysis/show-checker-list.c | 9 --
...c-library-functions-arg-enabled-checkers.c | 1 +
8 files changed, 174 insertions(+), 63 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 6c40e48e2f49b3..292a41e127bfd0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -985,6 +985,9 @@ Moved checkers
original checkers were implemented only using AST matching and make more
sense as a single clang-tidy check.
+- The checker ``alpha.unix.Chroot`` was modernized, improved and moved from
+ alpha to a main Unix family checker.
+
.. _release-notes-sanitizers:
Sanitizers
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index f34b25cd04bd18..5149faa50f72cf 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1750,6 +1750,21 @@ Critical section handling functions modeled by this checker:
}
}
+.. _unix-Chroot:
+
+unix.Chroot (C)
+"""""""""""""""""""""
+Check improper use of chroot.
+
+.. code-block:: c
+
+ void f();
+
+ void test() {
+ chroot("/usr/local");
+ f(); // warn: no call of chdir("/") immediately after chroot
+ }
+
.. _unix-Errno:
unix.Errno (C)
@@ -3275,21 +3290,6 @@ SEI CERT checkers which tries to find errors based on their `C coding rules <htt
alpha.unix
^^^^^^^^^^
-.. _alpha-unix-Chroot:
-
-alpha.unix.Chroot (C)
-"""""""""""""""""""""
-Check improper use of chroot.
-
-.. code-block:: c
-
- void f();
-
- void test() {
- chroot("/usr/local");
- f(); // warn: no call of chdir("/") immediately after chroot
- }
-
.. _alpha-unix-PthreadLock:
alpha.unix.PthreadLock (C)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index b03e707d638742..c409d8cfabb1ef 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -598,14 +598,14 @@ def VforkChecker : Checker<"Vfork">,
HelpText<"Check for proper usage of vfork">,
Documentation<HasDocumentation>;
-} // end "unix"
-
-let ParentPackage = UnixAlpha in {
-
def ChrootChecker : Checker<"Chroot">,
HelpText<"Check improper use of chroot">,
Documentation<HasDocumentation>;
+} // end "unix"
+
+let ParentPackage = UnixAlpha in {
+
def PthreadLockChecker : Checker<"PthreadLock">,
HelpText<"Simple lock -> unlock checker">,
Dependencies<[PthreadLockBase]>,
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 3a0a01c23de03e..d957ce9ed2ffc7 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/ASTContext.h"
+#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -24,21 +26,30 @@
using namespace clang;
using namespace ento;
-namespace {
-
// enum value that represent the jail state
-enum Kind { NO_CHROOT, ROOT_CHANGED, JAIL_ENTERED };
+enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
-bool isRootChanged(intptr_t k) { return k == ROOT_CHANGED; }
-//bool isJailEntered(intptr_t k) { return k == JAIL_ENTERED; }
+// Track chroot state changes for success, failure, state change
+// and "jail"
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
+
+// Track the call expression to chroot for accurate
+// warning messages
+REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootCall, const Expr *)
+
+namespace {
// This checker checks improper use of chroot.
-// The state transition:
+// The state transitions
+//
+// -> ROOT_CHANGE_FAILED
+// |
// NO_CHROOT ---chroot(path)--> ROOT_CHANGED ---chdir(/) --> JAIL_ENTERED
// | |
// ROOT_CHANGED<--chdir(..)-- JAIL_ENTERED<--chdir(..)--
// | |
// bug<--foo()-- JAIL_ENTERED<--foo()--
+//
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
// This bug refers to possibly break out of a chroot() jail.
const BugType BT_BreakJail{this, "Break out of jail"};
@@ -49,20 +60,17 @@ class ChrootChecker : public Checker<eval::Call, check::PreCall> {
public:
ChrootChecker() {}
- static void *getTag() {
- static int x;
- return &x;
- }
-
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
private:
void evalChroot(const CallEvent &Call, CheckerContext &C) const;
void evalChdir(const CallEvent &Call, CheckerContext &C) const;
-};
-} // end anonymous namespace
+ /// Searches for the ExplodedNode where chroot was called.
+ static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
+ CheckerContext &C);
+};
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
if (Chroot.matches(Call)) {
@@ -80,19 +88,53 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef state = C.getState();
ProgramStateManager &Mgr = state->getStateManager();
+ const TargetInfo &TI = C.getASTContext().getTargetInfo();
+ SValBuilder &SVB = C.getSValBuilder();
+ BasicValueFactory &BVF = SVB.getBasicValueFactory();
+ ConstraintManager &CM = Mgr.getConstraintManager();
- // Once encouter a chroot(), set the enum value ROOT_CHANGED directly in
- // the GDM.
- state = Mgr.addGDM(state, ChrootChecker::getTag(), (void*) ROOT_CHANGED);
- C.addTransition(state);
+ const QualType sIntTy = C.getASTContext().getIntTypeForBitwidth(
+ /*DestWidth=*/TI.getIntWidth(), /*Signed=*/true);
+
+ const Expr *ChrootCE = Call.getOriginExpr();
+ if (!ChrootCE)
+ return;
+ const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+
+ const LocationContext *LCtx = C.getLocationContext();
+ NonLoc RetVal =
+ C.getSValBuilder()
+ .conjureSymbolVal(nullptr, ChrootCE, LCtx, sIntTy, C.blockCount())
+ .castAs<NonLoc>();
+
+ ProgramStateRef StateChrootFailed, StateChrootSuccess;
+ std::tie(StateChrootFailed, StateChrootSuccess) = state->assume(RetVal);
+
+ const llvm::APSInt &Zero = BVF.getValue(0, sIntTy);
+ const llvm::APSInt &Minus1 = BVF.getValue(-1, sIntTy);
+
+ if (StateChrootFailed) {
+ StateChrootFailed = CM.assumeInclusiveRange(StateChrootFailed, RetVal,
+ Minus1, Minus1, true);
+ StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
+ StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
+ C.addTransition(StateChrootFailed->BindExpr(CE, LCtx, RetVal));
+ }
+
+ if (StateChrootSuccess) {
+ StateChrootSuccess =
+ CM.assumeInclusiveRange(StateChrootSuccess, RetVal, Zero, Zero, true);
+ StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
+ StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
+ C.addTransition(StateChrootSuccess->BindExpr(CE, LCtx, RetVal));
+ }
}
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef state = C.getState();
- ProgramStateManager &Mgr = state->getStateManager();
- // If there are no jail state in the GDM, just return.
- const void *k = state->FindGDM(ChrootChecker::getTag());
+ // If there are no jail state, just return.
+ const ChrootKind k = C.getState()->get<ChrootState>();
if (!k)
return;
@@ -104,15 +146,35 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
R = R->StripCasts();
if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
const StringLiteral* Str = StrRegion->getStringLiteral();
- if (Str->getString() == "/")
- state = Mgr.addGDM(state, ChrootChecker::getTag(),
- (void*) JAIL_ENTERED);
+ if (Str->getString() == "/") {
+ state = state->set<ChrootState>(JAIL_ENTERED);
+ }
}
}
C.addTransition(state);
}
+const ExplodedNode *ChrootChecker::getAcquisitionSite(const ExplodedNode *N,
+ CheckerContext &C) {
+ ProgramStateRef State = N->getState();
+ // When bug type is resource leak, exploded node N may not have state info
+ // for leaked file descriptor, but predecessor should have it.
+ if (!State->get<ChrootCall>())
+ N = N->getFirstPred();
+
+ const ExplodedNode *Pred = N;
+ while (N) {
+ State = N->getState();
+ if (!State->get<ChrootCall>())
+ return Pred;
+ Pred = N;
+ N = N->getFirstPred();
+ }
+
+ return nullptr;
+}
+
// Check the jail state before any function call except chroot and chdir().
void ChrootChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
@@ -121,17 +183,40 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
return;
// If jail state is ROOT_CHANGED, generate BugReport.
- void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
- if (k)
- if (isRootChanged((intptr_t) *k))
- if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
- constexpr llvm::StringLiteral Msg =
- "No call of chdir(\"/\") immediately after chroot";
- C.emitReport(
- std::make_unique<PathSensitiveBugReport>(BT_BreakJail, Msg, N));
- }
+ const ChrootKind k = C.getState()->get<ChrootState>();
+ if (k == ROOT_CHANGED) {
+ ExplodedNode *Err =
+ C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
+ if (!Err)
+ return;
+ const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
+
+ const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
+ assert(ChrootCallNode && "Could not find place of stream opening.");
+
+ PathDiagnosticLocation LocUsedForUniqueing;
+ if (const Stmt *ChrootStmt = ChrootCallNode->getStmtForDiagnostics())
+ LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+ ChrootStmt, C.getSourceManager(),
+ ChrootCallNode->getLocationContext());
+
+ std::unique_ptr<PathSensitiveBugReport> R =
+ std::make_unique<PathSensitiveBugReport>(
+ BT_BreakJail, "No call of chdir(\"/\") immediately after chroot",
+ Err, LocUsedForUniqueing,
+ ChrootCallNode->getLocationContext()->getDecl());
+
+ R->addNote("chroot called here",
+ PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
+ C.getSourceManager()),
+ {ChrootExpr->getSourceRange()});
+
+ C.emitReport(std::move(R));
+ }
}
+} // namespace
+
void ento::registerChrootChecker(CheckerManager &mgr) {
mgr.registerChecker<ChrootChecker>();
}
diff --git a/clang/test/Analysis/analyzer-enabled-checkers.c b/clang/test/Analysis/analyzer-enabled-checkers.c
index e605c62a66ad0e..78a5daab98b0c4 100644
--- a/clang/test/Analysis/analyzer-enabled-checkers.c
+++ b/clang/test/Analysis/analyzer-enabled-checkers.c
@@ -43,6 +43,7 @@
// CHECK-NEXT: security.insecureAPI.vfork
// CHECK-NEXT: unix.API
// CHECK-NEXT: unix.BlockInCriticalSection
+// CHECK-NEXT: unix.Chroot
// CHECK-NEXT: unix.cstring.CStringModeling
// CHECK-NEXT: unix.DynamicMemoryModeling
// CHECK-NEXT: unix.Errno
diff --git a/clang/test/Analysis/chroot.c b/clang/test/Analysis/chroot.c
index eb512c05f86f72..e682fa0a5019e7 100644
--- a/clang/test/Analysis/chroot.c
+++ b/clang/test/Analysis/chroot.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Chroot -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.Chroot -verify %s
extern int chroot(const char* path);
extern int chdir(const char* path);
@@ -7,7 +7,7 @@ void foo(void) {
}
void f1(void) {
- chroot("/usr/local"); // root changed.
+ chroot("/usr/local"); // expected-note {{chroot called here}}
foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
}
@@ -18,7 +18,37 @@ void f2(void) {
}
void f3(void) {
- chroot("/usr/local"); // root changed.
+ chroot("/usr/local"); // expected-note {{chroot called here}}
chdir("../"); // change working directory, still out of jail.
foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
}
+
+void f4(void) {
+ if (chroot("/usr/local") == 0) {
+ chdir("../"); // change working directory, still out of jail.
+ }
+}
+
+void f5(void) {
+ int v = chroot("/usr/local");
+ if (v == -1) {
+ foo(); // no warning, chroot failed
+ chdir("../"); // change working directory, still out of jail.
+ }
+}
+
+void f6(void) {
+ if (chroot("/usr/local") == -1) {
+ chdir("../"); // change working directory, still out of jail.
+ }
+}
+
+void f7(void) {
+ int v = chroot("/usr/local"); // expected-note {{chroot called here}}
+ if (v == -1) {
+ foo(); // no warning, chroot failed
+ chdir("../"); // change working directory, still out of jail.
+ } else {
+ foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
+ }
+}
diff --git a/clang/test/Analysis/show-checker-list.c b/clang/test/Analysis/show-checker-list.c
index 3d354c338b9b3a..99d7d4ac8dae0b 100644
--- a/clang/test/Analysis/show-checker-list.c
+++ b/clang/test/Analysis/show-checker-list.c
@@ -24,10 +24,6 @@
// RUN: -analyzer-checker-help-developer \
// RUN: 2>&1 | FileCheck %s -check-prefix=CHECK-STABLE-ALPHA-DEVELOPER
-// CHECK-STABLE-NOT: alpha.unix.Chroot
-// CHECK-DEVELOPER-NOT: alpha.unix.Chroot
-// CHECK-ALPHA: alpha.unix.Chroot
-
// Note that alpha.cplusplus.IteratorModeling is not only an alpha, but also a
// hidden checker. In this case, we'd only like to see it in the developer list.
// CHECK-ALPHA-NOT: alpha.cplusplus.IteratorModeling
@@ -42,10 +38,6 @@
// CHECK-ALPHA-NOT: debug.ConfigDumper
-// CHECK-STABLE-ALPHA: alpha.unix.Chroot
-// CHECK-DEVELOPER-ALPHA: alpha.unix.Chroot
-// CHECK-STABLE-DEVELOPER-NOT: alpha.unix.Chroot
-
// CHECK-STABLE-ALPHA: core.DivideZero
// CHECK-DEVELOPER-ALPHA-NOT: core.DivideZero
// CHECK-STABLE-DEVELOPER: core.DivideZero
@@ -55,6 +47,5 @@
// CHECK-STABLE-DEVELOPER: debug.ConfigDumper
-// CHECK-STABLE-ALPHA-DEVELOPER: alpha.unix.Chroot
// CHECK-STABLE-ALPHA-DEVELOPER: core.DivideZero
// CHECK-STABLE-ALPHA-DEVELOPER: debug.ConfigDumper
diff --git a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
index 345a4e8f44efd1..dad15806197d88 100644
--- a/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ b/clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -51,6 +51,7 @@
// CHECK-NEXT: security.insecureAPI.vfork
// CHECK-NEXT: unix.API
// CHECK-NEXT: unix.BlockInCriticalSection
+// CHECK-NEXT: unix.Chroot
// CHECK-NEXT: unix.cstring.CStringModeling
// CHECK-NEXT: unix.DynamicMemoryModeling
// CHECK-NEXT: unix.Errno
>From 7e513cd9c5cbc42a58d8261fd09879853d0b95a4 Mon Sep 17 00:00:00 2001
From: einvbri <vince.a.bridgers at ericsson.com>
Date: Wed, 27 Nov 2024 15:39:36 +0100
Subject: [PATCH 02/20] [analyzer] Modernize, improve and promote chroot
checker
Updates #1 for original MR per review comments
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860209751
* Update Release notes with specific gh# addressed, and why these changes
are made.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860210481
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860215842
* Update checkers.rst documentation on Chroot checker per review comments.
Describe what the checker does and reference to POS05-C.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860233866
* Update use of raw string literals per review comments.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860245965
* Use simpler form of getting IntTy per review comments.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860248233
* Simplify invocation of conjureSymbolVal per review comments.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860250243
* Simplify invocation of state->assume() per review comments.
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860253486
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860256509
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860268803
* Remove use of CM, assumeInclusiveRange(), bind return values
directly per review comments, simplifying modeling of chroot()
behavior.
---
clang/docs/ReleaseNotes.rst | 11 +++++--
clang/docs/analyzer/checkers.rst | 14 +++++++--
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 31 +++++++------------
3 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 292a41e127bfd0..5e5ef571ac67fd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -985,8 +985,15 @@ Moved checkers
original checkers were implemented only using AST matching and make more
sense as a single clang-tidy check.
-- The checker ``alpha.unix.Chroot`` was modernized, improved and moved from
- alpha to a main Unix family checker.
+- The checker ``alpha.unix.Chroot`` was modernized, improved and moved to
+ ``unix.Chroot``. Testing was done on open source projects that use chroot(),
+ and false issues addressed in the improvements based on real use cases. Open
+ source projects used for testing include nsjail, lxroot, dive and ruri.
+ This checker conforms to SEI Cert C recommendation `POS05-C. Limit access to
+ files by creating a jail
+ <https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
+ Fixes (#GH34697).
+ (#GH117791) [Documentation](https://clang.llvm.org/docs/analyzer/checkers.html#unix-chroot-c).
.. _release-notes-sanitizers:
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 5149faa50f72cf..f3dde66f78db88 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1753,8 +1753,12 @@ Critical section handling functions modeled by this checker:
.. _unix-Chroot:
unix.Chroot (C)
-"""""""""""""""""""""
-Check improper use of chroot.
+"""""""""""""""
+Check improper use of chroot described by SEI Cert C recommendation `POS05-C.
+Limit access to files by creating a jail
+<https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
+The checker finds usage patterns where chdir() is not called immediately
+after a call to chroot().
.. code-block:: c
@@ -1765,6 +1769,12 @@ Check improper use of chroot.
f(); // warn: no call of chdir("/") immediately after chroot
}
+ void test() {
+ chroot("/usr/local");
+ chdir("/"); // no warning
+ f();
+ }
+
.. _unix-Errno:
unix.Errno (C)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index d957ce9ed2ffc7..206562a8cdcf67 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -87,14 +87,10 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef state = C.getState();
- ProgramStateManager &Mgr = state->getStateManager();
- const TargetInfo &TI = C.getASTContext().getTargetInfo();
SValBuilder &SVB = C.getSValBuilder();
BasicValueFactory &BVF = SVB.getBasicValueFactory();
- ConstraintManager &CM = Mgr.getConstraintManager();
- const QualType sIntTy = C.getASTContext().getIntTypeForBitwidth(
- /*DestWidth=*/TI.getIntWidth(), /*Signed=*/true);
+ const QualType IntTy = C.getASTContext().IntTy;
const Expr *ChrootCE = Call.getOriginExpr();
if (!ChrootCE)
@@ -103,30 +99,24 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
const LocationContext *LCtx = C.getLocationContext();
NonLoc RetVal =
- C.getSValBuilder()
- .conjureSymbolVal(nullptr, ChrootCE, LCtx, sIntTy, C.blockCount())
+ SVB.conjureSymbolVal(/*SymbolTag=*/nullptr, ChrootCE, LCtx, IntTy, C.blockCount())
.castAs<NonLoc>();
- ProgramStateRef StateChrootFailed, StateChrootSuccess;
- std::tie(StateChrootFailed, StateChrootSuccess) = state->assume(RetVal);
+ auto [StateChrootFailed, StateChrootSuccess] = state->assume(RetVal);
- const llvm::APSInt &Zero = BVF.getValue(0, sIntTy);
- const llvm::APSInt &Minus1 = BVF.getValue(-1, sIntTy);
+ const llvm::APSInt &Zero = BVF.getValue(0, IntTy);
+ const llvm::APSInt &Minus1 = BVF.getValue(-1, IntTy);
if (StateChrootFailed) {
- StateChrootFailed = CM.assumeInclusiveRange(StateChrootFailed, RetVal,
- Minus1, Minus1, true);
StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
- C.addTransition(StateChrootFailed->BindExpr(CE, LCtx, RetVal));
+ C.addTransition(StateChrootFailed->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1}));
}
if (StateChrootSuccess) {
- StateChrootSuccess =
- CM.assumeInclusiveRange(StateChrootSuccess, RetVal, Zero, Zero, true);
StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
- C.addTransition(StateChrootSuccess->BindExpr(CE, LCtx, RetVal));
+ C.addTransition(StateChrootSuccess->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero}));
}
}
@@ -158,8 +148,9 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
const ExplodedNode *ChrootChecker::getAcquisitionSite(const ExplodedNode *N,
CheckerContext &C) {
ProgramStateRef State = N->getState();
- // When bug type is resource leak, exploded node N may not have state info
- // for leaked file descriptor, but predecessor should have it.
+ // When a chroot() issue is found, the current exploded node may not
+ // have state info for where chroot() was called for the bug report but
+ // a predecessor should have it.
if (!State->get<ChrootCall>())
N = N->getFirstPred();
@@ -202,7 +193,7 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
std::unique_ptr<PathSensitiveBugReport> R =
std::make_unique<PathSensitiveBugReport>(
- BT_BreakJail, "No call of chdir(\"/\") immediately after chroot",
+ BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
Err, LocUsedForUniqueing,
ChrootCallNode->getLocationContext()->getDecl());
>From 65d8ea247246bb941eb68ee4827f1b2ba424e768 Mon Sep 17 00:00:00 2001
From: einvbri <vince.a.bridgers at ericsson.com>
Date: Wed, 27 Nov 2024 16:07:13 +0100
Subject: [PATCH 03/20] [analyzer] Modernize, improve and promote chroot
checker
Update #2 - simple clang-format update I missed
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 206562a8cdcf67..0b8ef5897f61f6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -98,9 +98,9 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
const auto *CE = cast<CallExpr>(Call.getOriginExpr());
const LocationContext *LCtx = C.getLocationContext();
- NonLoc RetVal =
- SVB.conjureSymbolVal(/*SymbolTag=*/nullptr, ChrootCE, LCtx, IntTy, C.blockCount())
- .castAs<NonLoc>();
+ NonLoc RetVal = SVB.conjureSymbolVal(/*SymbolTag=*/nullptr, ChrootCE, LCtx,
+ IntTy, C.blockCount())
+ .castAs<NonLoc>();
auto [StateChrootFailed, StateChrootSuccess] = state->assume(RetVal);
@@ -110,13 +110,15 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
if (StateChrootFailed) {
StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
- C.addTransition(StateChrootFailed->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1}));
+ C.addTransition(
+ StateChrootFailed->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1}));
}
if (StateChrootSuccess) {
StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
- C.addTransition(StateChrootSuccess->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero}));
+ C.addTransition(
+ StateChrootSuccess->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero}));
}
}
>From 075e3c9b6e3b33fe0bb9133978726955a3ac5e44 Mon Sep 17 00:00:00 2001
From: einvbri <vince.a.bridgers at ericsson.com>
Date: Wed, 27 Nov 2024 19:50:10 +0100
Subject: [PATCH 04/20] [analyzer] Modernize, improve and promote chroot
checker
Updates #3
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860298423
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860294735
* Use early return, avoid indentation of code per comment
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860290631
* Inline Str per review comment
https://github.com/llvm/llvm-project/pull/117791#discussion_r1861148485
* Simplify use of conjured symbol per review comments
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860267300
* Remove unneeded branch per review comments
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860235575
* Remove unnecessary custom uniquing per review comments.
---
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 85 +++++++++----------
1 file changed, 39 insertions(+), 46 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 0b8ef5897f61f6..3e4fc1978ea328 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -86,39 +86,38 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
}
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef state = C.getState();
+ ProgramStateRef State = C.getState();
SValBuilder &SVB = C.getSValBuilder();
BasicValueFactory &BVF = SVB.getBasicValueFactory();
const QualType IntTy = C.getASTContext().IntTy;
+ // Using CallDescriptions to match on CallExpr, so no need
+ // to do null checks.
const Expr *ChrootCE = Call.getOriginExpr();
- if (!ChrootCE)
- return;
- const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+ const auto *CE = cast<CallExpr>(ChrootCE);
const LocationContext *LCtx = C.getLocationContext();
- NonLoc RetVal = SVB.conjureSymbolVal(/*SymbolTag=*/nullptr, ChrootCE, LCtx,
- IntTy, C.blockCount())
- .castAs<NonLoc>();
-
- auto [StateChrootFailed, StateChrootSuccess] = state->assume(RetVal);
const llvm::APSInt &Zero = BVF.getValue(0, IntTy);
const llvm::APSInt &Minus1 = BVF.getValue(-1, IntTy);
+ // Setup path where chroot fails, returning -1
+ ProgramStateRef StateChrootFailed =
+ State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1});
if (StateChrootFailed) {
StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
- C.addTransition(
- StateChrootFailed->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1}));
+ C.addTransition(StateChrootFailed);
}
+ // Setup path where chroot succeeds, returning 0
+ ProgramStateRef StateChrootSuccess =
+ State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero});
if (StateChrootSuccess) {
StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
- C.addTransition(
- StateChrootSuccess->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero}));
+ C.addTransition(StateChrootSuccess);
}
}
@@ -137,8 +136,7 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
if (const MemRegion *R = ArgVal.getAsRegion()) {
R = R->StripCasts();
if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
- const StringLiteral* Str = StrRegion->getStringLiteral();
- if (Str->getString() == "/") {
+ if (StrRegion->getStringLiteral()->getString() == "/") {
state = state->set<ChrootState>(JAIL_ENTERED);
}
}
@@ -175,37 +173,32 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
if (matchesAny(Call, Chroot, Chdir))
return;
- // If jail state is ROOT_CHANGED, generate BugReport.
- const ChrootKind k = C.getState()->get<ChrootState>();
- if (k == ROOT_CHANGED) {
- ExplodedNode *Err =
- C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
- if (!Err)
- return;
- const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
-
- const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
- assert(ChrootCallNode && "Could not find place of stream opening.");
-
- PathDiagnosticLocation LocUsedForUniqueing;
- if (const Stmt *ChrootStmt = ChrootCallNode->getStmtForDiagnostics())
- LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
- ChrootStmt, C.getSourceManager(),
- ChrootCallNode->getLocationContext());
-
- std::unique_ptr<PathSensitiveBugReport> R =
- std::make_unique<PathSensitiveBugReport>(
- BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
- Err, LocUsedForUniqueing,
- ChrootCallNode->getLocationContext()->getDecl());
-
- R->addNote("chroot called here",
- PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
- C.getSourceManager()),
- {ChrootExpr->getSourceRange()});
-
- C.emitReport(std::move(R));
- }
+ // If jail state is not ROOT_CHANGED just return.
+ if (C.getState()->get<ChrootState>() != ROOT_CHANGED)
+ return;
+
+ // Generate bug report.
+ ExplodedNode *Err =
+ C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
+ if (!Err)
+ return;
+ const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
+ assert(ChrootExpr && "Expected to find chroot call expansion.");
+
+ const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
+ assert(ChrootCallNode && "Could not find node invoking chroot.");
+
+ std::unique_ptr<PathSensitiveBugReport> R =
+ std::make_unique<PathSensitiveBugReport>(
+ BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
+ Err);
+
+ R->addNote("chroot called here",
+ PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
+ C.getSourceManager()),
+ {ChrootExpr->getSourceRange()});
+
+ C.emitReport(std::move(R));
}
} // namespace
>From d148f752991acd911414b9109805593a2e57bcff Mon Sep 17 00:00:00 2001
From: einvbri <vince.a.bridgers at ericsson.com>
Date: Thu, 28 Nov 2024 10:48:54 +0100
Subject: [PATCH 05/20] [analyzer] Modernize, improve and promote chroot
checker
Updates #4
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860231190
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860224501
https://github.com/llvm/llvm-project/pull/117791#discussion_r1860232925
* Moved egraph walk to bug report visitor per review comments
* Some minor text updates for comments and descriptions
---
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 96 +++++++++++++------
1 file changed, 67 insertions(+), 29 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 3e4fc1978ea328..e5bf86ceae5f83 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -6,7 +6,9 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines chroot checker, which checks improper use of chroot.
+// This file defines chroot checker, which checks improper use of chroot. This
+// is described by the SEI Cert C rule POS05-C. The checker is a warning not
+// a hard failure since it only checks for a recommended rule.
//
//===----------------------------------------------------------------------===//
@@ -66,10 +68,6 @@ class ChrootChecker : public Checker<eval::Call, check::PreCall> {
private:
void evalChroot(const CallEvent &Call, CheckerContext &C) const;
void evalChdir(const CallEvent &Call, CheckerContext &C) const;
-
- /// Searches for the ExplodedNode where chroot was called.
- static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
- CheckerContext &C);
};
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
@@ -145,26 +143,72 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
C.addTransition(state);
}
-const ExplodedNode *ChrootChecker::getAcquisitionSite(const ExplodedNode *N,
- CheckerContext &C) {
- ProgramStateRef State = N->getState();
- // When a chroot() issue is found, the current exploded node may not
- // have state info for where chroot() was called for the bug report but
- // a predecessor should have it.
- if (!State->get<ChrootCall>())
- N = N->getFirstPred();
-
- const ExplodedNode *Pred = N;
- while (N) {
- State = N->getState();
+class ChrootInvocationVisitor final : public BugReporterVisitor {
+ bool Satisfied = false;
+ const SymbolRef ChrootSym;
+
+ const ExplodedNode *getAcquisitionSite(const ExplodedNode *N) {
+ ProgramStateRef State = N->getState();
+ // When a chroot() issue is found, the current exploded node may not
+ // have state info for where chroot() was called for the bug report but
+ // a predecessor should have it.
if (!State->get<ChrootCall>())
- return Pred;
- Pred = N;
- N = N->getFirstPred();
+ N = N->getFirstPred();
+
+ const ExplodedNode *Pred = N;
+ while (N) {
+ State = N->getState();
+ if (!State->get<ChrootCall>())
+ return Pred;
+ Pred = N;
+ N = N->getFirstPred();
+ }
+
+ return nullptr;
}
- return nullptr;
-}
+public:
+ explicit ChrootInvocationVisitor(SymbolRef ChrootSym)
+ : ChrootSym(ChrootSym) {}
+
+ static void *getTag() {
+ static int Tag = 0;
+ return &Tag;
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.AddPointer(getTag());
+ }
+
+ PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) override {
+ if (Satisfied)
+ return nullptr;
+
+ const Expr *ChrootExpr = N->getFirstPred()->getState()->get<ChrootCall>();
+ if (!ChrootExpr)
+ return nullptr;
+
+ const Stmt *S = N->getStmtForDiagnostics();
+ if (!S)
+ return nullptr;
+
+ const ExplodedNode *ChrootNode = getAcquisitionSite(N);
+ if (!ChrootNode)
+ return nullptr;
+
+ Satisfied = true;
+ PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ N->getLocationContext());
+
+ BR.addNote("chroot called here",
+ PathDiagnosticLocation::create(ChrootNode->getLocation(),
+ BRC.getSourceManager()),
+ {ChrootExpr->getSourceRange()});
+ return nullptr;
+ }
+};
// Check the jail state before any function call except chroot and chdir().
void ChrootChecker::checkPreCall(const CallEvent &Call,
@@ -185,18 +229,12 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
assert(ChrootExpr && "Expected to find chroot call expansion.");
- const ExplodedNode *ChrootCallNode = getAcquisitionSite(Err, C);
- assert(ChrootCallNode && "Could not find node invoking chroot.");
-
std::unique_ptr<PathSensitiveBugReport> R =
std::make_unique<PathSensitiveBugReport>(
BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
Err);
- R->addNote("chroot called here",
- PathDiagnosticLocation::create(ChrootCallNode->getLocation(),
- C.getSourceManager()),
- {ChrootExpr->getSourceRange()});
+ R->addVisitor<ChrootInvocationVisitor>(nullptr);
C.emitReport(std::move(R));
}
>From 4e07bbe1677e3b8686fefe353473555ac12ef097 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:26:16 +0100
Subject: [PATCH 06/20] NFC Properly use BugReporterVisitor
---
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 78 +++++--------------
clang/test/Analysis/chroot.c | 16 ++--
2 files changed, 30 insertions(+), 64 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index e5bf86ceae5f83..698220cae52798 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -13,7 +13,6 @@
//===----------------------------------------------------------------------===//
#include "clang/AST/ASTContext.h"
-#include "clang/Basic/TargetInfo.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -34,11 +33,6 @@ enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
// Track chroot state changes for success, failure, state change
// and "jail"
REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootState, ChrootKind)
-
-// Track the call expression to chroot for accurate
-// warning messages
-REGISTER_TRAIT_WITH_PROGRAMSTATE(ChrootCall, const Expr *)
-
namespace {
// This checker checks improper use of chroot.
@@ -105,7 +99,6 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1});
if (StateChrootFailed) {
StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
- StateChrootFailed = StateChrootFailed->set<ChrootCall>(ChrootCE);
C.addTransition(StateChrootFailed);
}
@@ -114,7 +107,6 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero});
if (StateChrootSuccess) {
StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
- StateChrootSuccess = StateChrootSuccess->set<ChrootCall>(ChrootCE);
C.addTransition(StateChrootSuccess);
}
}
@@ -144,41 +136,9 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
}
class ChrootInvocationVisitor final : public BugReporterVisitor {
- bool Satisfied = false;
- const SymbolRef ChrootSym;
-
- const ExplodedNode *getAcquisitionSite(const ExplodedNode *N) {
- ProgramStateRef State = N->getState();
- // When a chroot() issue is found, the current exploded node may not
- // have state info for where chroot() was called for the bug report but
- // a predecessor should have it.
- if (!State->get<ChrootCall>())
- N = N->getFirstPred();
-
- const ExplodedNode *Pred = N;
- while (N) {
- State = N->getState();
- if (!State->get<ChrootCall>())
- return Pred;
- Pred = N;
- N = N->getFirstPred();
- }
-
- return nullptr;
- }
-
public:
- explicit ChrootInvocationVisitor(SymbolRef ChrootSym)
- : ChrootSym(ChrootSym) {}
-
- static void *getTag() {
- static int Tag = 0;
- return &Tag;
- }
-
- void Profile(llvm::FoldingSetNodeID &ID) const override {
- ID.AddPointer(getTag());
- }
+ explicit ChrootInvocationVisitor(const CallDescription &Chroot)
+ : Chroot{Chroot} {}
PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
BugReporterContext &BRC,
@@ -186,28 +146,32 @@ class ChrootInvocationVisitor final : public BugReporterVisitor {
if (Satisfied)
return nullptr;
- const Expr *ChrootExpr = N->getFirstPred()->getState()->get<ChrootCall>();
- if (!ChrootExpr)
+ auto StmtP = N->getLocation().getAs<StmtPoint>();
+ if (!StmtP)
return nullptr;
- const Stmt *S = N->getStmtForDiagnostics();
- if (!S)
+ const CallExpr *Call = StmtP->getStmtAs<CallExpr>();
+ if (!Call)
return nullptr;
- const ExplodedNode *ChrootNode = getAcquisitionSite(N);
- if (!ChrootNode)
+ if (!matchesAnyAsWritten(*Call, Chroot))
return nullptr;
Satisfied = true;
- PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ PathDiagnosticLocation Pos(Call, BRC.getSourceManager(),
N->getLocationContext());
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, "chroot called here",
+ /*addPosRange=*/true);
+ }
- BR.addNote("chroot called here",
- PathDiagnosticLocation::create(ChrootNode->getLocation(),
- BRC.getSourceManager()),
- {ChrootExpr->getSourceRange()});
- return nullptr;
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ static bool Tag;
+ ID.AddPointer(&Tag);
}
+
+private:
+ const CallDescription &Chroot;
+ bool Satisfied = false;
};
// Check the jail state before any function call except chroot and chdir().
@@ -226,16 +190,12 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
C.generateNonFatalErrorNode(C.getState(), C.getPredecessor());
if (!Err)
return;
- const Expr *ChrootExpr = C.getState()->get<ChrootCall>();
- assert(ChrootExpr && "Expected to find chroot call expansion.");
std::unique_ptr<PathSensitiveBugReport> R =
std::make_unique<PathSensitiveBugReport>(
BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
Err);
-
- R->addVisitor<ChrootInvocationVisitor>(nullptr);
-
+ R->addVisitor<ChrootInvocationVisitor>(Chroot);
C.emitReport(std::move(R));
}
diff --git a/clang/test/Analysis/chroot.c b/clang/test/Analysis/chroot.c
index e682fa0a5019e7..7d71f2cf9ca976 100644
--- a/clang/test/Analysis/chroot.c
+++ b/clang/test/Analysis/chroot.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=unix.Chroot -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=unix.Chroot -analyzer-output=text -verify %s
extern int chroot(const char* path);
extern int chdir(const char* path);
@@ -8,7 +8,9 @@ void foo(void) {
void f1(void) {
chroot("/usr/local"); // expected-note {{chroot called here}}
- foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
+ foo();
+ // expected-warning at -1 {{No call of chdir("/") immediately after chroot}}
+ // expected-note at -2 {{No call of chdir("/") immediately after chroot}}
}
void f2(void) {
@@ -20,7 +22,9 @@ void f2(void) {
void f3(void) {
chroot("/usr/local"); // expected-note {{chroot called here}}
chdir("../"); // change working directory, still out of jail.
- foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
+ foo();
+ // expected-warning at -1 {{No call of chdir("/") immediately after chroot}}
+ // expected-note at -2 {{No call of chdir("/") immediately after chroot}}
}
void f4(void) {
@@ -45,10 +49,12 @@ void f6(void) {
void f7(void) {
int v = chroot("/usr/local"); // expected-note {{chroot called here}}
- if (v == -1) {
+ if (v == -1) { // expected-note {{Taking false branch}}
foo(); // no warning, chroot failed
chdir("../"); // change working directory, still out of jail.
} else {
- foo(); // expected-warning {{No call of chdir("/") immediately after chroot}}
+ foo();
+ // expected-warning at -1 {{No call of chdir("/") immediately after chroot}}
+ // expected-note at -2 {{No call of chdir("/") immediately after chroot}}
}
}
>From 2b51731ff872885cb2f701e70c068102cf354e9a Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:30:13 +0100
Subject: [PATCH 07/20] NFC Simplify ChrootChecker::evalChdir
---
.../lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 698220cae52798..200f2f6b8ece47 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -112,27 +112,23 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
}
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef state = C.getState();
+ ProgramStateRef State = C.getState();
// If there are no jail state, just return.
- const ChrootKind k = C.getState()->get<ChrootState>();
- if (!k)
+ if (State->get<ChrootState>() == NO_CHROOT)
return;
// After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.
- const Expr *ArgExpr = Call.getArgExpr(0);
- SVal ArgVal = C.getSVal(ArgExpr);
+ SVal ArgVal = Call.getArgSVal(0);
if (const MemRegion *R = ArgVal.getAsRegion()) {
R = R->StripCasts();
- if (const StringRegion* StrRegion= dyn_cast<StringRegion>(R)) {
+ if (const auto *StrRegion = dyn_cast<StringRegion>(R)) {
if (StrRegion->getStringLiteral()->getString() == "/") {
- state = state->set<ChrootState>(JAIL_ENTERED);
+ C.addTransition(State->set<ChrootState>(JAIL_ENTERED));
}
}
}
-
- C.addTransition(state);
}
class ChrootInvocationVisitor final : public BugReporterVisitor {
>From 235d8a3e8dfde40d6607d43cc70d69b5f989de18 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:35:15 +0100
Subject: [PATCH 08/20] NFC Simplify ChrootChecker::evalChroot
---
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 37 ++++++-------------
1 file changed, 11 insertions(+), 26 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 200f2f6b8ece47..10d2dd31323758 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -78,37 +78,22 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
}
void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
- ProgramStateRef State = C.getState();
- SValBuilder &SVB = C.getSValBuilder();
- BasicValueFactory &BVF = SVB.getBasicValueFactory();
-
- const QualType IntTy = C.getASTContext().IntTy;
+ BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
+ const LocationContext *LCtx = C.getLocationContext();
// Using CallDescriptions to match on CallExpr, so no need
// to do null checks.
- const Expr *ChrootCE = Call.getOriginExpr();
- const auto *CE = cast<CallExpr>(ChrootCE);
-
- const LocationContext *LCtx = C.getLocationContext();
+ const auto *CE = cast<CallExpr>(Call.getOriginExpr());
- const llvm::APSInt &Zero = BVF.getValue(0, IntTy);
- const llvm::APSInt &Minus1 = BVF.getValue(-1, IntTy);
-
- // Setup path where chroot fails, returning -1
- ProgramStateRef StateChrootFailed =
- State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Minus1});
- if (StateChrootFailed) {
- StateChrootFailed = StateChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED);
- C.addTransition(StateChrootFailed);
- }
+ const QualType IntTy = C.getASTContext().IntTy;
+ SVal Zero = nonloc::ConcreteInt{BVF.getValue(0, IntTy)};
+ SVal Minus1 = nonloc::ConcreteInt{BVF.getValue(-1, IntTy)};
- // Setup path where chroot succeeds, returning 0
- ProgramStateRef StateChrootSuccess =
- State->BindExpr(CE, LCtx, nonloc::ConcreteInt{Zero});
- if (StateChrootSuccess) {
- StateChrootSuccess = StateChrootSuccess->set<ChrootState>(ROOT_CHANGED);
- C.addTransition(StateChrootSuccess);
- }
+ ProgramStateRef State = C.getState();
+ ProgramStateRef ChrootFailed = State->BindExpr(CE, LCtx, Minus1);
+ ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
+ C.addTransition(ChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED));
+ C.addTransition(ChrootSucceeded->set<ChrootState>(ROOT_CHANGED));
}
void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
>From ed1eff1bdc390a944ff9df1712535b6982e07427 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:36:39 +0100
Subject: [PATCH 09/20] NFC Rename variable
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 10d2dd31323758..6b5087b230efc1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -48,7 +48,7 @@ namespace {
//
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
// This bug refers to possibly break out of a chroot() jail.
- const BugType BT_BreakJail{this, "Break out of jail"};
+ const BugType BreakJailBug{this, "Break out of jail"};
const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1},
Chdir{CDM::CLibrary, {"chdir"}, 1};
@@ -174,7 +174,7 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
std::unique_ptr<PathSensitiveBugReport> R =
std::make_unique<PathSensitiveBugReport>(
- BT_BreakJail, R"(No call of chdir("/") immediately after chroot)",
+ BreakJailBug, R"(No call of chdir("/") immediately after chroot)",
Err);
R->addVisitor<ChrootInvocationVisitor>(Chroot);
C.emitReport(std::move(R));
>From 5fa01e7babd72d10909f3792f80023a870b6950b Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:37:28 +0100
Subject: [PATCH 10/20] NFC Cleanup private fields
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 6b5087b230efc1..0e826e79b55fe3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -47,21 +47,17 @@ namespace {
// bug<--foo()-- JAIL_ENTERED<--foo()--
//
class ChrootChecker : public Checker<eval::Call, check::PreCall> {
- // This bug refers to possibly break out of a chroot() jail.
- const BugType BreakJailBug{this, "Break out of jail"};
-
- const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1},
- Chdir{CDM::CLibrary, {"chdir"}, 1};
-
public:
- ChrootChecker() {}
-
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
private:
void evalChroot(const CallEvent &Call, CheckerContext &C) const;
void evalChdir(const CallEvent &Call, CheckerContext &C) const;
+
+ const BugType BreakJailBug{this, "Break out of jail"};
+ const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1};
+ const CallDescription Chdir{CDM::CLibrary, {"chdir"}, 1};
};
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
>From 84426d93445ba7c19bb304d636afb782740e93eb Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:40:11 +0100
Subject: [PATCH 11/20] Make evalChroot and evalChdir actually eval the call
when it does so
---
.../StaticAnalyzer/Checkers/ChrootChecker.cpp | 26 +++++++++----------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 0e826e79b55fe3..159cd5c6f2f6ec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -52,8 +52,8 @@ class ChrootChecker : public Checker<eval::Call, check::PreCall> {
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
private:
- void evalChroot(const CallEvent &Call, CheckerContext &C) const;
- void evalChdir(const CallEvent &Call, CheckerContext &C) const;
+ bool evalChroot(const CallEvent &Call, CheckerContext &C) const;
+ bool evalChdir(const CallEvent &Call, CheckerContext &C) const;
const BugType BreakJailBug{this, "Break out of jail"};
const CallDescription Chroot{CDM::CLibrary, {"chroot"}, 1};
@@ -61,19 +61,16 @@ class ChrootChecker : public Checker<eval::Call, check::PreCall> {
};
bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
- if (Chroot.matches(Call)) {
- evalChroot(Call, C);
- return true;
- }
- if (Chdir.matches(Call)) {
- evalChdir(Call, C);
- return true;
- }
+ if (Chroot.matches(Call))
+ return evalChroot(Call, C);
+
+ if (Chdir.matches(Call))
+ return evalChdir(Call, C);
return false;
}
-void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
+bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
const LocationContext *LCtx = C.getLocationContext();
@@ -90,14 +87,15 @@ void ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
C.addTransition(ChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED));
C.addTransition(ChrootSucceeded->set<ChrootState>(ROOT_CHANGED));
+ return true;
}
-void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
+bool ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
ProgramStateRef State = C.getState();
// If there are no jail state, just return.
if (State->get<ChrootState>() == NO_CHROOT)
- return;
+ return false;
// After chdir("/"), enter the jail, set the enum value JAIL_ENTERED.
SVal ArgVal = Call.getArgSVal(0);
@@ -107,9 +105,11 @@ void ChrootChecker::evalChdir(const CallEvent &Call, CheckerContext &C) const {
if (const auto *StrRegion = dyn_cast<StringRegion>(R)) {
if (StrRegion->getStringLiteral()->getString() == "/") {
C.addTransition(State->set<ChrootState>(JAIL_ENTERED));
+ return true;
}
}
}
+ return false;
}
class ChrootInvocationVisitor final : public BugReporterVisitor {
>From 22e68a619d69bb6c32c7b6958d0eb89ff2193f29 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:41:22 +0100
Subject: [PATCH 12/20] NFC Reflow the header comment
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 159cd5c6f2f6ec..7056e45744968b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -6,9 +6,10 @@
//
//===----------------------------------------------------------------------===//
//
-// This file defines chroot checker, which checks improper use of chroot. This
-// is described by the SEI Cert C rule POS05-C. The checker is a warning not
-// a hard failure since it only checks for a recommended rule.
+// This file defines chroot checker, which checks improper use of chroot.
+// This is described by the SEI Cert C rule POS05-C.
+// The checker is a warning not a hard failure since it only checks for a
+// recommended rule.
//
//===----------------------------------------------------------------------===//
>From 9851a1d31f888fcdf866e1979105d7dbcc96e3f3 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:42:34 +0100
Subject: [PATCH 13/20] NFC Hide ChrootKind
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 7056e45744968b..1d012d6c4a5edc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -28,8 +28,9 @@
using namespace clang;
using namespace ento;
-// enum value that represent the jail state
+namespace {
enum ChrootKind { NO_CHROOT, ROOT_CHANGED, ROOT_CHANGE_FAILED, JAIL_ENTERED };
+} // namespace
// Track chroot state changes for success, failure, state change
// and "jail"
>From 09ea72527767998a757199aa5960e51a88351b57 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:44:28 +0100
Subject: [PATCH 14/20] NFC Reorder stuff
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 1d012d6c4a5edc..72e78ac2e38bff 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -75,6 +75,7 @@ bool ChrootChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
const LocationContext *LCtx = C.getLocationContext();
+ ProgramStateRef State = C.getState();
// Using CallDescriptions to match on CallExpr, so no need
// to do null checks.
@@ -84,10 +85,10 @@ bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
SVal Zero = nonloc::ConcreteInt{BVF.getValue(0, IntTy)};
SVal Minus1 = nonloc::ConcreteInt{BVF.getValue(-1, IntTy)};
- ProgramStateRef State = C.getState();
ProgramStateRef ChrootFailed = State->BindExpr(CE, LCtx, Minus1);
- ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
C.addTransition(ChrootFailed->set<ChrootState>(ROOT_CHANGE_FAILED));
+
+ ProgramStateRef ChrootSucceeded = State->BindExpr(CE, LCtx, Zero);
C.addTransition(ChrootSucceeded->set<ChrootState>(ROOT_CHANGED));
return true;
}
>From 36c90677d052c8d58743f1ef0739a94df4f12edb Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:44:45 +0100
Subject: [PATCH 15/20] Simplify registerChrootChecker and
shouldRegisterChrootChecker
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 72e78ac2e38bff..9c5bd2d067de66 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -181,10 +181,8 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
} // namespace
-void ento::registerChrootChecker(CheckerManager &mgr) {
- mgr.registerChecker<ChrootChecker>();
+void ento::registerChrootChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<ChrootChecker>();
}
-bool ento::shouldRegisterChrootChecker(const CheckerManager &mgr) {
- return true;
-}
+bool ento::shouldRegisterChrootChecker(const CheckerManager &) { return true; }
>From 7e37107c422129d7b68f5e0c1d226495ca97f55f Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 28 Nov 2024 19:45:10 +0100
Subject: [PATCH 16/20] NFC Remove redundant type
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 9c5bd2d067de66..c5cd2bd1411864 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -171,10 +171,8 @@ void ChrootChecker::checkPreCall(const CallEvent &Call,
if (!Err)
return;
- std::unique_ptr<PathSensitiveBugReport> R =
- std::make_unique<PathSensitiveBugReport>(
- BreakJailBug, R"(No call of chdir("/") immediately after chroot)",
- Err);
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BreakJailBug, R"(No call of chdir("/") immediately after chroot)", Err);
R->addVisitor<ChrootInvocationVisitor>(Chroot);
C.emitReport(std::move(R));
}
>From 81818c9146ac3e689eccff5f06969c2fa3b560be Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 29 Nov 2024 08:04:11 +0100
Subject: [PATCH 17/20] NFC Prefer the member call over the free function
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index c5cd2bd1411864..5a09e1b160736c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -134,7 +134,7 @@ class ChrootInvocationVisitor final : public BugReporterVisitor {
if (!Call)
return nullptr;
- if (!matchesAnyAsWritten(*Call, Chroot))
+ if (!Chroot.matchesAsWritten(*Call))
return nullptr;
Satisfied = true;
>From e632364161ec198953b9edcceec4bc6a4aa1bf31 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 29 Nov 2024 08:07:07 +0100
Subject: [PATCH 18/20] NFC Add final
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 5a09e1b160736c..507f8205e6e3ce 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -48,7 +48,7 @@ namespace {
// | |
// bug<--foo()-- JAIL_ENTERED<--foo()--
//
-class ChrootChecker : public Checker<eval::Call, check::PreCall> {
+class ChrootChecker final : public Checker<eval::Call, check::PreCall> {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
>From b2edb862acf4ca4c005def6d2f9f8dc7e7c644fb Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 29 Nov 2024 08:20:29 +0100
Subject: [PATCH 19/20] NFC Drop a comment
---
clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp | 3 ---
1 file changed, 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
index 507f8205e6e3ce..99fc0a953ef178 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
@@ -76,9 +76,6 @@ bool ChrootChecker::evalChroot(const CallEvent &Call, CheckerContext &C) const {
BasicValueFactory &BVF = C.getSValBuilder().getBasicValueFactory();
const LocationContext *LCtx = C.getLocationContext();
ProgramStateRef State = C.getState();
-
- // Using CallDescriptions to match on CallExpr, so no need
- // to do null checks.
const auto *CE = cast<CallExpr>(Call.getOriginExpr());
const QualType IntTy = C.getASTContext().IntTy;
>From 4bab2562b085b35af00772d1b9289a0333d761d2 Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Fri, 29 Nov 2024 08:21:30 +0100
Subject: [PATCH 20/20] Clarify some docs
---
clang/docs/analyzer/checkers.rst | 14 ++++++++++----
clang/test/Analysis/chroot.c | 8 ++++++++
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index f3dde66f78db88..e56b2493d564b3 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -1757,19 +1757,25 @@ unix.Chroot (C)
Check improper use of chroot described by SEI Cert C recommendation `POS05-C.
Limit access to files by creating a jail
<https://wiki.sei.cmu.edu/confluence/display/c/POS05-C.+Limit+access+to+files+by+creating+a+jail>`_.
-The checker finds usage patterns where chdir() is not called immediately
-after a call to chroot().
+The checker finds usage patterns where ``chdir("/")`` is not called immediately
+after a call to ``chroot(path)``.
.. code-block:: c
void f();
- void test() {
+ void test_bad() {
chroot("/usr/local");
f(); // warn: no call of chdir("/") immediately after chroot
}
- void test() {
+ void test_bad_path() {
+ chroot("/usr/local");
+ chdir("/usr"); // warn: no call of chdir("/") immediately after chroot
+ f();
+ }
+
+ void test_good() {
chroot("/usr/local");
chdir("/"); // no warning
f();
diff --git a/clang/test/Analysis/chroot.c b/clang/test/Analysis/chroot.c
index 7d71f2cf9ca976..34f460b0f01540 100644
--- a/clang/test/Analysis/chroot.c
+++ b/clang/test/Analysis/chroot.c
@@ -58,3 +58,11 @@ void f7(void) {
// expected-note at -2 {{No call of chdir("/") immediately after chroot}}
}
}
+
+void f8() {
+ chroot("/usr/local"); // expected-note {{chroot called here}}
+ chdir("/usr"); // This chdir was ineffective because it's not exactly `chdir("/")`.
+ foo();
+ // expected-warning at -1 {{No call of chdir("/") immediately after chroot}}
+ // expected-note at -2 {{No call of chdir("/") immediately after chroot}}
+}
More information about the cfe-commits
mailing list