[analyzer] Modernize, improve and promote chroot checker

[vabridgers]

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.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: None (vabridgers)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/117791.diff

5 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/docs/analyzer/checkers.rst (+15-15)
• (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4-4)
• (modified) clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp (+118-32)
• (modified) clang/test/Analysis/chroot.c (+33-3)

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..bf4d2af2eea21d 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,41 @@ 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));
-      }
+  //void *const* k = C.getState()->FindGDM(ChrootChecker::getTag());
+  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/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}}
+  }
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[steakhal]

This checker deserved some love for sure. Thank you for pushing for this.
I left quite a few comments, touching style and also direction of this patch.
Thanks Vince!

[vabridgers]

@steakhal and @NagyDonat , thanks for the comments. I'll address and update the patch. Best

[steakhal]

@steakhal and @NagyDonat , thanks for the comments. I'll address and update the patch. Best

@vabridgers Please avoid force pushes. Prefer merge over rebase while doing the reviews. It's okay to have a sequence of commits fixing up certain behavior. Once the review is done, the PR is anyways squashed when merging. This helps to crossreference existing comments to your PR, that would GitHub loose track of every time you do a force push.

EDIT: I'm highlighting this not because you force pushed to this PR, but rather expressing that shouldn't be necessary, and that the fixup commits and merges are the preferred way in GH PRs. I think I've seen recent PRs from you where force pushes were prominent. So I figured I'd point this out to have a more fluent review experience.

[vabridgers]

@vabridgers Please avoid force pushes.
I understand and will abide by this request. I had tried rebasing and pushing at one time and found I needed to force a push upload my newest changes and thought that was the default flow.

[vabridgers]

@steakhal , I believe all comments are now resolved. Thank you for the thoughtful and detailed comments, look forward to concluding this in the best way possible. Thank you!

[steakhal]

Hi Vince, I figured it's easier if I just push to your branch with my recommendations.
Let me know if you like it. Challenge it if not.

[vabridgers]

Hi @steakhal , no problem. Thanks for the proactive help! I'll take a look. I'll get a round of testing started too to make sure there are no unexpected problems. Best!

[vabridgers]

Thanks @steakhal !