Skip to content

file-system: drop redundant quotes around PathFmt and assert relative paths #15329

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.

Sign up for GitHub

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

Merged
Ericson2314 merged 2 commits into from
Feb 25, 2026
Merged

file-system: drop redundant quotes around PathFmt and assert relative paths #15329

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4dfbca
Fix issues from #15313
amaanq Feb 24, 2026
d5dafb3
local-fs-store: make toRealPath accept StorePath only
amaanq Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libcmd/repl-interacter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ ReadlineLikeInteracter::Guard ReadlineLikeInteracter::init(detail::ReplCompleter
#if !USE_READLINE
el_hist_size = 1000;
#endif
read_history(historyFile.c_str());
read_history(historyFile.string().c_str());
auto oldRepl = curRepl;
curRepl = repl;
Guard restoreRepl([oldRepl] { curRepl = oldRepl; });
Expand Down Expand Up @@ -208,7 +208,7 @@ bool ReadlineLikeInteracter::getLine(std::string & input, ReplPromptType promptT

ReadlineLikeInteracter::~ReadlineLikeInteracter()
{
write_history(historyFile.c_str());
write_history(historyFile.string().c_str());
}

}; // namespace nix
2 changes: 1 addition & 1 deletion src/libcmd/repl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
auto localStore = state->store.dynamic_pointer_cast<LocalFSStore>();
if (localStore && command == ":bl") {
std::string symlink = "repl-result-" + outputName;
localStore->addPermRoot(outputPath, absPath(symlink));
localStore->addPermRoot(outputPath, absPath(symlink).string());
logger->cout(" ./%s -> %s", symlink, state->store->printStorePath(outputPath));
} else {
logger->cout(" %s -> %s", outputName, state->store->printStorePath(outputPath));
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/builtins/buildenv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
createLinks(state, srcFile, dstFile, priority);
continue;
} else if (S_ISLNK(dstSt.st_mode)) {
auto target = canonPath(dstFile, true);
auto target = canonPath(dstFile, true).string();
if (!S_ISDIR(lstat(target).st_mode))
throw Error("collision between %1% and non-directory %2%", PathFmt(srcFile), PathFmt(target));
if (unlink(dstFile.c_str()) == -1)
Expand Down Expand Up @@ -104,7 +104,7 @@ static void createLinks(State & state, const Path & srcDir, const Path & dstDir,
if (S_ISLNK(dstSt.st_mode)) {
auto prevPriority = state.priorities[dstFile];
if (prevPriority == priority)
throw BuildEnvFileConflictError(readLink(dstFile), srcFile, priority);
throw BuildEnvFileConflictError(readLink(dstFile).string(), srcFile, priority);
if (prevPriority < priority)
continue;
if (unlink(dstFile.c_str()) == -1)
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/daemon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ static void performOp(
"you are not privileged to create perm roots\n\n"
"hint: you can just do this client-side without special privileges, and probably want to do that instead.");
auto storePath = WorkerProto::Serialise<StorePath>::read(*store, rconn);
Path gcRoot = absPath(readString(conn.from));
Path gcRoot = absPath(readString(conn.from)).string();
logger->startWork();
auto & localFSStore = require<LocalFSStore>(*store);
localFSStore.addPermRoot(storePath, gcRoot);
Expand All @@ -690,7 +690,7 @@ static void performOp(
}

case WorkerProto::Op::AddIndirectRoot: {
Path path = absPath(readString(conn.from));
Path path = absPath(readString(conn.from)).string();

logger->startWork();
auto & indirectRootStore = require<IndirectRootStore>(*store);
Expand Down
21 changes: 11 additions & 10 deletions src/libstore/gc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static std::string gcRootsDir = "gcroots";
void LocalStore::addIndirectRoot(const Path & path)
{
std::string hash = hashString(HashAlgorithm::SHA1, path).to_string(HashFormat::Nix32, false);
Path realRoot = canonPath((config->stateDir.get() / gcRootsDir / "auto" / hash).string());
Path realRoot = canonPath((config->stateDir.get() / gcRootsDir / "auto" / hash).string()).string();
makeSymlink(realRoot, path);
}

Expand All @@ -57,7 +57,7 @@ void LocalStore::createTempRootsFile()
if (pathExists(fnTempRoots))
/* It *must* be stale, since there can be no two
processes with the same pid. */
unlink(fnTempRoots.c_str());
unlink(fnTempRoots.string().c_str());

*fdTempRoots = openLockFile(fnTempRoots, true);

Expand Down Expand Up @@ -236,14 +236,14 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
}

else if (type == std::filesystem::file_type::symlink) {
Path target = readLink(path);
Path target = readLink(path).string();
if (isInStore(target))
foundRoot(path, target);

/* Handle indirect roots. */
else {
auto parentPath = std::filesystem::path(path).parent_path();
target = absPath(target, &parentPath);
target = absPath(target, &parentPath).string();
if (!pathExists(target)) {
if (isInDir(path, std::filesystem::path{config->stateDir.get()} / gcRootsDir / "auto")) {
printInfo("removing stale link from '%1%' to '%2%'", path, target);
Expand All @@ -252,7 +252,7 @@ void LocalStore::findRoots(const Path & path, std::filesystem::file_type type, R
} else {
if (!std::filesystem::is_symlink(target))
return;
Path target2 = readLink(target);
Path target2 = readLink(target).string();
if (isInStore(target2))
foundRoot(target, target2);
}
Expand Down Expand Up @@ -408,7 +408,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
throw UnimplementedError("External GC client not implemented yet");
#else
if (fcntl(fdServer.get(), F_SETFL, fcntl(fdServer.get(), F_GETFL) | O_NONBLOCK) == -1)
throw SysError("making socket '%s' non-blocking", PathFmt(socketPath));
throw SysError("making socket %s non-blocking", PathFmt(socketPath));

Pipe shutdownPipe;
shutdownPipe.create();
Expand Down Expand Up @@ -539,6 +539,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
/* Helper function that deletes a path from the store and throws
GCLimitReached if we've deleted enough garbage. */
auto deleteFromStore = [&](std::string_view baseName, bool isKnownPath) {
assert(!std::filesystem::path(baseName).is_absolute());
Path path = storeDir + "/" + std::string(baseName);
Comment thread
Ericson2314 marked this conversation as resolved.
auto realPath = config->realStoreDir.get() / std::string(baseName);

Expand All @@ -548,7 +549,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
if (baseName.find("tmp-", 0) == 0) {
AutoCloseFD tmpDirFd = openDirectory(realPath);
if (!tmpDirFd || !lockFile(tmpDirFd.get(), ltWrite, false)) {
debug("skipping locked tempdir '%s'", PathFmt(realPath));
debug("skipping locked tempdir %s", PathFmt(realPath));
return;
}
}
Expand Down Expand Up @@ -713,7 +714,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
printInfo("determining live/dead paths...");

try {
AutoCloseDir dir(opendir(config->realStoreDir.get().c_str()));
AutoCloseDir dir(opendir(config->realStoreDir.get().string().c_str()));
if (!dir)
throw SysError("opening directory %1%", PathFmt(config->realStoreDir.get()));

Expand Down Expand Up @@ -757,7 +758,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
if (options.action == GCOptions::gcDeleteDead || options.action == GCOptions::gcDeleteSpecific) {
printInfo("deleting unused links...");

AutoCloseDir dir(opendir(linksDir.c_str()));
AutoCloseDir dir(opendir(linksDir.string().c_str()));
if (!dir)
throw SysError("opening directory %1%", PathFmt(linksDir));

Expand All @@ -769,7 +770,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
std::string name = dirent->d_name;
if (name == "." || name == "..")
continue;
Path path = linksDir + "/" + name;
Path path = (linksDir / name).string();

auto st = lstat(path);

Expand Down
18 changes: 12 additions & 6 deletions src/libstore/include/nix/store/local-fs-store.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,24 @@ public:

Setting<std::filesystem::path> stateDir{
this,
rootDir.get() ? *rootDir.get() + "/nix/var/nix" : getDefaultStateDir(),
rootDir.get() ? *rootDir.get() / "nix/var/nix" : std::filesystem::path(getDefaultStateDir()),
"state",
"Directory where Nix stores state."};
"Directory where Nix stores state.",
};

Setting<std::filesystem::path> logDir{
this,
rootDir.get() ? *rootDir.get() + "/nix/var/log/nix" : getDefaultLogDir(),
rootDir.get() ? *rootDir.get() / "nix/var/log/nix" : std::filesystem::path(getDefaultLogDir()),
"log",
"directory where Nix stores log files."};
"directory where Nix stores log files.",
};

Setting<std::filesystem::path> realStoreDir{
this, rootDir.get() ? *rootDir.get() + "/nix/store" : storeDir, "real", "Physical path of the Nix store."};
this,
rootDir.get() ? *rootDir.get() / "nix/store" : std::filesystem::path(storeDir),
"real",
"Physical path of the Nix store.",
};
};

struct alignas(8) /* Work around ASAN failures on i686-linux. */
Expand Down Expand Up @@ -114,7 +120,7 @@ struct alignas(8) /* Work around ASAN failures on i686-linux. */
Path toRealPath(const Path & storePath)
{
assert(isInStore(storePath));
return getRealStoreDir() + "/" + std::string(storePath, storeDir.size() + 1);
return (getRealStoreDir() / std::string(storePath, storeDir.size() + 1)).string();

@xokdvium xokdvium Feb 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems quite janky. What if storePath has some trailing slashes after storeDir? This would discard the LHS.

@amaanq amaanq Feb 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think storePath is always canonical here, because it comes from printStorePath which produces storeDir + "/" + storePath.to_string(), so there shouldn't be double or trailing slashes. The StorePath itself was validated in parseStorePath, which calls canonPath

@xokdvium xokdvium Feb 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason we can't accept StorePath here? I guess it would be a rather big change though...

@amaanq amaanq Feb 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah we could though I guess it would be a much larger change

@amaanq amaanq Feb 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Or maybe not, because there's one call by derivation-builder.cc on line 1866 that passes in finalDestPath + ".check"..

@xokdvium xokdvium Feb 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah right, it's not exactly a valid store path I suppose, but maybe we can hack it in in a way that's not that janky

@amaanq amaanq Feb 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done, made it a separate commit

@Ericson2314 Ericson2314 Feb 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh this is very nice outcome!

}

std::optional<std::string> getBuildLogExact(const StorePath & path) override;
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/indirect-root-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ void IndirectRootStore::makeSymlink(const Path & link, const Path & target)

Path IndirectRootStore::addPermRoot(const StorePath & storePath, const Path & _gcRoot)
{
Path gcRoot(canonPath(_gcRoot));
Path gcRoot(canonPath(_gcRoot).string());

if (isInStore(gcRoot))
throw Error(
Expand Down
3 changes: 2 additions & 1 deletion src/libstore/local-fs-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ LocalFSStoreConfig::LocalFSStoreConfig(PathView rootDir, const Params & params)
*/
, rootDir{makeRootDirSetting(
*this,
!rootDir.empty() && params.count("root") == 0 ? std::optional<Path>{canonPath(rootDir)} : std::nullopt)}
!rootDir.empty() && params.count("root") == 0 ? std::optional<Path>{canonPath(rootDir).string()}
: std::nullopt)}
{
}

Expand Down
4 changes: 2 additions & 2 deletions src/libstore/local-overlay-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ void LocalOverlayStore::collectGarbage(const GCOptions & options, GCResults & re

void LocalOverlayStore::deleteStorePath(const Path & path, uint64_t & bytesFreed, bool isKnownPath)
{
auto mergedDir = config->realStoreDir.get() + "/";
auto mergedDir = config->realStoreDir.get().string() + "/";
if (path.substr(0, mergedDir.length()) != mergedDir) {
warn("local-overlay: unexpected gc path '%s' ", path);
return;
Expand Down Expand Up @@ -261,7 +261,7 @@ LocalStore::VerificationResult LocalOverlayStore::verifyAllValidPaths(RepairFlag
StorePathSet done;

auto existsInStoreDir = [&](const StorePath & storePath) {
return pathExists(config->realStoreDir.get() + "/" + storePath.to_string());
return pathExists((config->realStoreDir.get() / storePath.to_string()).string());
};

bool errors = false;
Expand Down
12 changes: 6 additions & 6 deletions src/libstore/local-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ LocalStore::LocalStore(ref<const Config> config)

if (st.st_uid != 0 || st.st_gid != gr->gr_gid || (st.st_mode & ~S_IFMT) != perm) {
if (chown(config->realStoreDir.get().c_str(), 0, gr->gr_gid) == -1)
throw SysError("changing ownership of path '%s'", PathFmt(config->realStoreDir.get()));
throw SysError("changing ownership of path %s", PathFmt(config->realStoreDir.get()));
chmod(config->realStoreDir.get(), perm);
}
}
Expand Down Expand Up @@ -204,7 +204,7 @@ LocalStore::LocalStore(ref<const Config> config)
auto st = maybeStat(reservedPath);
if (!st || st->st_size != gcSettings.reservedSize) {
AutoCloseFD fd = toDescriptor(open(
reservedPath.c_str(),
reservedPath.string().c_str(),
O_WRONLY | O_CREAT
#ifndef _WIN32
| O_CLOEXEC
Expand Down Expand Up @@ -397,15 +397,15 @@ AutoCloseFD LocalStore::openGCLock()
{
auto fnGCLock = config->stateDir.get() / "gc.lock";
auto fdGCLock = open(
fnGCLock.c_str(),
fnGCLock.string().c_str(),
O_RDWR | O_CREAT
#ifndef _WIN32
| O_CLOEXEC
#endif
,
0600);
if (!fdGCLock)
throw SysError("opening global GC lock '%1%'", PathFmt(fnGCLock));
throw SysError("opening global GC lock %1%", PathFmt(fnGCLock));
return toDescriptor(fdGCLock);
}

Expand Down Expand Up @@ -450,7 +450,7 @@ LocalStore::~LocalStore()
auto fdTempRoots(_fdTempRoots.lock());
if (*fdTempRoots) {
fdTempRoots->close();
unlink(fnTempRoots.c_str());
unlink(fnTempRoots.string().c_str());
}
} catch (...) {
ignoreExceptionInDestructor();
Expand Down Expand Up @@ -502,7 +502,7 @@ void LocalStore::openDB(State & state, bool create)
throw Error("cannot create database while in read-only mode");
}

if (access(dbDir.c_str(), R_OK | (config->readOnly ? 0 : W_OK)))
if (access(dbDir.string().c_str(), R_OK | (config->readOnly ? 0 : W_OK)))
throw SysError("Nix database directory %1% is not writable", PathFmt(dbDir));

/* Open the Nix database. */
Expand Down
14 changes: 7 additions & 7 deletions src/libstore/optimise-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ LocalStore::InodeHash LocalStore::loadInodeHash()
debug("loading hash inodes in memory");
InodeHash inodeHash;

AutoCloseDir dir(opendir(linksDir.c_str()));
AutoCloseDir dir(opendir(linksDir.string().c_str()));
if (!dir)
throw SysError("opening directory %1%", PathFmt(linksDir));

Expand All @@ -72,9 +72,9 @@ Strings LocalStore::readDirectoryIgnoringInodes(const std::filesystem::path & pa
{
Strings names;

AutoCloseDir dir(opendir(path.c_str()));
AutoCloseDir dir(opendir(path.string().c_str()));
if (!dir)
throw SysError("opening directory '%s'", PathFmt(path));
throw SysError("opening directory %s", PathFmt(path));

struct dirent * dirent;
while (errno = 0, dirent = readdir(dir.get())) { /* sic */
Expand All @@ -91,7 +91,7 @@ Strings LocalStore::readDirectoryIgnoringInodes(const std::filesystem::path & pa
names.push_back(name);
}
if (errno)
throw SysError("reading directory '%s'", PathFmt(path));
throw SysError("reading directory %s", PathFmt(path));

return names;
}
Expand All @@ -111,7 +111,7 @@ void LocalStore::optimisePath_(
https://github.com/NixOS/nix/pull/2230 for more discussion. */

if (std::regex_search(path.string(), std::regex("\\.app/Contents/.+$"))) {
debug("'%s' is not allowed to be linked in macOS", PathFmt(path));
debug("%s is not allowed to be linked in macOS", PathFmt(path));
return;
}
#endif
Expand Down Expand Up @@ -142,7 +142,7 @@ void LocalStore::optimisePath_(

/* This can still happen on top-level files. */
if (st.st_nlink > 1 && inodeHash.count(st.st_ino)) {
debug("'%s' is already linked, with %d other file(s)", PathFmt(path), st.st_nlink - 2);
debug("%s is already linked, with %d other file(s)", PathFmt(path), st.st_nlink - 2);
return;
}

Expand All @@ -162,7 +162,7 @@ void LocalStore::optimisePath_(
HashAlgorithm::SHA256)
.hash;
});
debug("'%s' has hash '%s'", PathFmt(path), hash.to_string(HashFormat::Nix32, true));
debug("%s has hash '%s'", PathFmt(path), hash.to_string(HashFormat::Nix32, true));

/* Check if this is a known hash. */
std::filesystem::path linkPath = std::filesystem::path{linksDir} / hash.to_string(HashFormat::Nix32, false);
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/store-api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ std::filesystem::path Store::followLinksToStore(std::string_view _path) const
}

if (!isInStore(path.string()))
throw BadStorePath("path '%1%' is not in the Nix store", PathFmt(path));
throw BadStorePath("path %1% is not in the Nix store", PathFmt(path));
return path;
}

Expand Down
2 changes: 1 addition & 1 deletion src/libstore/store-dir-config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ StorePath StoreDirConfig::parseStorePath(std::string_view path) const
// Windows <-> Unix ssh-ing).
auto p =
#ifdef _WIN32
path
std::filesystem::path(path)
#else
canonPath(std::string(path))
#endif
Expand Down
2 changes: 1 addition & 1 deletion src/libutil/executable-path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void ExecutablePath::parseAppend(const OsString & path)

OsString ExecutablePath::render() const
{
std::vector<PathView> path2;
std::vector<OsStringView> path2;
path2.reserve(directories.size());
for (auto & p : directories)
path2.push_back(p.native());
Expand Down
Loading
Loading