[clang] [clang][analyzer] Add checker 'security.UnsafeSymlinkTest' (PR #221184)
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 23 00:51:49 PDT 2026
https://github.com/balazske updated https://github.com/llvm/llvm-project/pull/221184
>From 9f16f90c9bdf0fe60268f10bc5ddd64a8a530559 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Thu, 3 Sep 2026 17:37:21 +0200
Subject: [PATCH 1/2] [clang][analyzer] Add checker
'security.UnsafeSymlinkTest'
---
clang/docs/analyzer/checkers.rst | 114 ++++
.../clang/StaticAnalyzer/Checkers/Checkers.td | 4 +
.../StaticAnalyzer/Checkers/CMakeLists.txt | 1 +
.../Checkers/UnsafeSymlinkTestChecker.cpp | 541 ++++++++++++++++++
.../test/Analysis/unsafe-symlink-test-notes.c | 115 ++++
clang/test/Analysis/unsafe-symlink-test.c | 412 +++++++++++++
6 files changed, 1187 insertions(+)
create mode 100644 clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
create mode 100644 clang/test/Analysis/unsafe-symlink-test-notes.c
create mode 100644 clang/test/Analysis/unsafe-symlink-test.c
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 1f6b974d5ca7a7..77337274e70eac 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2048,6 +2048,120 @@ 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.UnsafeSymlinkTest (C, C++)
+"""""""""""""""""""""""""""""""""""
+
+Check unsafe detection of symbolic links.
+
+The following code is not a safe way to detect a symbolic link. The file can be
+manipulated asynchronously between the call to ``lstat`` and ``open`` and data
+in ``fs`` may become outdated:
+
+.. code-block:: c
+
+ void handle_file(const char *filename) {
+ struct stat fs;
+ int fd;
+
+ if (lstat(filename, &fs) == -1)
+ return;
+
+ if (!S_ISLNK(fs.st_mode)) {
+ fd = open(filename, O_RDWR); // warning: inaccurate check for symbolic link status of file
+ if (fd == -1)
+ return;
+ }
+ // ...
+ }
+
+The checker produces a warning in similar cases when a file is opened after the
+``stat`` data was obtained for it and presence of symbolic link was checked by
+macro ``S_ISLNK``.
+
+A secure way is to use the ``O_NOFOLLOW`` value in the ``flags`` argument at
+``open``. If this flag is not available on the implementation, the file status
+can be obtained a second time after the ``open`` call. If there is no difference
+between this data and the previously (before open) obtained data, the presence
+of symbolic link can be checked in a safe way.
+
+.. code-block:: c
+
+ void write_nosymlink(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1) {
+ // error: fstat failed
+ // ...
+ return;
+ }
+
+ if (stat1.st_mode != stat2.st_mode || stat1.st_ino != stat2.st_ino || stat1.st_dev != stat2.st_dev) {
+ // error: file was changed
+ // ...
+ return;
+ }
+
+ if (S_ISLNK(stat1.st_mode)) {
+ // file is a symbolic link
+ // ...
+ return;
+ }
+
+ write(fd, buf, size);
+ // ...
+ }
+
+It is important to compare all fields ``st_mode``, ``st_ino`` and ``st_dev`` of
+the ``stat`` structure. This checker emits additionally a warning if a file
+write or read attempt is made in a similar case when these comparisons are
+incomplete (or missing).
+
+.. code-block:: c
+
+ void write_nosymlink(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1) {
+ // ...
+ return;
+ }
+
+ if (stat1.st_mode != stat2.st_mode) { // missing comparison of st_ino and st_dev
+ // ...
+ return;
+ }
+
+ if (S_ISLNK(stat1.st_mode)) {
+ // ...
+ return;
+ }
+
+ write(fd, buf, size); // warning: possibly missing check for external change of file before it was opened
+ // ...
+ }
+
+This kind of warning is produced when a ``lstat`` - ``open`` - ``fstat`` call
+sequence is found for the same file before write or read attempt (and the
+comparisons of status data are missing).
+
.. _security-VAList:
security.VAList (C, C++)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index b6b3857dc7b35f..588850a79111ec 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1007,6 +1007,10 @@ let ParentPackage = Security in {
"'setuid(getuid())' (CERT: POS36-C)">,
Documentation<HasDocumentation>;
+ def UnsafeSymlinkTestChecker : Checker<"UnsafeSymlinkTest">,
+ HelpText<"Check unsafe detection of symbolic links">,
+ Documentation<HasDocumentation>;
+
def VAListChecker : Checker<"VAList">,
HelpText<"Warn on misuse of va_list objects">,
Documentation<HasDocumentation>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index befd60dbec5429..fbb7c3b838085b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -128,6 +128,7 @@ add_clang_library(clangStaticAnalyzerCheckers
UninitializedObject/UninitializedPointee.cpp
UnixAPIChecker.cpp
UnreachableCodeChecker.cpp
+ UnsafeSymlinkTestChecker.cpp
UseAfterLifetimeEnd.cpp
VforkChecker.cpp
VLASizeChecker.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
new file mode 100644
index 00000000000000..a5a0c050ebc12e
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
@@ -0,0 +1,541 @@
+//===-- UnsafeSymlinkTestChecker.cpp ------------------------------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Defines a checker that checks for unsafe symlink detection. This checks for
+// 2 related conditions:
+// - File status is read and used to detect symlink before the file is opened.
+// The file can be changed asynchronously between reading the status data and
+// opening the file, so this check is not safe to use.
+// - To fix the previous issue, the file status can be read after the open too
+// and compared to the previous value. If it did not change, the symlink
+// status is safely determined (the file can not be changed externally after
+// it was opened). The checker can detect a missing comparison of the "before"
+// and "after" status values.
+// (In all cases use of the O_NOFOLLOW flag at 'open' prevents the warning.)
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include <optional>
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+/// Used to identify a file name.
+/// If created with a symbolic region, use the region as key.
+/// If created with a string region, use the contained string as key (different
+/// string regions with same content should be equal).
+struct FileNameKey {
+ std::string FileNameStr;
+ const MemRegion *Region = nullptr;
+
+ FileNameKey(const MemRegion *R) {
+ R = R->StripCasts();
+ if (const auto *SR = dyn_cast<StringRegion>(R))
+ FileNameStr = SR->getStringLiteral()->getString();
+ else
+ Region = R;
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.AddString(FileNameStr);
+ ID.AddPointer(Region);
+ }
+
+ bool operator==(const FileNameKey &RHS) const {
+ return FileNameStr == RHS.FileNameStr && Region == RHS.Region;
+ }
+
+ bool operator<(const FileNameKey &RHS) const {
+ if (!Region && !RHS.Region)
+ return FileNameStr < RHS.FileNameStr;
+ return Region < RHS.Region;
+ }
+
+ std::string getFileName(llvm::StringRef PrefixStr) const {
+ if (!Region)
+ return (llvm::Twine(PrefixStr) + "'" + FileNameStr + "'").str();
+ return "";
+ }
+};
+
+/// Data maintained about a region belonging to a "struct stat".
+struct StatData {
+ /// Region of a 'struct stat' object.
+ const SubRegion *Region;
+ /// Value of the field 'st_mode'.
+ SVal StModeVal;
+ /// Value of the field 'st_ino'.
+ SVal StInoVal;
+ /// Value of the field 'st_dev'.
+ SVal StDevVal;
+
+ bool operator==(const StatData &D) const {
+ return Region == D.Region && StModeVal == D.StModeVal &&
+ StInoVal == D.StInoVal && StDevVal == D.StDevVal;
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.AddPointer(Region);
+ StModeVal.Profile(ID);
+ StInoVal.Profile(ID);
+ StDevVal.Profile(ID);
+ }
+};
+
+/// Data about a file after `lstat` (but not `open`) was called.
+struct FileDataLStat {
+ /// Information about the `stat` structure that was passed to `lstat`.
+ StatData LStatD;
+ /// Indicates if a test for symbolic link on the `st_mode` field of the `stat`
+ /// structure was performed, using the `S_ISLNK` macro.
+ bool LinkCheckPerformed;
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ LStatD.Profile(ID);
+ ID.AddBoolean(LinkCheckPerformed);
+ }
+
+ bool operator==(const FileDataLStat &R) const {
+ return LStatD == R.LStatD && LinkCheckPerformed == R.LinkCheckPerformed;
+ }
+};
+
+/// Data about a file after `lstat` and `open` was called (no symbolic link test
+/// with `S_ISLNK` was performed in between).
+struct FileDataOpened {
+ /// Information about the `stat` structure that was passed to `lstat`.
+ StatData LStatD;
+ /// Information about the `stat` structure that was passed to `fstat`.
+ StatData FStatD;
+ /// Data about the file name (this is used for checker messages).
+ FileNameKey FName;
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ LStatD.Profile(ID);
+ FStatD.Profile(ID);
+ }
+
+ bool operator==(const FileDataOpened &R) const {
+ return LStatD == R.LStatD && FStatD == R.FStatD;
+ }
+};
+
+struct StatFieldsDecl {
+ const FieldDecl *StModeFD;
+ const FieldDecl *StInoFD;
+ const FieldDecl *StDevFD;
+
+ bool isValid() const { return StModeFD && StInoFD && StDevFD; }
+};
+
+struct ASTData {
+ const FieldDecl *StModeFD;
+ const FieldDecl *StInoFD;
+ const FieldDecl *StDevFD;
+ QualType StructStatType;
+ int64_t O_NOFOLLOWValue;
+ bool IsValid;
+ void checkValid() {
+ IsValid = StModeFD && StInoFD && StDevFD && !StructStatType.isNull();
+ }
+};
+
+class UnsafeSymlinkTestChecker
+ : public Checker<check::PostCall, check::BranchCondition,
+ check::RegionChanges, check::DeadSymbols> {
+ const CallDescription LStatFn{CDM::CLibrary, {"lstat"}, 2};
+ const CallDescription OpenFn{CDM::CLibrary, {"open"}, 2};
+ const CallDescription FStatFn{CDM::CLibrary, {"fstat"}, 2};
+ const CallDescriptionSet FileAccessFn{
+ {CDM::CLibrary, {"write"}, 3}, {CDM::CLibrary, {"writev"}, 3},
+ {CDM::CLibrary, {"pwrite"}, 4}, {CDM::CLibrary, {"read"}, 3},
+ {CDM::CLibrary, {"readv"}, 3}, {CDM::CLibrary, {"pread"}, 4},
+ {CDM::CLibrary, {"lseek"}, 3}};
+
+ const BugType BT{this, "Security error", "Incorrect check for symbolic link",
+ false};
+
+ mutable std::optional<ASTData> ASTValues;
+
+public:
+ void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkBranchCondition(const Stmt *S, CheckerContext &C) const;
+ ProgramStateRef checkRegionChanges(ProgramStateRef State,
+ const InvalidatedSymbols *Invalidated,
+ ArrayRef<const MemRegion *> Explicits,
+ ArrayRef<const MemRegion *> Regions,
+ const StackFrame *SF,
+ const CallEvent *Call) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+
+private:
+ const SubRegion *castRegionToStructStat(const MemRegion *R,
+ CheckerContext &C) const {
+ if (!R)
+ return nullptr;
+ std::optional<const MemRegion *> CastR = C.getStoreManager().castRegion(
+ R, C.getASTContext().getPointerType(ASTValues->StructStatType));
+ if (!CastR)
+ return R->getAs<SubRegion>();
+ const SubRegion *SR = (*CastR)->getAs<SubRegion>();
+ return SR ? SR : R->getAs<SubRegion>();
+ }
+ StatData getStatData(const SubRegion *StatR, ProgramStateRef State,
+ CheckerContext &C) const {
+ MemRegionManager &RM = C.getStoreManager().getRegionManager();
+ auto *StatR1 = castRegionToStructStat(StatR, C);
+ auto GetFieldSVal = [&](const FieldDecl *FD) {
+ return State->getSVal(RM.getFieldRegion(FD, StatR1));
+ };
+ return {StatR, GetFieldSVal(ASTValues->StModeFD),
+ GetFieldSVal(ASTValues->StInoFD), GetFieldSVal(ASTValues->StDevFD)};
+ }
+ const NoteTag *getNoteTag(const MemRegion *R, std::string Message,
+ CheckerContext &C) const;
+ void initData(const RecordDecl *StatDecl, const Preprocessor &PP) const;
+};
+
+} // end anonymous namespace
+
+/// Data about files where `lstat` was called but not `open`.
+REGISTER_MAP_WITH_PROGRAMSTATE(LStatCalledMap, FileNameKey, FileDataLStat)
+
+/// Data about files where `lstat` and `open` was called.
+REGISTER_MAP_WITH_PROGRAMSTATE(LStatOpenCalledMap, SymbolRef, FileDataOpened)
+
+const NoteTag *UnsafeSymlinkTestChecker::getNoteTag(const MemRegion *R,
+ std::string Message,
+ CheckerContext &C) const {
+ return C.getNoteTag(
+ [this, R, Message](PathSensitiveBugReport &BR) -> std::string {
+ if (BR.isInteresting(R) && &BR.getBugType() == &BT)
+ return Message;
+ return "";
+ });
+}
+
+static const FieldDecl *findField(llvm::StringRef FieldName,
+ const RecordDecl *RD) {
+ auto FoundField =
+ llvm::find_if(RD->fields(), [&FieldName](const FieldDecl *F) {
+ return F->getNameAsString() == FieldName;
+ });
+ if (FoundField == RD->fields().end())
+ return nullptr;
+ return *FoundField;
+}
+
+void UnsafeSymlinkTestChecker::initData(const RecordDecl *StatDecl,
+ const Preprocessor &PP) const {
+ if (StatDecl) {
+ ASTValues = {findField("st_mode", StatDecl),
+ findField("st_ino", StatDecl),
+ findField("st_dev", StatDecl),
+ StatDecl->getASTContext().getCanonicalTagType(StatDecl),
+ 0,
+ false};
+ if (std::optional<int> Val = tryExpandAsInteger("O_NOFOLLOW", PP))
+ ASTValues->O_NOFOLLOWValue = *Val;
+ } else {
+ ASTValues = {nullptr};
+ }
+ ASTValues->checkValid();
+}
+
+void UnsafeSymlinkTestChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ if (ASTValues && !ASTValues->IsValid)
+ return;
+
+ ProgramStateRef State = C.getState();
+
+ if (LStatFn.matches(Call)) {
+ if (!ASTValues) {
+ initData(
+ Call.parameters()[1]->getType()->getPointeeType()->getAsRecordDecl(),
+ C.getPreprocessor());
+ if (!ASTValues->IsValid)
+ return;
+ }
+
+ const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
+ const auto *StatReg =
+ dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
+ if (!FNameReg || !StatReg)
+ return;
+
+ FileNameKey FName(FNameReg);
+ State = State->set<LStatCalledMap>(FName,
+ {getStatData(StatReg, State, C), false});
+ C.addTransition(State, getNoteTag(StatReg,
+ (llvm::Twine("File status") +
+ FName.getFileName(" of file ") +
+ " is read here before opening the file")
+ .str(),
+ C));
+ return;
+ }
+
+ if (OpenFn.matches(Call)) {
+ const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
+ FileNameKey FName(FNameReg);
+ const FileDataLStat *LStatData = State->get<LStatCalledMap>(FName);
+ SymbolRef FileDescSym = Call.getReturnValue().getAsSymbol();
+ if (!FNameReg || !LStatData || !FileDescSym)
+ return;
+
+ State = State->remove<LStatCalledMap>(FNameReg);
+
+ if (ASTValues->O_NOFOLLOWValue != 0) {
+ const llvm::APSInt *FlagsValue =
+ C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
+ if (!FlagsValue) {
+ C.addTransition(State);
+ return;
+ }
+ if (std::optional<int64_t> FVal = FlagsValue->tryExtValue();
+ FVal && (*FVal & ASTValues->O_NOFOLLOWValue)) {
+ C.addTransition(State);
+ return;
+ }
+ }
+
+ if (!LStatData->LinkCheckPerformed) {
+ State = State->set<LStatOpenCalledMap>(
+ FileDescSym,
+ {LStatData->LStatD, {nullptr, SVal{}, SVal{}, SVal{}}, FName});
+ } else {
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT,
+ (llvm::Twine("Inaccurate check for symbolic link status of file") +
+ FName.getFileName(" "))
+ .str(),
+ N);
+ R->addNote("The file can be manipulated externally between calling "
+ "'lstat' and opening the file",
+ {Call.getSourceRange().getBegin(), C.getSourceManager()});
+ R->addRange(Call.getSourceRange());
+ R->markInteresting(LStatData->LStatD.Region);
+ C.emitReport(std::move(R));
+ return;
+ }
+ }
+ }
+
+ if (FStatFn.matches(Call)) {
+ SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
+ const auto *FStatReg =
+ dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
+ if (!FileDescSym || !FStatReg)
+ return;
+ const FileDataOpened *FileData =
+ State->get<LStatOpenCalledMap>(FileDescSym);
+ if (!FileData)
+ return;
+ State = State->set<LStatOpenCalledMap>(
+ FileDescSym,
+ {FileData->LStatD, getStatData(FStatReg, State, C), FileData->FName});
+ C.addTransition(State,
+ getNoteTag(FStatReg,
+ (llvm::Twine("File status") +
+ FileData->FName.getFileName(" of file ") +
+ " is read here after opening the file")
+ .str(),
+ C));
+ return;
+ }
+
+ if (FileAccessFn.contains(Call)) {
+ SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
+ if (!FileDescSym)
+ return;
+ const FileDataOpened *FileData =
+ State->get<LStatOpenCalledMap>(FileDescSym);
+ if (!FileData)
+ return;
+ State = State->remove<LStatOpenCalledMap>(FileDescSym);
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT,
+ (llvm::Twine("Possibly missing check for external change of file") +
+ FileData->FName.getFileName(" "))
+ .str(),
+ N);
+ R->addNote(
+ "File status was obtained before and after opening the file which "
+ "indicates possible intent of a safe check for symbolic link",
+ {Call.getSourceRange().getBegin(), C.getSourceManager()});
+ R->addNote("For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' "
+ "before and after open should be checked for equality",
+ {Call.getSourceRange().getBegin(), C.getSourceManager()});
+ R->addRange(Call.getSourceRange());
+ R->markInteresting(FileData->LStatD.Region);
+ R->markInteresting(FileData->FStatD.Region);
+ C.emitReport(std::move(R));
+ return;
+ }
+ }
+
+ C.addTransition(State);
+}
+
+namespace {
+class FindMacroVisitor : public ConstStmtVisitor<FindMacroVisitor, bool> {
+ const CheckerContext &C;
+ ProgramStateRef State;
+ const MemRegion *LStatInfoStModeReg;
+
+ bool VisitChildren(const Stmt *S) {
+ for (const Stmt *Child : S->children())
+ if (Child && Visit(Child))
+ return true;
+ return false;
+ }
+
+public:
+ FindMacroVisitor(const CheckerContext &C, ProgramStateRef State,
+ const MemRegion *LStatInfoStModeReg)
+ : C(C), State(State), LStatInfoStModeReg(LStatInfoStModeReg) {}
+ bool VisitStmt(const Stmt *S) { return VisitChildren(S); }
+ bool VisitExpr(const Expr *E) {
+ if (check(E))
+ return true;
+ return VisitChildren(E);
+ }
+
+private:
+ bool check(const Expr *E) {
+ const MemRegion *R = State->getSVal(E, C.getStackFrame()).getAsRegion();
+ if (R != LStatInfoStModeReg)
+ return false;
+ SourceLocation BL = E->getBeginLoc();
+ if (!BL.isMacroID())
+ return false;
+ const SourceManager &SM = C.getASTContext().getSourceManager();
+ SourceLocation StartL;
+ if (!SM.isMacroArgExpansion(BL, &StartL))
+ return false;
+ StringRef MacroName = Lexer::getImmediateMacroName(BL, SM, C.getLangOpts());
+ return MacroName == "S_ISLNK";
+ }
+};
+} // end anonymous namespace
+
+void UnsafeSymlinkTestChecker::checkBranchCondition(const Stmt *S,
+ CheckerContext &C) const {
+ if (!ASTValues || !ASTValues->IsValid)
+ return;
+
+ ExplodedNode *NewNode = C.getPredecessor();
+ LStatCalledMapTy LStatCalled = NewNode->getState()->get<LStatCalledMap>();
+ for (auto I : LStatCalled) {
+ const FieldRegion *FR =
+ C.getStoreManager().getRegionManager().getFieldRegion(
+ ASTValues->StModeFD,
+ castRegionToStructStat(I.second.LStatD.Region, C));
+ ProgramStateRef State = NewNode->getState();
+ FindMacroVisitor FindS_ISLNK(C, State, FR);
+ if (FindS_ISLNK.Visit(S)) {
+ State = State->set<LStatCalledMap>(I.first, {I.second.LStatD, true});
+ NewNode =
+ C.addTransition(State, NewNode,
+ getNoteTag(I.second.LStatD.Region,
+ (llvm::Twine("Possible test if file") +
+ I.first.getFileName(" ") +
+ " is a symbolic link detected here")
+ .str(),
+ C));
+ }
+ }
+
+ ProgramStateRef State = NewNode->getState();
+ LStatOpenCalledMapTy LStatOpenCalled = State->get<LStatOpenCalledMap>();
+ auto CheckEqual = [State, &C](SVal V1, SVal V2) {
+ auto DefVal1 = V1.getAs<DefinedOrUnknownSVal>();
+ auto DefVal2 = V2.getAs<DefinedOrUnknownSVal>();
+ if (!DefVal1 || !DefVal2)
+ return false;
+ DefinedOrUnknownSVal EQV =
+ C.getSValBuilder().evalEQ(State, *DefVal1, *DefVal2);
+ auto [EQTrue, EQFalse] = State->assume(EQV);
+ return EQTrue && !EQFalse;
+ };
+ for (auto I : LStatOpenCalled) {
+ if (I.second.FStatD.Region)
+ if (CheckEqual(I.second.FStatD.StModeVal, I.second.LStatD.StModeVal) &&
+ CheckEqual(I.second.FStatD.StInoVal, I.second.LStatD.StInoVal) &&
+ CheckEqual(I.second.FStatD.StDevVal, I.second.LStatD.StDevVal))
+ State = State->remove<LStatOpenCalledMap>(I.first);
+ }
+
+ C.addTransition(State, NewNode);
+}
+
+ProgramStateRef UnsafeSymlinkTestChecker::checkRegionChanges(
+ ProgramStateRef State, const InvalidatedSymbols *Invalidated,
+ ArrayRef<const MemRegion *> Explicits, ArrayRef<const MemRegion *> Regions,
+ const StackFrame *SF, const CallEvent *Call) const {
+ if (Call && (LStatFn.matches(*Call) || OpenFn.matches(*Call) ||
+ FStatFn.matches(*Call)))
+ return State;
+
+ if (Invalidated) {
+ for (SymbolRef I : *Invalidated)
+ State = State->remove<LStatOpenCalledMap>(I);
+ }
+ llvm::SmallPtrSet<const MemRegion *, 4> InvalidatedR;
+ for (const MemRegion *R : Regions)
+ InvalidatedR.insert(R);
+ for (auto I : State->get<LStatCalledMap>())
+ if (!I.second.LinkCheckPerformed &&
+ InvalidatedR.contains(I.second.LStatD.Region))
+ State = State->remove<LStatCalledMap>(I.first);
+ for (auto I : State->get<LStatOpenCalledMap>())
+ if (InvalidatedR.contains(I.second.LStatD.Region) ||
+ InvalidatedR.contains(I.second.FStatD.Region))
+ State = State->remove<LStatOpenCalledMap>(I.first);
+ return State;
+}
+
+void UnsafeSymlinkTestChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+ if (!ASTValues || !ASTValues->IsValid)
+ return;
+
+ ProgramStateRef State = C.getState();
+ for (auto I : State->get<LStatCalledMap>()) {
+ if (const auto *SymReg = dyn_cast_or_null<SymbolicRegion>(I.first.Region);
+ SymReg && SymReg->getSymbol() && SymReaper.isDead(SymReg->getSymbol()))
+ State = State->remove<LStatCalledMap>(I.first.Region);
+ }
+ for (auto I : State->get<LStatOpenCalledMap>()) {
+ if (SymReaper.isDead(I.first))
+ State = State->remove<LStatOpenCalledMap>(I.first);
+ }
+
+ C.addTransition(State);
+}
+
+void ento::registerUnsafeSymlinkTestChecker(CheckerManager &mgr) {
+ mgr.registerChecker<UnsafeSymlinkTestChecker>();
+}
+
+bool ento::shouldRegisterUnsafeSymlinkTestChecker(const CheckerManager &mgr) {
+ return true;
+}
diff --git a/clang/test/Analysis/unsafe-symlink-test-notes.c b/clang/test/Analysis/unsafe-symlink-test-notes.c
new file mode 100644
index 00000000000000..3454b72350073b
--- /dev/null
+++ b/clang/test/Analysis/unsafe-symlink-test-notes.c
@@ -0,0 +1,115 @@
+// RUN: %clang_analyze_cc1 %s -triple=x86_64-unknown-linux \
+// RUN: -analyzer-output=text -verify \
+// RUN: -analyzer-checker=core,security.UnsafeSymlinkTest
+
+struct stat {
+ int st_mode;
+ int st_ino;
+ int st_dev;
+};
+
+typedef int size_t;
+typedef size_t ssize_t;
+int lstat(const char *restrict path, struct stat *restrict buf);
+int open(const char *path, int oflag);
+ssize_t write(int fildes, const void *buf, size_t nbyte);
+int fstat(int fildes, struct stat *buf);
+
+#define S_ISLNK(M) ((M & 2) != 0)
+
+void test_fstat_single(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1) // expected-note{{File status is read here before opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1) // expected-note{{File status is read here after opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
+ // expected-note{{Possibly missing check for external change of file}} \\
+ // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
+ // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+}
+
+void test_fstat_2(const char *fn2, const char *buf, size_t size) {
+ const char *const fn1 = "x/y.z";
+ struct stat lstat1;
+ struct stat lstat2;
+ int fd1, fd2;
+
+ if (lstat(fn1, &lstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here before opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+ if (lstat(fn2, &lstat2) == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ fd1 = open(fn1, 1);
+ if (fd1 == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+ fd2 = open(fn2, 1);
+ if (fd2 == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ struct stat fstat1;
+ struct stat fstat2;
+ if (fstat(fd1, &fstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here after opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+ if (fstat(fd2, &fstat2) == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ if (fstat2.st_mode == lstat2.st_mode && fstat2.st_ino == lstat2.st_ino && fstat2.st_dev == lstat2.st_dev) { // \\
+ // expected-note{{Assuming 'fstat2.st_mode' is equal to 'lstat2.st_mode'}} \\
+ // expected-note{{Left side of '&&' is true}} \\
+ // expected-note{{Assuming 'fstat2.st_ino' is equal to 'lstat2.st_ino'}} \\
+ // expected-note{{Left side of '&&' is true}} \\
+ // expected-note{{Assuming 'fstat2.st_dev' is equal to 'lstat2.st_dev'}} \\
+ // expected-note{{'fstat2.st_dev' is equal to 'lstat2.st_dev'}} \\
+ // expected-note{{Taking true branch}}
+ write(fd2, buf, size);
+ write(fd1, buf, size); // expected-warning{{Possibly missing check for external change of file 'x/y.z'}} \\
+ // expected-note{{Possibly missing check for external change of file 'x/y.z'}} \\
+ // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
+ // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ }
+}
+
+const char *const g_filename = "a/b/c";
+
+void test_islnk() {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat(g_filename, &lstat_info) == -1) // expected-note{{File status of file 'a/b/c' is read here before opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ if (!S_ISLNK(lstat_info.st_mode)) { // expected-note{{Possible test if file 'a/b/c' is a symbolic link detected here}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking true branch}}
+ fd = open(g_filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file 'a/b/c'}} \\
+ // expected-note{{Inaccurate check for symbolic link status of file 'a/b/c'}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd == -1)
+ return;
+ }
+}
diff --git a/clang/test/Analysis/unsafe-symlink-test.c b/clang/test/Analysis/unsafe-symlink-test.c
new file mode 100644
index 00000000000000..7719d02a0e9f8c
--- /dev/null
+++ b/clang/test/Analysis/unsafe-symlink-test.c
@@ -0,0 +1,412 @@
+// RUN: %clang_analyze_cc1 %s -triple=x86_64-unknown-linux \
+// RUN: -verify \
+// RUN: -analyzer-checker=core,security.UnsafeSymlinkTest
+
+struct stat {
+ int st_mode;
+ int st_ino;
+ int st_dev;
+};
+
+typedef int size_t;
+typedef size_t ssize_t;
+int lstat(const char *restrict path, struct stat *restrict buf);
+int open(const char *path, int oflag);
+ssize_t write(int fildes, const void *buf, size_t nbyte);
+ssize_t read(int fildes, void *buf, size_t nbyte);
+int fstat(int fildes, struct stat *buf);
+
+#define S_ISLNK(M) ((M & 2) != 0)
+#define O_NOFOLLOW (4)
+#define O_OTHER (2)
+
+void test_islnk_local(const char *filename) {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat(filename, &lstat_info) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open(filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_islnk_param(const char *filename, struct stat *lstat_info) {
+ int fd;
+
+ if (lstat(filename, lstat_info) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info->st_mode)) {
+ fd = open(filename, O_OTHER); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_no_islnk(const char *filename) {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat(filename, &lstat_info) == -1)
+ return;
+
+ if (lstat_info.st_mode > 1) {
+ fd = open(filename, O_OTHER); // no-warning
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_islnk_nofollow(const char *filename) {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat(filename, &lstat_info) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open(filename, O_NOFOLLOW | O_OTHER); // no-warning
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_lstat_other(const char *filename, struct stat *lstat_info1, struct stat *lstat_info2) {
+ int fd;
+
+ if (lstat(filename, lstat_info1) == -1)
+ return;
+
+ if (lstat("file", lstat_info2) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info2->st_mode)) {
+ fd = open(filename, 1); // no-warning
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_lstat_multi(const char *filename1, const char *filename2) {
+ struct stat lstat_info1;
+ struct stat lstat_info2;
+
+ if (lstat(filename1, &lstat_info1) == -1)
+ return;
+ if (lstat(filename2, &lstat_info2) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info1.st_mode) && !S_ISLNK(lstat_info2.st_mode)) {
+ int fd1 = open(filename1, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd1 == -1)
+ return;
+ int fd2 = open(filename2, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd2 == -1)
+ return;
+ }
+}
+
+void test_lstat_str_const() {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat("x/y", &lstat_info) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open("x/y", 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_fstat_nocheck(const char *filename, char *buf, size_t size) {
+ struct stat lstat_info;
+ int fd;
+
+ if (lstat(filename, &lstat_info) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat1;
+ if (fstat(fd, &stat1) == -1)
+ return;
+
+ read(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
+ // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
+ // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+}
+
+void test_fstat_badcheck(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ if (stat1.st_mode == stat2.st_mode)
+ write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
+ // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
+ // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+}
+
+void test_fstat_badcheck_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+ int fd;
+
+ if (lstat(filename, stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ if (fstat(fd, stat2) == -1)
+ return;
+
+ if (stat1->st_mode == stat2->st_mode)
+ write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
+ // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
+ // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+}
+
+void test_fstat_goodcheck(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ if (stat1.st_mode == stat2.st_mode && stat1.st_ino == stat2.st_ino && stat1.st_dev == stat2.st_dev)
+ write(fd, buf, size); // no-warning
+}
+
+void test_fstat_goodcheck_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+ int fd;
+
+ if (lstat(filename, stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ if (fstat(fd, stat2) == -1)
+ return;
+
+ if (stat1->st_mode == stat2->st_mode && stat1->st_ino == stat2->st_ino && stat1->st_dev == stat2->st_dev)
+ write(fd, buf, size); // no-warning
+}
+
+void test_fstat_nofollow_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+ int fd;
+
+ if (lstat(filename, stat1) == -1)
+ return;
+
+ fd = open(filename, O_NOFOLLOW);
+ if (fd == -1)
+ return;
+
+ if (fstat(fd, stat2) == -1)
+ return;
+
+ write(fd, buf, size); // no-warning
+}
+
+void test_fstat_nofollow_unknown(const char *filename, const char *buf, size_t size, int flags) {
+ int fd;
+ struct stat stat1;
+ struct stat stat2;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, flags);
+ if (fd == -1)
+ return;
+
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ write(fd, buf, size); // no-warning
+}
+
+extern void f_stat(struct stat *);
+extern void f_fd(int *);
+
+void test_fstat_inval1(const char *filename, const char *buf, size_t size) {
+ struct stat stat_e1;
+ int fd;
+
+ if (lstat(filename, &stat_e1) == -1)
+ return;
+
+ f_stat(&stat_e1);
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ write(fd, buf, size);
+}
+
+void test_fstat_inval2(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ f_stat(&stat1);
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ write(fd, buf, size);
+}
+
+void test_fstat_inval3(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ f_stat(&stat1);
+
+ write(fd, buf, size);
+}
+
+void test_fstat_inval4(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ f_stat(&stat2);
+
+ write(fd, buf, size);
+}
+
+void test_fstat_inval5(const char *filename, const char *buf, size_t size) {
+ struct stat stat1;
+ int fd;
+
+ if (lstat(filename, &stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ struct stat stat2;
+ if (fstat(fd, &stat2) == -1)
+ return;
+
+ f_fd(&fd);
+
+ write(fd, buf, size);
+}
+
+void test_fstat_inval_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+ int fd;
+
+ if (lstat(filename, stat1) == -1)
+ return;
+
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+
+ f_stat(stat1);
+
+ if (fstat(fd, stat2) == -1)
+ return;
+
+ if (stat1->st_mode == stat2->st_mode && stat1->st_ino == stat2->st_ino && stat1->st_dev == stat2->st_dev)
+ write(fd, buf, size); // no-warning
+}
+
+void test_islnk_inval1_p(const char *filename, struct stat *lstat_info) {
+ int fd;
+
+ if (lstat(filename, lstat_info) == -1)
+ return;
+
+ f_stat(lstat_info);
+
+ if (!S_ISLNK(lstat_info->st_mode)) {
+ fd = open(filename, 1); // no-warning
+ if (fd == -1)
+ return;
+ }
+}
+
+void test_islnk_inval2_p(const char *filename, struct stat *lstat_info) {
+ int fd;
+
+ if (lstat(filename, lstat_info) == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info->st_mode)) {
+ f_stat(lstat_info);
+ fd = open(filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
+ // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ if (fd == -1)
+ return;
+ }
+}
>From 62a7a13d6e10aadfbcb397b628d802348d05993e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Wed, 23 Sep 2026 09:51:10 +0200
Subject: [PATCH 2/2] rewrite of documentation, messages, parts of code
---
clang/docs/analyzer/checkers.rst | 94 ++-
.../Checkers/UnsafeSymlinkTestChecker.cpp | 557 ++++++++++--------
.../test/Analysis/unsafe-symlink-test-notes.c | 82 ++-
clang/test/Analysis/unsafe-symlink-test.c | 373 ++++++------
4 files changed, 584 insertions(+), 522 deletions(-)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 77337274e70eac..e09000a9dff87a 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2051,80 +2051,53 @@ This check corresponds to SEI CERT Rule `POS36-C <https://wiki.sei.cmu.edu/confl
security.UnsafeSymlinkTest (C, C++)
"""""""""""""""""""""""""""""""""""
-Check unsafe detection of symbolic links.
+Check for race condition at detection of symbolic links.
-The following code is not a safe way to detect a symbolic link. The file can be
-manipulated asynchronously between the call to ``lstat`` and ``open`` and data
-in ``fs`` may become outdated:
+If the intent is to open the file based on if it is a symbolic link or not,
+a TOCTOU (time-of-check, time-of-use) condition may happen if the check is performed in an incorrect way.
+This situation occurs if the file state is obtained with ``lstat`` and this data is used to find out if the file is a symlink before the file is opened.
+It is possible for external processes to modify the file between the ``lstat`` and ``open`` calls.
-.. code-block:: c
+To avoid this problem the ``O_NOFOLLOW`` flag can be passed to the second argument of the ``open`` call (and using ``lstat`` is not needed).
- void handle_file(const char *filename) {
- struct stat fs;
- int fd;
+If the ``O_NOFOLLOW`` flag is not available on a specific implementation,
+an alternative solution is to get the file state a second time after it was opened and compare this to the previous state data (from before the open).
- if (lstat(filename, &fs) == -1)
- return;
+This check can detect the following case:
- if (!S_ISLNK(fs.st_mode)) {
- fd = open(filename, O_RDWR); // warning: inaccurate check for symbolic link status of file
- if (fd == -1)
- return;
- }
- // ...
- }
+1. Function ``lstat`` is called on a file.
+2. The file is opened with ``open`` (without the ``O_NOFOLLOW`` flag).
+3. Optionally, file state is read again with function ``fstat``.
+4. If ``fstat`` was used, the previous and new state is compared incompletely.
+ In the correct (no-warning) case the fields ``st_mode``, ``st_ino`` and ``st_dev`` needs to be compared for equality.
+5. File is accessed for read or write (with variants of ``read`` or ``write`` functions).
+6. The ``S_ISLNK`` macro was used on the ``st_mode`` field of the file state value got from the ``lstat`` call in a condition part of a statement (can occur before or after opening the file).
-The checker produces a warning in similar cases when a file is opened after the
-``stat`` data was obtained for it and presence of symbolic link was checked by
-macro ``S_ISLNK``.
+If this situation is detected the checker emits a warning at step 5 (file read or write).
+If a call to ``fstat`` was found, there must be a comparison of ``st_mode``, ``st_ino`` and ``st_dev`` fields to the previous state to omit the warning.
-A secure way is to use the ``O_NOFOLLOW`` value in the ``flags`` argument at
-``open``. If this flag is not available on the implementation, the file status
-can be obtained a second time after the ``open`` call. If there is no difference
-between this data and the previously (before open) obtained data, the presence
-of symbolic link can be checked in a safe way.
+Examples:
.. code-block:: c
- void write_nosymlink(const char *filename, const char *buf, size_t size) {
- struct stat stat1;
+ void handle_file(const char *filename, const char *data, size_t size) {
+ struct stat fs;
int fd;
- if (lstat(filename, &stat1) == -1)
- return;
-
- fd = open(filename, 1);
- if (fd == -1)
- return;
-
- struct stat stat2;
- if (fstat(fd, &stat2) == -1) {
- // error: fstat failed
- // ...
+ if (lstat(filename, &fs) == -1)
return;
- }
- if (stat1.st_mode != stat2.st_mode || stat1.st_ino != stat2.st_ino || stat1.st_dev != stat2.st_dev) {
- // error: file was changed
- // ...
+ if (S_ISLNK(fs.st_mode))
return;
- }
- if (S_ISLNK(stat1.st_mode)) {
- // file is a symbolic link
- // ...
+ fd = open(filename, O_RDWR);
+ if (fd == -1)
return;
- }
- write(fd, buf, size);
+ write(fd, data, size); // warn
// ...
}
-It is important to compare all fields ``st_mode``, ``st_ino`` and ``st_dev`` of
-the ``stat`` structure. This checker emits additionally a warning if a file
-write or read attempt is made in a similar case when these comparisons are
-incomplete (or missing).
-
.. code-block:: c
void write_nosymlink(const char *filename, const char *buf, size_t size) {
@@ -2134,34 +2107,35 @@ incomplete (or missing).
if (lstat(filename, &stat1) == -1)
return;
- fd = open(filename, 1);
+ fd = open(filename, O_RDWR);
if (fd == -1)
return;
struct stat stat2;
if (fstat(fd, &stat2) == -1) {
+ // error: fstat failed
// ...
return;
}
- if (stat1.st_mode != stat2.st_mode) { // missing comparison of st_ino and st_dev
+ // correct condition is:
+ // (stat1.st_mode != stat2.st_mode || stat1.st_ino != stat2.st_ino || stat1.st_dev != stat2.st_dev)
+ if (stat1.st_mode != stat2.st_mode || stat1.st_ino != stat2.st_ino) {
+ // error: file was changed
// ...
return;
}
if (S_ISLNK(stat1.st_mode)) {
+ // file is a symbolic link
// ...
return;
}
- write(fd, buf, size); // warning: possibly missing check for external change of file before it was opened
+ write(fd, buf, size); // warn
// ...
}
-This kind of warning is produced when a ``lstat`` - ``open`` - ``fstat`` call
-sequence is found for the same file before write or read attempt (and the
-comparisons of status data are missing).
-
.. _security-VAList:
security.VAList (C, C++)
diff --git a/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
index a5a0c050ebc12e..8b3ae1eb636f3d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UnsafeSymlinkTestChecker.cpp
@@ -1,4 +1,4 @@
-//===-- UnsafeSymlinkTestChecker.cpp ------------------------------*- C++ -*--//
+//===-- UnsafeSymlinkTestChecker.cpp --------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -6,17 +6,9 @@
//
//===----------------------------------------------------------------------===//
//
-// Defines a checker that checks for unsafe symlink detection. This checks for
-// 2 related conditions:
-// - File status is read and used to detect symlink before the file is opened.
-// The file can be changed asynchronously between reading the status data and
-// opening the file, so this check is not safe to use.
-// - To fix the previous issue, the file status can be read after the open too
-// and compared to the previous value. If it did not change, the symlink
-// status is safely determined (the file can not be changed externally after
-// it was opened). The checker can detect a missing comparison of the "before"
-// and "after" status values.
-// (In all cases use of the O_NOFOLLOW flag at 'open' prevents the warning.)
+// Defines a checker that checks for incorrect symlink detection.
+// The checker works according to the rule CERT POS35-C. "Avoid race conditions
+// while checking for the existence of a symbolic link".
//
//===----------------------------------------------------------------------===//
@@ -39,10 +31,11 @@ namespace {
/// If created with a symbolic region, use the region as key.
/// If created with a string region, use the contained string as key (different
/// string regions with same content should be equal).
-struct FileNameKey {
+class FileNameKey {
std::string FileNameStr;
const MemRegion *Region = nullptr;
+public:
FileNameKey(const MemRegion *R) {
R = R->StripCasts();
if (const auto *SR = dyn_cast<StringRegion>(R))
@@ -51,26 +44,28 @@ struct FileNameKey {
Region = R;
}
- void Profile(llvm::FoldingSetNodeID &ID) const {
- ID.AddString(FileNameStr);
- ID.AddPointer(Region);
- }
-
bool operator==(const FileNameKey &RHS) const {
- return FileNameStr == RHS.FileNameStr && Region == RHS.Region;
+ return std::tie(FileNameStr, Region) ==
+ std::tie(RHS.FileNameStr, RHS.Region);
}
bool operator<(const FileNameKey &RHS) const {
- if (!Region && !RHS.Region)
- return FileNameStr < RHS.FileNameStr;
- return Region < RHS.Region;
+ return std::tie(FileNameStr, Region) <
+ std::tie(RHS.FileNameStr, RHS.Region);
}
+ const MemRegion *getRegionOrNull() const { return Region; }
+
std::string getFileName(llvm::StringRef PrefixStr) const {
if (!Region)
return (llvm::Twine(PrefixStr) + "'" + FileNameStr + "'").str();
return "";
}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.AddString(FileNameStr);
+ ID.AddPointer(Region);
+ }
};
/// Data maintained about a region belonging to a "struct stat".
@@ -85,15 +80,15 @@ struct StatData {
SVal StDevVal;
bool operator==(const StatData &D) const {
- return Region == D.Region && StModeVal == D.StModeVal &&
- StInoVal == D.StInoVal && StDevVal == D.StDevVal;
+ return std::tie(Region, StModeVal, StInoVal, StDevVal) ==
+ std::tie(D.Region, D.StModeVal, D.StInoVal, D.StDevVal);
}
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddPointer(Region);
- StModeVal.Profile(ID);
- StInoVal.Profile(ID);
- StDevVal.Profile(ID);
+ ID.Add(StModeVal);
+ ID.Add(StInoVal);
+ ID.Add(StDevVal);
}
};
@@ -105,42 +100,39 @@ struct FileDataLStat {
/// structure was performed, using the `S_ISLNK` macro.
bool LinkCheckPerformed;
- void Profile(llvm::FoldingSetNodeID &ID) const {
- LStatD.Profile(ID);
- ID.AddBoolean(LinkCheckPerformed);
+ bool operator==(const FileDataLStat &R) const {
+ return std::tie(LStatD, LinkCheckPerformed) ==
+ std::tie(R.LStatD, R.LinkCheckPerformed);
}
- bool operator==(const FileDataLStat &R) const {
- return LStatD == R.LStatD && LinkCheckPerformed == R.LinkCheckPerformed;
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.Add(LStatD);
+ ID.AddBoolean(LinkCheckPerformed);
}
};
-/// Data about a file after `lstat` and `open` was called (no symbolic link test
-/// with `S_ISLNK` was performed in between).
+/// Data about a file after `lstat` and `open` was called.
struct FileDataOpened {
/// Information about the `stat` structure that was passed to `lstat`.
StatData LStatD;
/// Information about the `stat` structure that was passed to `fstat`.
StatData FStatD;
+ /// Indicates if a test for symbolic link on the `st_mode` field of any of the
+ /// `stat` structures was performed, using the `S_ISLNK` macro.
+ bool LinkCheckPerformed;
/// Data about the file name (this is used for checker messages).
FileNameKey FName;
- void Profile(llvm::FoldingSetNodeID &ID) const {
- LStatD.Profile(ID);
- FStatD.Profile(ID);
- }
-
bool operator==(const FileDataOpened &R) const {
- return LStatD == R.LStatD && FStatD == R.FStatD;
+ return std::tie(LStatD, FStatD, LinkCheckPerformed) ==
+ std::tie(R.LStatD, R.FStatD, R.LinkCheckPerformed);
}
-};
-
-struct StatFieldsDecl {
- const FieldDecl *StModeFD;
- const FieldDecl *StInoFD;
- const FieldDecl *StDevFD;
- bool isValid() const { return StModeFD && StInoFD && StDevFD; }
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ ID.Add(LStatD);
+ ID.Add(FStatD);
+ ID.AddBoolean(LinkCheckPerformed);
+ }
};
struct ASTData {
@@ -157,18 +149,26 @@ struct ASTData {
class UnsafeSymlinkTestChecker
: public Checker<check::PostCall, check::BranchCondition,
- check::RegionChanges, check::DeadSymbols> {
- const CallDescription LStatFn{CDM::CLibrary, {"lstat"}, 2};
- const CallDescription OpenFn{CDM::CLibrary, {"open"}, 2};
- const CallDescription FStatFn{CDM::CLibrary, {"fstat"}, 2};
+ check::RegionChanges, check::DeadSymbols,
+ check::LiveSymbols> {
+ using FnHandler = std::function<void(const UnsafeSymlinkTestChecker *,
+ const CallEvent &, CheckerContext &)>;
+
+ CallDescriptionMap<FnHandler> Callbacks = {
+ {{CDM::CLibrary, {"lstat"}, 2}, &UnsafeSymlinkTestChecker::handleLStat},
+ {{CDM::CLibrary, {"open"}, 2}, &UnsafeSymlinkTestChecker::handleOpen},
+ {{CDM::CLibrary, {"fstat"}, 2}, &UnsafeSymlinkTestChecker::handleFStat},
+ };
const CallDescriptionSet FileAccessFn{
{CDM::CLibrary, {"write"}, 3}, {CDM::CLibrary, {"writev"}, 3},
{CDM::CLibrary, {"pwrite"}, 4}, {CDM::CLibrary, {"read"}, 3},
{CDM::CLibrary, {"readv"}, 3}, {CDM::CLibrary, {"pread"}, 4},
- {CDM::CLibrary, {"lseek"}, 3}};
+ {CDM::CLibrary, {"lseek"}, 3},
+ };
- const BugType BT{this, "Security error", "Incorrect check for symbolic link",
- false};
+ const BugType BT{this, "Security error",
+ "Race condition when checking for symbolic link",
+ /*SuppressOnSink=*/false};
mutable std::optional<ASTData> ASTValues;
@@ -182,35 +182,34 @@ class UnsafeSymlinkTestChecker
const StackFrame *SF,
const CallEvent *Call) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+ void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SymReaper) const;
private:
const SubRegion *castRegionToStructStat(const MemRegion *R,
- CheckerContext &C) const {
- if (!R)
- return nullptr;
- std::optional<const MemRegion *> CastR = C.getStoreManager().castRegion(
- R, C.getASTContext().getPointerType(ASTValues->StructStatType));
- if (!CastR)
- return R->getAs<SubRegion>();
- const SubRegion *SR = (*CastR)->getAs<SubRegion>();
- return SR ? SR : R->getAs<SubRegion>();
- }
+ CheckerContext &C) const;
+ const FieldRegion *getStModeRegion(const MemRegion *StatR,
+ CheckerContext &C) const;
+ /// Return the "pretty printed" name of a field of a MemRegion of a
+ /// "struct stat" object.
+ /// @param PrefixStr Print this before the field name.
+ /// @param EmptyStr Use this string if the field can not be pretty-printed.
+ std::string getFieldVarString(const MemRegion *StatR,
+ const FieldDecl *StatFieldD,
+ StringRef PrefixStr, StringRef EmptyStr,
+ CheckerContext &C) const;
StatData getStatData(const SubRegion *StatR, ProgramStateRef State,
- CheckerContext &C) const {
- MemRegionManager &RM = C.getStoreManager().getRegionManager();
- auto *StatR1 = castRegionToStructStat(StatR, C);
- auto GetFieldSVal = [&](const FieldDecl *FD) {
- return State->getSVal(RM.getFieldRegion(FD, StatR1));
- };
- return {StatR, GetFieldSVal(ASTValues->StModeFD),
- GetFieldSVal(ASTValues->StInoFD), GetFieldSVal(ASTValues->StDevFD)};
- }
+ CheckerContext &C) const;
const NoteTag *getNoteTag(const MemRegion *R, std::string Message,
CheckerContext &C) const;
+
void initData(const RecordDecl *StatDecl, const Preprocessor &PP) const;
+ void handleLStat(const CallEvent &Call, CheckerContext &C) const;
+ void handleOpen(const CallEvent &Call, CheckerContext &C) const;
+ void handleFStat(const CallEvent &Call, CheckerContext &C) const;
+ void handleFileAccess(const CallEvent &Call, CheckerContext &C) const;
};
-} // end anonymous namespace
+} // namespace
/// Data about files where `lstat` was called but not `open`.
REGISTER_MAP_WITH_PROGRAMSTATE(LStatCalledMap, FileNameKey, FileDataLStat)
@@ -218,15 +217,62 @@ REGISTER_MAP_WITH_PROGRAMSTATE(LStatCalledMap, FileNameKey, FileDataLStat)
/// Data about files where `lstat` and `open` was called.
REGISTER_MAP_WITH_PROGRAMSTATE(LStatOpenCalledMap, SymbolRef, FileDataOpened)
+const SubRegion *
+UnsafeSymlinkTestChecker::castRegionToStructStat(const MemRegion *R,
+ CheckerContext &C) const {
+ if (!R)
+ return nullptr;
+ std::optional<const MemRegion *> CastR = C.getStoreManager().castRegion(
+ R, C.getASTContext().getPointerType(ASTValues->StructStatType));
+ if (CastR) {
+ if (const SubRegion *SR = (*CastR)->getAs<SubRegion>())
+ return SR;
+ }
+ return R->getAs<SubRegion>();
+}
+
+const FieldRegion *
+UnsafeSymlinkTestChecker::getStModeRegion(const MemRegion *StatR,
+ CheckerContext &C) const {
+ return C.getStoreManager().getRegionManager().getFieldRegion(
+ ASTValues->StModeFD, castRegionToStructStat(StatR, C));
+}
+
+std::string UnsafeSymlinkTestChecker::getFieldVarString(
+ const MemRegion *StatR, const FieldDecl *StatFieldD, StringRef PrefixStr,
+ StringRef EmptyStr, CheckerContext &C) const {
+ const MemRegion *R = C.getStoreManager().getRegionManager().getFieldRegion(
+ StatFieldD, castRegionToStructStat(StatR, C));
+ if (!R->canPrintPretty())
+ return EmptyStr.str();
+ SmallString<64> Buf;
+ llvm::raw_svector_ostream Out(Buf);
+ Out << PrefixStr;
+ R->printPretty(Out);
+ return Out.str().str();
+}
+
+StatData UnsafeSymlinkTestChecker::getStatData(const SubRegion *StatR,
+ ProgramStateRef State,
+ CheckerContext &C) const {
+ MemRegionManager &RM = C.getStoreManager().getRegionManager();
+ auto *StatR1 = castRegionToStructStat(StatR, C);
+ auto GetFieldSVal = [&](const FieldDecl *FD) {
+ return State->getSVal(RM.getFieldRegion(FD, StatR1));
+ };
+ return {StatR, GetFieldSVal(ASTValues->StModeFD),
+ GetFieldSVal(ASTValues->StInoFD), GetFieldSVal(ASTValues->StDevFD)};
+}
+
const NoteTag *UnsafeSymlinkTestChecker::getNoteTag(const MemRegion *R,
std::string Message,
CheckerContext &C) const {
- return C.getNoteTag(
- [this, R, Message](PathSensitiveBugReport &BR) -> std::string {
- if (BR.isInteresting(R) && &BR.getBugType() == &BT)
- return Message;
- return "";
- });
+ return C.getNoteTag([this, R, M = std::move(Message)](
+ PathSensitiveBugReport &BR) -> std::string {
+ if (&BR.getBugType() == &BT && BR.isInteresting(R))
+ return M;
+ return "";
+ });
}
static const FieldDecl *findField(llvm::StringRef FieldName,
@@ -247,8 +293,8 @@ void UnsafeSymlinkTestChecker::initData(const RecordDecl *StatDecl,
findField("st_ino", StatDecl),
findField("st_dev", StatDecl),
StatDecl->getASTContext().getCanonicalTagType(StatDecl),
- 0,
- false};
+ /*O_NOFOLLOWValue=*/0,
+ /*IsValid=*/false};
if (std::optional<int> Val = tryExpandAsInteger("O_NOFOLLOW", PP))
ASTValues->O_NOFOLLOWValue = *Val;
} else {
@@ -257,142 +303,170 @@ void UnsafeSymlinkTestChecker::initData(const RecordDecl *StatDecl,
ASTValues->checkValid();
}
-void UnsafeSymlinkTestChecker::checkPostCall(const CallEvent &Call,
- CheckerContext &C) const {
- if (ASTValues && !ASTValues->IsValid)
- return;
+void UnsafeSymlinkTestChecker::handleLStat(const CallEvent &Call,
+ CheckerContext &C) const {
+ if (!ASTValues) {
+ const QualType T = Call.parameters()[1]->getType();
+ initData(T->isPointerType() ? T->getPointeeType()->getAsRecordDecl()
+ : nullptr,
+ C.getPreprocessor());
+ if (!ASTValues->IsValid)
+ return;
+ }
ProgramStateRef State = C.getState();
+ const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
+ const auto *StatReg =
+ dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
+ if (!FNameReg || !StatReg || !castRegionToStructStat(StatReg, C))
+ return;
- if (LStatFn.matches(Call)) {
- if (!ASTValues) {
- initData(
- Call.parameters()[1]->getType()->getPointeeType()->getAsRecordDecl(),
- C.getPreprocessor());
- if (!ASTValues->IsValid)
- return;
- }
-
- const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
- const auto *StatReg =
- dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
- if (!FNameReg || !StatReg)
- return;
+ FileNameKey FName(FNameReg);
+ State = State->set<LStatCalledMap>(FName,
+ {getStatData(StatReg, State, C), false});
+ C.addTransition(State,
+ getNoteTag(StatReg,
+ (llvm::Twine("File status") +
+ FName.getFileName(" of file ") + " is read here" +
+ getFieldVarString(StatReg, ASTValues->StModeFD,
+ " into ", "", C) +
+ " before opening the file")
+ .str(),
+ C));
+}
- FileNameKey FName(FNameReg);
- State = State->set<LStatCalledMap>(FName,
- {getStatData(StatReg, State, C), false});
- C.addTransition(State, getNoteTag(StatReg,
- (llvm::Twine("File status") +
- FName.getFileName(" of file ") +
- " is read here before opening the file")
- .str(),
- C));
+void UnsafeSymlinkTestChecker::handleOpen(const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
+ FileNameKey FName(FNameReg);
+ const FileDataLStat *LStatData = State->get<LStatCalledMap>(FName);
+ SymbolRef FileDescSym = Call.getReturnValue().getAsSymbol();
+ if (!FNameReg || !LStatData || !FileDescSym)
return;
- }
-
- if (OpenFn.matches(Call)) {
- const MemRegion *FNameReg = Call.getArgSVal(0).getAsRegion();
- FileNameKey FName(FNameReg);
- const FileDataLStat *LStatData = State->get<LStatCalledMap>(FName);
- SymbolRef FileDescSym = Call.getReturnValue().getAsSymbol();
- if (!FNameReg || !LStatData || !FileDescSym)
- return;
- State = State->remove<LStatCalledMap>(FNameReg);
+ State = State->remove<LStatCalledMap>(FNameReg);
- if (ASTValues->O_NOFOLLOWValue != 0) {
- const llvm::APSInt *FlagsValue =
- C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
- if (!FlagsValue) {
- C.addTransition(State);
- return;
- }
+ // If presence of O_NOFOLLOW can be verified, ignore this execution path.
+ // Otherwise it can be assumed that O_NOFOLLOW is not set, because when it is
+ // set S_ISLNK should not appear ('LinkCheckPerformed' will be false) so that
+ // case is still ignored.
+ if (ASTValues->O_NOFOLLOWValue != 0) {
+ const llvm::APSInt *FlagsValue =
+ C.getSValBuilder().getKnownValue(State, Call.getArgSVal(1));
+ if (FlagsValue) {
if (std::optional<int64_t> FVal = FlagsValue->tryExtValue();
FVal && (*FVal & ASTValues->O_NOFOLLOWValue)) {
C.addTransition(State);
return;
}
}
+ }
- if (!LStatData->LinkCheckPerformed) {
- State = State->set<LStatOpenCalledMap>(
- FileDescSym,
- {LStatData->LStatD, {nullptr, SVal{}, SVal{}, SVal{}}, FName});
- } else {
- if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BT,
- (llvm::Twine("Inaccurate check for symbolic link status of file") +
- FName.getFileName(" "))
- .str(),
- N);
- R->addNote("The file can be manipulated externally between calling "
- "'lstat' and opening the file",
- {Call.getSourceRange().getBegin(), C.getSourceManager()});
- R->addRange(Call.getSourceRange());
- R->markInteresting(LStatData->LStatD.Region);
- C.emitReport(std::move(R));
- return;
- }
- }
+ State = State->set<LStatOpenCalledMap>(FileDescSym,
+ {LStatData->LStatD,
+ {nullptr, SVal{}, SVal{}, SVal{}},
+ LStatData->LinkCheckPerformed,
+ FName});
+
+ C.addTransition(State, getNoteTag(LStatData->LStatD.Region,
+ (llvm::Twine("File") +
+ FName.getFileName(" ") + " is opened here")
+ .str(),
+ C));
+}
+
+void UnsafeSymlinkTestChecker::handleFStat(const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
+ const auto *FStatReg =
+ dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
+ if (!FileDescSym || !FStatReg || !castRegionToStructStat(FStatReg, C))
+ return;
+
+ const FileDataOpened *FileData = State->get<LStatOpenCalledMap>(FileDescSym);
+ if (!FileData)
+ return;
+
+ State = State->set<LStatOpenCalledMap>(
+ FileDescSym, {FileData->LStatD, getStatData(FStatReg, State, C),
+ FileData->LinkCheckPerformed, FileData->FName});
+ C.addTransition(
+ State,
+ getNoteTag(
+ FStatReg,
+ (llvm::Twine("File status") +
+ FileData->FName.getFileName(" of file ") + " is read here" +
+ getFieldVarString(FStatReg, ASTValues->StModeFD, " into ", "", C) +
+ " after opening the file")
+ .str(),
+ C));
+}
+
+void UnsafeSymlinkTestChecker::handleFileAccess(const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
+ if (!FileDescSym)
+ return;
+
+ const FileDataOpened *FileData = State->get<LStatOpenCalledMap>(FileDescSym);
+ if (!FileData)
+ return;
+
+ State = State->remove<LStatOpenCalledMap>(FileDescSym);
+ if (!FileData->LinkCheckPerformed) {
+ C.addTransition(State);
+ return;
}
- if (FStatFn.matches(Call)) {
- SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
- const auto *FStatReg =
- dyn_cast_or_null<SubRegion>(Call.getArgSVal(1).getAsRegion());
- if (!FileDescSym || !FStatReg)
- return;
- const FileDataOpened *FileData =
- State->get<LStatOpenCalledMap>(FileDescSym);
- if (!FileData)
- return;
- State = State->set<LStatOpenCalledMap>(
- FileDescSym,
- {FileData->LStatD, getStatData(FStatReg, State, C), FileData->FName});
- C.addTransition(State,
- getNoteTag(FStatReg,
- (llvm::Twine("File status") +
- FileData->FName.getFileName(" of file ") +
- " is read here after opening the file")
- .str(),
- C));
+ auto CheckEqual = [State, &C](SVal V1, SVal V2) {
+ auto DefVal1 = V1.getAs<DefinedOrUnknownSVal>();
+ auto DefVal2 = V2.getAs<DefinedOrUnknownSVal>();
+ if (!DefVal1 || !DefVal2)
+ return false;
+ DefinedOrUnknownSVal EQV =
+ C.getSValBuilder().evalEQ(State, *DefVal1, *DefVal2);
+ auto [EQTrue, EQFalse] = State->assume(EQV);
+ return EQTrue && !EQFalse;
+ };
+ if (FileData->FStatD.Region &&
+ CheckEqual(FileData->FStatD.StModeVal, FileData->LStatD.StModeVal) &&
+ CheckEqual(FileData->FStatD.StInoVal, FileData->LStatD.StInoVal) &&
+ CheckEqual(FileData->FStatD.StDevVal, FileData->LStatD.StDevVal)) {
+ C.addTransition(State);
return;
}
- if (FileAccessFn.contains(Call)) {
- SymbolRef FileDescSym = Call.getArgSVal(0).getAsSymbol();
- if (!FileDescSym)
- return;
- const FileDataOpened *FileData =
- State->get<LStatOpenCalledMap>(FileDescSym);
- if (!FileData)
- return;
- State = State->remove<LStatOpenCalledMap>(FileDescSym);
- if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
- auto R = std::make_unique<PathSensitiveBugReport>(
- BT,
- (llvm::Twine("Possibly missing check for external change of file") +
- FileData->FName.getFileName(" "))
- .str(),
- N);
- R->addNote(
- "File status was obtained before and after opening the file which "
- "indicates possible intent of a safe check for symbolic link",
- {Call.getSourceRange().getBegin(), C.getSourceManager()});
- R->addNote("For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' "
- "before and after open should be checked for equality",
- {Call.getSourceRange().getBegin(), C.getSourceManager()});
- R->addRange(Call.getSourceRange());
- R->markInteresting(FileData->LStatD.Region);
- R->markInteresting(FileData->FStatD.Region);
- C.emitReport(std::move(R));
- return;
- }
+ if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ BT,
+ (llvm::Twine("File") + FileData->FName.getFileName(" ") +
+ " might have been changed between call to 'lstat' and 'open' "
+ "therefore " +
+ getFieldVarString(FileData->LStatD.Region, ASTValues->StModeFD, "",
+ "the file status value", C) +
+ " may not contain the state of the file at open")
+ .str(),
+ N);
+ R->addRange(Call.getSourceRange());
+ R->markInteresting(FileData->LStatD.Region);
+ R->markInteresting(FileData->FStatD.Region);
+ C.emitReport(std::move(R));
+ return;
}
+}
- C.addTransition(State);
+void UnsafeSymlinkTestChecker::checkPostCall(const CallEvent &Call,
+ CheckerContext &C) const {
+ if (ASTValues && !ASTValues->IsValid)
+ return;
+
+ if (const FnHandler *Fn = Callbacks.lookup(Call))
+ (*Fn)(this, Call, C);
+ else if (FileAccessFn.contains(Call))
+ handleFileAccess(Call, C);
}
namespace {
@@ -445,54 +519,53 @@ void UnsafeSymlinkTestChecker::checkBranchCondition(const Stmt *S,
ExplodedNode *NewNode = C.getPredecessor();
LStatCalledMapTy LStatCalled = NewNode->getState()->get<LStatCalledMap>();
for (auto I : LStatCalled) {
- const FieldRegion *FR =
- C.getStoreManager().getRegionManager().getFieldRegion(
- ASTValues->StModeFD,
- castRegionToStructStat(I.second.LStatD.Region, C));
+ const FieldRegion *FR = getStModeRegion(I.second.LStatD.Region, C);
ProgramStateRef State = NewNode->getState();
FindMacroVisitor FindS_ISLNK(C, State, FR);
if (FindS_ISLNK.Visit(S)) {
State = State->set<LStatCalledMap>(I.first, {I.second.LStatD, true});
- NewNode =
- C.addTransition(State, NewNode,
- getNoteTag(I.second.LStatD.Region,
- (llvm::Twine("Possible test if file") +
- I.first.getFileName(" ") +
- " is a symbolic link detected here")
- .str(),
- C));
+ NewNode = C.addTransition(
+ State, NewNode,
+ getNoteTag(I.second.LStatD.Region,
+ (llvm::Twine(getFieldVarString(I.second.LStatD.Region,
+ ASTValues->StModeFD, "",
+ "File status value", C)) +
+ " is checked here for symbolic link")
+ .str(),
+ C));
}
}
- ProgramStateRef State = NewNode->getState();
- LStatOpenCalledMapTy LStatOpenCalled = State->get<LStatOpenCalledMap>();
- auto CheckEqual = [State, &C](SVal V1, SVal V2) {
- auto DefVal1 = V1.getAs<DefinedOrUnknownSVal>();
- auto DefVal2 = V2.getAs<DefinedOrUnknownSVal>();
- if (!DefVal1 || !DefVal2)
- return false;
- DefinedOrUnknownSVal EQV =
- C.getSValBuilder().evalEQ(State, *DefVal1, *DefVal2);
- auto [EQTrue, EQFalse] = State->assume(EQV);
- return EQTrue && !EQFalse;
- };
+ LStatOpenCalledMapTy LStatOpenCalled =
+ NewNode->getState()->get<LStatOpenCalledMap>();
for (auto I : LStatOpenCalled) {
- if (I.second.FStatD.Region)
- if (CheckEqual(I.second.FStatD.StModeVal, I.second.LStatD.StModeVal) &&
- CheckEqual(I.second.FStatD.StInoVal, I.second.LStatD.StInoVal) &&
- CheckEqual(I.second.FStatD.StDevVal, I.second.LStatD.StDevVal))
- State = State->remove<LStatOpenCalledMap>(I.first);
- }
+ if (I.second.LinkCheckPerformed)
+ continue;
- C.addTransition(State, NewNode);
+ const FieldRegion *FR = getStModeRegion(I.second.LStatD.Region, C);
+ ProgramStateRef State = NewNode->getState();
+ FindMacroVisitor FindS_ISLNK(C, State, FR);
+ if (FindS_ISLNK.Visit(S)) {
+ State = State->set<LStatOpenCalledMap>(
+ I.first, {I.second.LStatD, I.second.FStatD, true, I.second.FName});
+ NewNode = C.addTransition(
+ State, NewNode,
+ getNoteTag(I.second.LStatD.Region,
+ (llvm::Twine(getFieldVarString(I.second.LStatD.Region,
+ ASTValues->StModeFD, "",
+ "File status value", C)) +
+ " is checked here for symbolic link")
+ .str(),
+ C));
+ }
+ }
}
ProgramStateRef UnsafeSymlinkTestChecker::checkRegionChanges(
ProgramStateRef State, const InvalidatedSymbols *Invalidated,
ArrayRef<const MemRegion *> Explicits, ArrayRef<const MemRegion *> Regions,
const StackFrame *SF, const CallEvent *Call) const {
- if (Call && (LStatFn.matches(*Call) || OpenFn.matches(*Call) ||
- FStatFn.matches(*Call)))
+ if (Call && Callbacks.lookup(*Call))
return State;
if (Invalidated) {
@@ -503,8 +576,7 @@ ProgramStateRef UnsafeSymlinkTestChecker::checkRegionChanges(
for (const MemRegion *R : Regions)
InvalidatedR.insert(R);
for (auto I : State->get<LStatCalledMap>())
- if (!I.second.LinkCheckPerformed &&
- InvalidatedR.contains(I.second.LStatD.Region))
+ if (InvalidatedR.contains(I.second.LStatD.Region))
State = State->remove<LStatCalledMap>(I.first);
for (auto I : State->get<LStatOpenCalledMap>())
if (InvalidatedR.contains(I.second.LStatD.Region) ||
@@ -520,9 +592,10 @@ void UnsafeSymlinkTestChecker::checkDeadSymbols(SymbolReaper &SymReaper,
ProgramStateRef State = C.getState();
for (auto I : State->get<LStatCalledMap>()) {
- if (const auto *SymReg = dyn_cast_or_null<SymbolicRegion>(I.first.Region);
+ if (const auto *SymReg =
+ dyn_cast_or_null<SymbolicRegion>(I.first.getRegionOrNull());
SymReg && SymReg->getSymbol() && SymReaper.isDead(SymReg->getSymbol()))
- State = State->remove<LStatCalledMap>(I.first.Region);
+ State = State->remove<LStatCalledMap>(I.first.getRegionOrNull());
}
for (auto I : State->get<LStatOpenCalledMap>()) {
if (SymReaper.isDead(I.first))
@@ -532,6 +605,22 @@ void UnsafeSymlinkTestChecker::checkDeadSymbols(SymbolReaper &SymReaper,
C.addTransition(State);
}
+void UnsafeSymlinkTestChecker::checkLiveSymbols(ProgramStateRef State,
+ SymbolReaper &SymReaper) const {
+ if (!ASTValues || !ASTValues->IsValid)
+ return;
+
+ // The SVal objects at these regions may be needed at checkFileAccess later.
+ // These values may expire before that point if the parent region is not
+ // marked as live.
+ for (auto I : State->get<LStatOpenCalledMap>()) {
+ if (I.second.FStatD.Region)
+ SymReaper.markLive(I.second.FStatD.Region);
+ if (I.second.LStatD.Region)
+ SymReaper.markLive(I.second.LStatD.Region);
+ }
+}
+
void ento::registerUnsafeSymlinkTestChecker(CheckerManager &mgr) {
mgr.registerChecker<UnsafeSymlinkTestChecker>();
}
diff --git a/clang/test/Analysis/unsafe-symlink-test-notes.c b/clang/test/Analysis/unsafe-symlink-test-notes.c
index 3454b72350073b..3a07a63062cb77 100644
--- a/clang/test/Analysis/unsafe-symlink-test-notes.c
+++ b/clang/test/Analysis/unsafe-symlink-test-notes.c
@@ -13,43 +13,47 @@ typedef size_t ssize_t;
int lstat(const char *restrict path, struct stat *restrict buf);
int open(const char *path, int oflag);
ssize_t write(int fildes, const void *buf, size_t nbyte);
+ssize_t read(int fildes, void *buf, size_t nbyte);
int fstat(int fildes, struct stat *buf);
#define S_ISLNK(M) ((M & 2) != 0)
-void test_fstat_single(const char *filename, const char *buf, size_t size) {
+void test_simple(const char *filename, const char *buf, size_t size) {
struct stat stat1;
int fd;
- if (lstat(filename, &stat1) == -1) // expected-note{{File status is read here before opening the file}} \\
+ if (lstat(filename, &stat1) == -1) // expected-note{{File status is read here into 'stat1.st_mode' before opening the file}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
- fd = open(filename, 1);
+ if (S_ISLNK(stat1.st_mode)) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{'stat1.st_mode' is checked here for symbolic link}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ fd = open(filename, 1); // expected-note{{File is opened here}}
if (fd == -1) // expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
struct stat stat2;
- if (fstat(fd, &stat2) == -1) // expected-note{{File status is read here after opening the file}} \\
+ if (fstat(fd, &stat2) == -1) // expected-note{{File status is read here into 'stat2.st_mode' after opening the file}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
- write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
- // expected-note{{Possibly missing check for external change of file}} \\
- // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
- // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ write(fd, buf, size); // expected-warning{{File might have been changed between call to 'lstat' and 'open' therefore 'stat1.st_mode' may not contain the state of the file at open}} \\
+ // expected-note{{File might have been changed between call to 'lstat' and 'open' therefore 'stat1.st_mode' may not contain the state of the file at open}}
}
-void test_fstat_2(const char *fn2, const char *buf, size_t size) {
+void test_multiple(const char *fn2, const char *buf, size_t size) {
const char *const fn1 = "x/y.z";
struct stat lstat1;
struct stat lstat2;
int fd1, fd2;
- if (lstat(fn1, &lstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here before opening the file}} \\
+ if (lstat(fn1, &lstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here into 'lstat1.st_mode' before opening the file}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
@@ -57,7 +61,14 @@ void test_fstat_2(const char *fn2, const char *buf, size_t size) {
// expected-note{{Taking false branch}}
return;
- fd1 = open(fn1, 1);
+ if (S_ISLNK(lstat1.st_mode) || S_ISLNK(lstat2.st_mode)) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{'lstat1.st_mode' is checked here for symbolic link}} \\
+ // expected-note{{Left side of '||' is false}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ fd1 = open(fn1, 1); // expected-note{{File 'x/y.z' is opened here}}
if (fd1 == -1) // expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
@@ -68,7 +79,7 @@ void test_fstat_2(const char *fn2, const char *buf, size_t size) {
struct stat fstat1;
struct stat fstat2;
- if (fstat(fd1, &fstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here after opening the file}} \\
+ if (fstat(fd1, &fstat1) == -1) // expected-note{{File status of file 'x/y.z' is read here into 'fstat1.st_mode' after opening the file}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
@@ -82,34 +93,57 @@ void test_fstat_2(const char *fn2, const char *buf, size_t size) {
// expected-note{{Assuming 'fstat2.st_ino' is equal to 'lstat2.st_ino'}} \\
// expected-note{{Left side of '&&' is true}} \\
// expected-note{{Assuming 'fstat2.st_dev' is equal to 'lstat2.st_dev'}} \\
- // expected-note{{'fstat2.st_dev' is equal to 'lstat2.st_dev'}} \\
// expected-note{{Taking true branch}}
write(fd2, buf, size);
- write(fd1, buf, size); // expected-warning{{Possibly missing check for external change of file 'x/y.z'}} \\
- // expected-note{{Possibly missing check for external change of file 'x/y.z'}} \\
- // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
- // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ write(fd1, buf, size); // expected-warning{{File 'x/y.z' might have been changed between call to 'lstat' and 'open' therefore 'lstat1.st_mode' may not contain the state of the file at open}} \\
+ // expected-note{{File 'x/y.z' might have been changed between call to 'lstat' and 'open' therefore 'lstat1.st_mode' may not contain the state of the file at open}}
}
}
const char *const g_filename = "a/b/c";
-void test_islnk() {
+void test_nofstat() {
struct stat lstat_info;
int fd;
- if (lstat(g_filename, &lstat_info) == -1) // expected-note{{File status of file 'a/b/c' is read here before opening the file}} \\
+ if (lstat(g_filename, &lstat_info) == -1) // expected-note{{File status of file 'a/b/c' is read here into 'lstat_info.st_mode' before opening the file}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking false branch}}
return;
- if (!S_ISLNK(lstat_info.st_mode)) { // expected-note{{Possible test if file 'a/b/c' is a symbolic link detected here}} \\
+ if (!S_ISLNK(lstat_info.st_mode)) { // expected-note{{'lstat_info.st_mode' is checked here for symbolic link}} \\
// expected-note{{Assuming the condition is false}} \\
// expected-note{{Taking true branch}}
- fd = open(g_filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file 'a/b/c'}} \\
- // expected-note{{Inaccurate check for symbolic link status of file 'a/b/c'}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
- if (fd == -1)
+ fd = open(g_filename, 1); // expected-note{{File 'a/b/c' is opened here}}
+ if (fd == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
return;
+
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File 'a/b/c' might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}} \\
+ // expected-note{{File 'a/b/c' might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}}
}
}
+
+void test_stat_param(const char *filename, struct stat *lstatd) {
+ int fd;
+
+ if (lstat(filename, lstatd) == -1) // expected-note{{File status is read here into field 'st_mode' before opening the file}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ if (S_ISLNK(lstatd->st_mode)) // expected-note{{field 'st_mode' is checked here for symbolic link}} \\
+ // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ fd = open(filename, 1); // expected-note{{File is opened here}}
+ if (fd == -1) // expected-note{{Assuming the condition is false}} \\
+ // expected-note{{Taking false branch}}
+ return;
+
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File might have been changed between call to 'lstat' and 'open' therefore field 'st_mode' may not contain the state of the file at open}} \\
+ // expected-note{{File might have been changed between call to 'lstat' and 'open' therefore field 'st_mode' may not contain the state of the file at open}}
+}
diff --git a/clang/test/Analysis/unsafe-symlink-test.c b/clang/test/Analysis/unsafe-symlink-test.c
index 7719d02a0e9f8c..08fb488bba3243 100644
--- a/clang/test/Analysis/unsafe-symlink-test.c
+++ b/clang/test/Analysis/unsafe-symlink-test.c
@@ -1,5 +1,5 @@
// RUN: %clang_analyze_cc1 %s -triple=x86_64-unknown-linux \
-// RUN: -verify \
+// RUN: -verify -analyzer-config eagerly-assume=false \
// RUN: -analyzer-checker=core,security.UnsafeSymlinkTest
struct stat {
@@ -20,7 +20,7 @@ int fstat(int fildes, struct stat *buf);
#define O_NOFOLLOW (4)
#define O_OTHER (2)
-void test_islnk_local(const char *filename) {
+void test_lstat_islnk_open(const char *filename) {
struct stat lstat_info;
int fd;
@@ -28,385 +28,350 @@ void test_islnk_local(const char *filename) {
return;
if (!S_ISLNK(lstat_info.st_mode)) {
- fd = open(filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ fd = open(filename, 1);
if (fd == -1)
return;
- }
-}
-
-void test_islnk_param(const char *filename, struct stat *lstat_info) {
- int fd;
-
- if (lstat(filename, lstat_info) == -1)
- return;
- if (!S_ISLNK(lstat_info->st_mode)) {
- fd = open(filename, O_OTHER); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
- if (fd == -1)
- return;
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}}
}
}
-void test_no_islnk(const char *filename) {
- struct stat lstat_info;
+void test_lstat_islnk_open_fstat(const char *filename) {
+ struct stat lstat_info, fstat_info;
int fd;
if (lstat(filename, &lstat_info) == -1)
return;
- if (lstat_info.st_mode > 1) {
- fd = open(filename, O_OTHER); // no-warning
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open(filename, 1);
if (fd == -1)
return;
+
+ if (fstat(fd, &fstat_info) != -1) {
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}}
+ }
}
}
-void test_islnk_nofollow(const char *filename) {
- struct stat lstat_info;
+void test_lstat_open_islnk_fstat_nocompare() {
+ struct stat lstat_info, fstat_info;
int fd;
- if (lstat(filename, &lstat_info) == -1)
+ if (lstat("filename", &lstat_info) == -1)
return;
- if (!S_ISLNK(lstat_info.st_mode)) {
- fd = open(filename, O_NOFOLLOW | O_OTHER); // no-warning
- if (fd == -1)
- return;
+ fd = open("filename", 1);
+ if (fd == -1)
+ return;
+
+ if (!S_ISLNK(lstat_info.st_mode) && fstat(fd, &fstat_info) != -1 && lstat_info.st_mode == fstat_info.st_mode) {
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File 'filename' might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}}
}
}
-void test_lstat_other(const char *filename, struct stat *lstat_info1, struct stat *lstat_info2) {
+void test_lstat_open_islnk_fstat_nocompare_p(struct stat *fstat_info) {
+ struct stat lstat_info;
int fd;
- if (lstat(filename, lstat_info1) == -1)
+ if (lstat("filename", &lstat_info) == -1)
return;
- if (lstat("file", lstat_info2) == -1)
+ fd = open("filename", 1);
+ if (fd == -1)
return;
- if (!S_ISLNK(lstat_info2->st_mode)) {
- fd = open(filename, 1); // no-warning
- if (fd == -1)
- return;
+ if (!S_ISLNK(lstat_info.st_mode) && fstat(fd, fstat_info) != -1 && lstat_info.st_mode == fstat_info->st_mode) {
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File 'filename' might have been changed between call to 'lstat' and 'open' therefore 'lstat_info.st_mode' may not contain the state of the file at open}}
}
}
-void test_lstat_multi(const char *filename1, const char *filename2) {
- struct stat lstat_info1;
- struct stat lstat_info2;
+void test_lstat_open_fstat_noislnk(const char *filename) {
+ struct stat lstat_info, fstat_info;
+ int fd;
- if (lstat(filename1, &lstat_info1) == -1)
+ if (lstat(filename, &lstat_info) == -1)
return;
- if (lstat(filename2, &lstat_info2) == -1)
+
+ fd = open(filename, 1);
+ if (fd == -1)
return;
- if (!S_ISLNK(lstat_info1.st_mode) && !S_ISLNK(lstat_info2.st_mode)) {
- int fd1 = open(filename1, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
- if (fd1 == -1)
- return;
- int fd2 = open(filename2, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
- if (fd2 == -1)
- return;
+ if (fstat(fd, &fstat_info) != -1 && !S_ISLNK(fstat_info.st_mode)) {
+ char buf[10];
+ read(fd, buf, 10); // no-warning
}
}
-void test_lstat_str_const() {
- struct stat lstat_info;
+void test_lstat_islnk_open_fstat_compare(const char *filename) {
+ struct stat lstat_info, fstat_info;
int fd;
- if (lstat("x/y", &lstat_info) == -1)
+ if (lstat(filename, &lstat_info) == -1)
return;
if (!S_ISLNK(lstat_info.st_mode)) {
- fd = open("x/y", 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
+ fd = open(filename, 1);
if (fd == -1)
return;
+
+ if (fstat(fd, &fstat_info) != -1 && lstat_info.st_mode == fstat_info.st_mode && lstat_info.st_ino == fstat_info.st_ino && lstat_info.st_dev == fstat_info.st_dev) {
+ char buf[10];
+ read(fd, buf, 10); // no-warning
+ }
}
}
-void test_fstat_nocheck(const char *filename, char *buf, size_t size) {
- struct stat lstat_info;
+void test_lstat_islnk_open_fstat_compare_other(const char *filename) {
+ struct stat lstat_info, fstat_info;
int fd;
if (lstat(filename, &lstat_info) == -1)
return;
+ if (S_ISLNK(lstat_info.st_mode))
+ return;
+
fd = open(filename, 1);
if (fd == -1)
return;
- struct stat stat1;
- if (fstat(fd, &stat1) == -1)
+ if (fstat(fd, &fstat_info) == -1)
+ return;
+
+ if (lstat_info.st_mode != fstat_info.st_mode || lstat_info.st_dev != fstat_info.st_dev || fstat_info.st_ino != lstat_info.st_ino)
return;
- read(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
- // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
- // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ char buf[10];
+ read(fd, buf, 10); // no-warning
}
-void test_fstat_badcheck(const char *filename, const char *buf, size_t size) {
- struct stat stat1;
+void test_lstat_islnk_open_fstat_compare_p(const char *filename, struct stat *fstat_info) {
+ struct stat lstat_info;
int fd;
- if (lstat(filename, &stat1) == -1)
- return;
-
- fd = open(filename, 1);
- if (fd == -1)
+ if (lstat(filename, &lstat_info) == -1)
return;
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
- return;
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
- if (stat1.st_mode == stat2.st_mode)
- write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
- // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
- // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ if (fstat(fd, fstat_info) != -1 && fstat_info->st_mode == lstat_info.st_mode && lstat_info.st_ino == fstat_info->st_ino && fstat_info->st_dev == lstat_info.st_dev) {
+ char buf[10];
+ read(fd, buf, 10); // no-warning
+ }
+ }
}
-void test_fstat_badcheck_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+void test_lstat_open_noislnk(const char *filename) {
+ struct stat lstat_info, fstat_info;
int fd;
- if (lstat(filename, stat1) == -1)
+ if (lstat(filename, &lstat_info) == -1)
return;
fd = open(filename, 1);
if (fd == -1)
return;
- if (fstat(fd, stat2) == -1)
- return;
-
- if (stat1->st_mode == stat2->st_mode)
- write(fd, buf, size); // expected-warning{{Possibly missing check for external change of file}} \\
- // expected-note{{File status was obtained before and after opening the file which indicates possible intent of a safe check for symbolic link}} \\
- // expected-note{{For a safe check the fields 'st_mode', 'st_ino' and 'st_dev' before and after open should be checked for equality}}
+ char buf[10];
+ read(fd, buf, 10); // no-warning
}
-void test_fstat_goodcheck(const char *filename, const char *buf, size_t size) {
- struct stat stat1;
+const char *const GlobalFName = "aaa/bbb";
+
+void test_stat_param(struct stat *lstatd) {
int fd;
- if (lstat(filename, &stat1) == -1)
+ if (lstat(GlobalFName, lstatd) == -1)
return;
- fd = open(filename, 1);
- if (fd == -1)
+ if (S_ISLNK(lstatd->st_mode))
return;
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
+ fd = open(GlobalFName, O_OTHER);
+ if (fd == -1)
return;
- if (stat1.st_mode == stat2.st_mode && stat1.st_ino == stat2.st_ino && stat1.st_dev == stat2.st_dev)
- write(fd, buf, size); // no-warning
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File 'aaa/bbb' might have been changed between call to 'lstat' and 'open' therefore field 'st_mode' may not contain the state of the file at open}}
}
-void test_fstat_goodcheck_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+void test_nofollow(const char *filename) {
+ struct stat lstat_info;
int fd;
- if (lstat(filename, stat1) == -1)
- return;
-
- fd = open(filename, 1);
- if (fd == -1)
- return;
-
- if (fstat(fd, stat2) == -1)
+ if (lstat(filename, &lstat_info) == -1)
return;
- if (stat1->st_mode == stat2->st_mode && stat1->st_ino == stat2->st_ino && stat1->st_dev == stat2->st_dev)
- write(fd, buf, size); // no-warning
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open(filename, O_NOFOLLOW | O_OTHER);
+ if (fd == -1)
+ return;
+ char buf[10];
+ read(fd, buf, 10); // no-warning
+ }
}
-void test_fstat_nofollow_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+void test_nofollow_unknown(int flags) {
+ struct stat lstat_info;
int fd;
- if (lstat(filename, stat1) == -1)
- return;
-
- fd = open(filename, O_NOFOLLOW);
- if (fd == -1)
- return;
-
- if (fstat(fd, stat2) == -1)
+ if (lstat("f", &lstat_info) == -1)
return;
- write(fd, buf, size); // no-warning
+ if (!S_ISLNK(lstat_info.st_mode)) {
+ fd = open("f", flags);
+ if (fd == -1)
+ return;
+ char buf[10];
+ read(fd, buf, 10); // expected-warning{{File 'f' might have been changed between call to 'lstat' and 'open'}}
+ }
}
-void test_fstat_nofollow_unknown(const char *filename, const char *buf, size_t size, int flags) {
+void test_another_file(const char *filename, struct stat *lstat_info1, struct stat *lstat_info2) {
int fd;
- struct stat stat1;
- struct stat stat2;
-
- if (lstat(filename, &stat1) == -1)
- return;
- fd = open(filename, flags);
- if (fd == -1)
+ if (lstat(filename, lstat_info1) == -1)
return;
- if (fstat(fd, &stat2) == -1)
+ if (lstat("file", lstat_info2) == -1)
return;
- write(fd, buf, size); // no-warning
+ if (!S_ISLNK(lstat_info2->st_mode)) {
+ fd = open(filename, 1);
+ if (fd == -1)
+ return;
+ char buf[10];
+ read(fd, buf, 10); // no-warning
+ }
}
-extern void f_stat(struct stat *);
-extern void f_fd(int *);
-
-void test_fstat_inval1(const char *filename, const char *buf, size_t size) {
- struct stat stat_e1;
- int fd;
+void test_more_files(const char *filename1, const char *filename2) {
+ struct stat lstat_info1;
+ struct stat lstat_info2;
- if (lstat(filename, &stat_e1) == -1)
+ if (lstat(filename1, &lstat_info1) == -1)
return;
-
- f_stat(&stat_e1);
-
- fd = open(filename, 1);
- if (fd == -1)
+ if (lstat(filename2, &lstat_info2) == -1)
return;
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
- return;
+ if (!S_ISLNK(lstat_info1.st_mode) && !S_ISLNK(lstat_info2.st_mode)) {
+ int fd1 = open(filename1, 1);
+ if (fd1 == -1)
+ return;
+ int fd2 = open(filename2, 1);
+ if (fd2 == -1)
+ return;
- write(fd, buf, size);
+ char buf[10];
+ read(fd1, buf, 10); // expected-warning{{File might have been changed between call to 'lstat' and 'open'}}
+ read(fd2, buf, 10); // expected-warning{{File might have been changed between call to 'lstat' and 'open'}}
+ }
}
-void test_fstat_inval2(const char *filename, const char *buf, size_t size) {
+extern void f_stat(struct stat *);
+extern void f_fd(int *);
+
+void test_lstat_inval_open(const char *buf, size_t size) {
struct stat stat1;
int fd;
- if (lstat(filename, &stat1) == -1)
+ if (lstat("a/b", &stat1) == -1)
return;
- fd = open(filename, 1);
- if (fd == -1)
+ if (S_ISLNK(stat1.st_mode))
return;
f_stat(&stat1);
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
+ fd = open("a/b", 1);
+ if (fd == -1)
return;
- write(fd, buf, size);
+ write(fd, buf, size); // no-warning
}
-void test_fstat_inval3(const char *filename, const char *buf, size_t size) {
+void test_lstat_open_inval(const char *buf, size_t size) {
struct stat stat1;
int fd;
- if (lstat(filename, &stat1) == -1)
+ if (lstat("a/b", &stat1) == -1)
return;
- fd = open(filename, 1);
- if (fd == -1)
+ if (S_ISLNK(stat1.st_mode))
return;
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
+ fd = open("a/b", 1);
+ if (fd == -1)
return;
f_stat(&stat1);
- write(fd, buf, size);
+ write(fd, buf, size); // no-warning
}
-void test_fstat_inval4(const char *filename, const char *buf, size_t size) {
- struct stat stat1;
+void test_lstat_open_fstat_inval(const char *buf, size_t size) {
+ struct stat stat1, stat2;
int fd;
- if (lstat(filename, &stat1) == -1)
+ if (lstat("a/b", &stat1) == -1)
return;
- fd = open(filename, 1);
+ if (S_ISLNK(stat1.st_mode))
+ return;
+
+ fd = open("a/b", 1);
if (fd == -1)
return;
- struct stat stat2;
if (fstat(fd, &stat2) == -1)
return;
f_stat(&stat2);
- write(fd, buf, size);
+ write(fd, buf, size); // no-warning
}
-void test_fstat_inval5(const char *filename, const char *buf, size_t size) {
+void test_inval_fd(const char *buf, size_t size) {
struct stat stat1;
int fd;
- if (lstat(filename, &stat1) == -1)
+ if (lstat("a/b", &stat1) == -1)
return;
- fd = open(filename, 1);
- if (fd == -1)
+ if (S_ISLNK(stat1.st_mode))
return;
- struct stat stat2;
- if (fstat(fd, &stat2) == -1)
+ fd = open("a/b", 1);
+ if (fd == -1)
return;
f_fd(&fd);
- write(fd, buf, size);
+ write(fd, buf, size); // no-warning
}
-void test_fstat_inval_p(const char *filename, const char *buf, size_t size, struct stat *stat1, struct stat *stat2) {
+void test_inval_p(struct stat *stat1, const char *buf, size_t size) {
int fd;
- if (lstat(filename, stat1) == -1)
+ if (lstat("a/b", stat1) == -1)
return;
- fd = open(filename, 1);
- if (fd == -1)
+ if (S_ISLNK(stat1->st_mode))
return;
f_stat(stat1);
- if (fstat(fd, stat2) == -1)
- return;
-
- if (stat1->st_mode == stat2->st_mode && stat1->st_ino == stat2->st_ino && stat1->st_dev == stat2->st_dev)
- write(fd, buf, size); // no-warning
-}
-
-void test_islnk_inval1_p(const char *filename, struct stat *lstat_info) {
- int fd;
-
- if (lstat(filename, lstat_info) == -1)
- return;
-
- f_stat(lstat_info);
-
- if (!S_ISLNK(lstat_info->st_mode)) {
- fd = open(filename, 1); // no-warning
- if (fd == -1)
- return;
- }
-}
-
-void test_islnk_inval2_p(const char *filename, struct stat *lstat_info) {
- int fd;
-
- if (lstat(filename, lstat_info) == -1)
+ fd = open("a/b", 1);
+ if (fd == -1)
return;
- if (!S_ISLNK(lstat_info->st_mode)) {
- f_stat(lstat_info);
- fd = open(filename, 1); // expected-warning{{Inaccurate check for symbolic link status of file}} \\
- // expected-note{{The file can be manipulated externally between calling 'lstat' and opening the file}}
- if (fd == -1)
- return;
- }
+ write(fd, buf, size); // no-warning
}
More information about the cfe-commits
mailing list