Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace call to stat, lstat, and maybeLstat with appropriate functions from std::filesystem #10937

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open
8 changes: 4 additions & 4 deletions src/libexpr/flake/flakeref.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "url-parts.hh"
#include "fetchers.hh"
#include "registry.hh"
#include <filesystem>

namespace nix {

Expand Down Expand Up @@ -101,8 +102,7 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
path = absPath(path, baseDir);

if (isFlake) {

if (!S_ISDIR(lstat(path).st_mode)) {
if (!std::filesystem::is_directory(std::filesystem::symlink_status(path))) {
if (baseNameOf(path) == "flake.nix") {
// Be gentle with people who accidentally write `/foo/bar/flake.nix` instead of `/foo/bar`
warn(
Expand All @@ -119,7 +119,7 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
notice("path '%s' does not contain a 'flake.nix', searching up",path);

// Save device to detect filesystem boundary
dev_t device = lstat(path).st_dev;
dev_t device = unix::lstat(path).st_dev;
bool found = false;
while (path != "/") {
if (pathExists(path + "/flake.nix")) {
Expand All @@ -128,7 +128,7 @@ std::pair<FlakeRef, std::string> parsePathFlakeRefWithFragment(
} else if (pathExists(path + "/.git"))
throw Error("path '%s' is not part of a flake (neither it nor its parent directories contain a 'flake.nix' file)", path);
else {
if (lstat(path).st_dev != device)
if (unix::lstat(path).st_dev != device)
throw Error("unable to find a flake before encountering filesystem boundary at '%s'", path);
}
path = dirOf(path);
Expand Down
5 changes: 3 additions & 2 deletions src/libfetchers/mercurial.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "store-path-accessor.hh"
#include "fetch-settings.hh"

#include <filesystem>
#include <sys/time.h>

using namespace std::string_literals;
Expand Down Expand Up @@ -199,9 +200,9 @@ struct MercurialInputScheme : InputScheme
assert(hasPrefix(p, actualPath));
std::string file(p, actualPath.size() + 1);

auto st = lstat(p);
auto st = std::filesystem::symlink_status(p);

if (S_ISDIR(st.st_mode)) {
if (std::filesystem::is_directory(st)) {
auto prefix = file + "/";
auto i = files.lower_bound(prefix);
return i != files.end() && hasPrefix(*i, prefix);
Expand Down
36 changes: 22 additions & 14 deletions src/libstore/build/derivation-goal.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "derivation-goal.hh"
#include <filesystem>
#ifndef _WIN32 // TODO enable build hook on Windows
# include "hook-instance.hh"
#endif
Expand Down Expand Up @@ -770,35 +771,42 @@ void DerivationGoal::tryLocalBuild() {
}


static void chmod_(const Path & path, mode_t mode)
{
if (chmod(path.c_str(), mode) == -1)
throw SysError("setting permissions on '%s'", path);
}


/* Move/rename path 'src' to 'dst'. Temporarily make 'src' writable if
it's a directory and we're not root (to be able to update the
directory's parent link ".."). */
static void movePath(const Path & src, const Path & dst)
{
auto st = lstat(src);
namespace fs = std::filesystem;
auto st = fs::symlink_status(src);

bool changePerm = (
#ifndef _WIN32
geteuid()
#else
!isRootUser()
#endif
&& S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR));
&& fs::is_directory(st)
&& (st.permissions() & fs::perms::owner_write) == fs::perms::none);

if (changePerm)
chmod_(src, st.st_mode | S_IWUSR);
auto permOpts = fs::perm_options::replace | fs::perm_options::nofollow;

std::filesystem::rename(src, dst);
if (changePerm) {
try {
fs::permissions(src, st.permissions() | fs::perms::owner_write, permOpts);
} catch (const fs::filesystem_error & e) {
throw SysError("setting permissions on '%s'", src);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does SysError actually work correctly after a std::filesystem exception? I don't know if it's guaranteed to leave errno in a known state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation says that fs::filesystem_error is only thrown on OS errors so I think it's safe to assume that the errno is appropriately set when we're in this catch block?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably a safe assumption on Unix-like systems, but I'm not sure if it's the case on Windows. Maybe @Ericson2314 knows?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose on windows we than get these long error codes instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ericson2314 I have changed the code to use the SysError with explicit errNo (see e0090fe)

}
}

fs::rename(src, dst);

if (changePerm)
chmod_(dst, st.st_mode);
if (changePerm) {
try {
fs::permissions(dst, st.permissions(), permOpts);
} catch (const fs::filesystem_error & e) {
throw SysError("setting permissions on '%s'", dst);
}
}
}


Expand Down
43 changes: 24 additions & 19 deletions src/libstore/builtins/buildenv.cc
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "buildenv.hh"
#include "derivations.hh"
#include "file-system.hh"
#include "signals.hh"

#include <filesystem>
#include <sys/stat.h>
#include <sys/types.h>
#include <fcntl.h>
Expand All @@ -18,11 +20,13 @@ struct State
/* For each activated package, create symlinks */
static void createLinks(State & state, const Path & srcDir, const Path & dstDir, int priority)
{
std::filesystem::directory_iterator srcFiles;
namespace fs = std::filesystem;

fs::directory_iterator srcFiles;

try {
srcFiles = std::filesystem::directory_iterator{srcDir};
} catch (std::filesystem::filesystem_error & e) {
srcFiles = fs::directory_iterator{srcDir};
} catch (fs::filesystem_error & e) {
if (e.code() == std::errc::not_a_directory) {
warn("not including '%s' in the user environment because it's not a directory", srcDir);
return;
Expand All @@ -36,19 +40,20 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
if (name.string()[0] == '.')
/* not matched by glob */
continue;
auto srcFile = (std::filesystem::path{srcDir} / name).string();
auto dstFile = (std::filesystem::path{dstDir} / name).string();
auto srcFile = (fs::path{srcDir} / name).string();
auto dstFile = (fs::path{dstDir} / name).string();

fs::file_status srcSt;

struct stat srcSt;
try {
if (stat(srcFile.c_str(), &srcSt) == -1)
throw SysError("getting status of '%1%'", srcFile);
} catch (SysError & e) {
if (e.errNo == ENOENT || e.errNo == ENOTDIR) {
srcSt = fs::status(srcFile);

if (srcSt.type() == fs::file_type::not_found) {
warn("skipping dangling symlink '%s'", dstFile);
continue;
}
throw;
} catch (fs::filesystem_error & e) {
throw SysError("getting status of '%1%'", srcFile);
}

/* The files below are special-cased to that they don't show
Expand All @@ -66,16 +71,16 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
hasSuffix(srcFile, "/manifest.json"))
continue;

else if (S_ISDIR(srcSt.st_mode)) {
auto dstStOpt = maybeLstat(dstFile.c_str());
else if (fs::is_directory(srcSt)) {
auto dstStOpt = maybeSymlinkStat(dstFile);
if (dstStOpt) {
auto & dstSt = *dstStOpt;
if (S_ISDIR(dstSt.st_mode)) {
if (fs::is_directory(dstSt)) {
createLinks(state, srcFile, dstFile, priority);
continue;
} else if (S_ISLNK(dstSt.st_mode)) {
} else if (fs::is_symlink(dstSt)) {
auto target = canonPath(dstFile, true);
if (!S_ISDIR(lstat(target).st_mode))
if (!fs::is_directory(fs::symlink_status(target)))
throw Error("collision between '%1%' and non-directory '%2%'", srcFile, target);
if (unlink(dstFile.c_str()) == -1)
throw SysError("unlinking '%1%'", dstFile);
Expand All @@ -93,10 +98,10 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
}

else {
auto dstStOpt = maybeLstat(dstFile.c_str());
auto dstStOpt = maybeSymlinkStat(dstFile);
if (dstStOpt) {
auto & dstSt = *dstStOpt;
if (S_ISLNK(dstSt.st_mode)) {
if (fs::is_symlink(dstSt)) {
auto prevPriority = state.priorities[dstFile];
if (prevPriority == priority)
throw BuildEnvFileConflictError(
Expand All @@ -108,7 +113,7 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
continue;
if (unlink(dstFile.c_str()) == -1)
throw SysError("unlinking '%1%'", dstFile);
} else if (S_ISDIR(dstSt.st_mode))
} else if (fs::is_directory(dstSt))
throw Error("collision between non-directory '%1%' and directory '%2%'", srcFile, dstFile);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/gc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -852,7 +852,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
if (name == "." || name == "..") continue;
Path path = linksDir + "/" + name;

auto st = lstat(path);
auto st = unix::lstat(path);

if (st.st_nlink != 1) {
actualSize += st.st_size;
Expand Down
6 changes: 3 additions & 3 deletions src/libstore/local-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,10 @@ LocalStore::LocalStore(const Params & params)
/* Ensure that the store and its parents are not symlinks. */
if (!settings.allowSymlinkedStore) {
Path path = realStoreDir;
struct stat st;
while (path != "/") {
st = lstat(path);
if (S_ISLNK(st.st_mode))
std::error_code _ec;
// ignore errors, this will return false when there's an error
if (std::filesystem::is_symlink(path, _ec))
throw Error(
"the path '%1%' is a symlink; "
"this is not allowed for the Nix store and its parent directories",
Expand Down
18 changes: 11 additions & 7 deletions src/libstore/optimise-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <cstdlib>
#include <cstring>
#include <filesystem>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
Expand All @@ -19,9 +20,14 @@ namespace nix {

static void makeWritable(const Path & path)
{
auto st = lstat(path);
if (chmod(path.c_str(), st.st_mode | S_IWUSR) == -1)
auto st = std::filesystem::symlink_status(path);
auto writePerms = st.permissions() | std::filesystem::perms::owner_write;
try {
std::filesystem::permissions(path, writePerms);
}
catch (std::filesystem::filesystem_error &e) {
throw SysError("changing writability of '%1%'", path);
}
}


Expand Down Expand Up @@ -94,7 +100,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,
{
checkInterrupt();

auto st = lstat(path);
auto st = unix::lstat(path);

#if __APPLE__
/* HFS/macOS has some undocumented security feature disabling hardlinking for
Expand Down Expand Up @@ -159,7 +165,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,

/* Maybe delete the link, if it has been corrupted. */
if (std::filesystem::exists(std::filesystem::symlink_status(linkPath))) {
auto stLink = lstat(linkPath.string());
auto stLink = unix::lstat(linkPath.string());
if (st.st_size != stLink.st_size
|| (repair && hash != ({
hashPath(
Expand Down Expand Up @@ -201,9 +207,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats,

/* Yes! We've seen a file with the same contents. Replace the
current file with a hard link to that file. */
auto stLink = lstat(linkPath.string());

if (st.st_ino == stLink.st_ino) {
if (std::filesystem::equivalent(path, linkPath)) {
debug("'%1%' is already linked to '%2%'", path, linkPath);
return;
}
Expand Down
6 changes: 3 additions & 3 deletions src/libstore/posix-fs-canonicalise.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct

void canonicaliseTimestampAndPermissions(const Path & path)
{
canonicaliseTimestampAndPermissions(path, lstat(path));
canonicaliseTimestampAndPermissions(path, unix::lstat(path));
}


Expand All @@ -76,7 +76,7 @@ static void canonicalisePathMetaData_(
}
#endif

auto st = lstat(path);
auto st = unix::lstat(path);

/* Really make sure that the path is of a supported type. */
if (!(S_ISREG(st.st_mode) || S_ISDIR(st.st_mode) || S_ISLNK(st.st_mode)))
Expand Down Expand Up @@ -174,7 +174,7 @@ void canonicalisePathMetaData(
#ifndef _WIN32
/* On platforms that don't have lchown(), the top-level path can't
be a symlink, since we can't change its ownership. */
auto st = lstat(path);
auto st = unix::lstat(path);

if (st.st_uid != geteuid()) {
assert(S_ISLNK(st.st_mode));
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/profiles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ std::pair<Generations, std::optional<GenerationNumber>> findGenerations(Path pro
gens.push_back({
.number = *n,
.path = path,
.creationTime = lstat(path).st_mtime
.creationTime = unix::lstat(path).st_mtime
});
}
}
Expand Down
Loading
Loading