diff --git a/src/libstore/unix/build/derivation-builder.cc b/src/libstore/unix/build/derivation-builder.cc index 4a0abe901..722700c7c 100644 --- a/src/libstore/unix/build/derivation-builder.cc +++ b/src/libstore/unix/build/derivation-builder.cc @@ -1270,13 +1270,10 @@ void DerivationBuilderImpl::chownToBuilder(int fd, const std::filesystem::path & void DerivationBuilderImpl::writeBuilderFile(const std::string & name, std::string_view contents) { - auto relPath = CanonPath(name); /* Path must be the same after normalisation. This is an additional sanity check in addition to existing parsing checks for non-structured attrs exportReferencesGraph. In practice we only expect a single path component without any `..`, `.` components. */ - if (relPath.rel() != name || std::ranges::distance(relPath) != 1) - throw Error("invalid file name for a builder file: '%s'", name); - + auto relPath = CanonPath::fromFilename(name); AutoCloseFD fd = openFileEnsureBeneathNoSymlinks( tmpDirFd.get(), relPath, O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666); auto path = tmpDir / relPath.rel(); /* This is used only for error messages. */ diff --git a/src/libutil-tests/canon-path.cc b/src/libutil-tests/canon-path.cc index aae9285c4..ac40d6b0e 100644 --- a/src/libutil-tests/canon-path.cc +++ b/src/libutil-tests/canon-path.cc @@ -51,6 +51,19 @@ TEST(CanonPath, nullBytes) ASSERT_THROW(CanonPath(s, CanonPath::root), BadCanonPath); } +TEST(CanonPath, fromFilename) +{ + ASSERT_THROW(CanonPath::fromFilename("."), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename(".."), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename(""), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename("/.."), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename("/."), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename("/abc"), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename("abc/d"), BadCanonPath); + ASSERT_THROW(CanonPath::fromFilename("/abc/d"), BadCanonPath); + ASSERT_EQ(CanonPath::fromFilename("abc").rel(), "abc"); +} + TEST(CanonPath, from_existing) { CanonPath p0("foo//bar/"); diff --git a/src/libutil/canon-path.cc b/src/libutil/canon-path.cc index 22ca3e066..e62abdf31 100644 --- a/src/libutil/canon-path.cc +++ b/src/libutil/canon-path.cc @@ -23,6 +23,16 @@ static void ensureNoNullBytes(std::string_view s) } } +CanonPath CanonPath::fromFilename(std::string_view segment) +{ + auto res = CanonPath(segment); + /* Use existing canonicalisation logic for CanonPath to check that the segment + is already a valid filename. */ + if (segment != res.rel() || std::ranges::distance(res) != 1) + throw BadCanonPath("invalid filename '%s'", segment); + return res; +} + CanonPath::CanonPath(std::string_view raw) : path(absPathPure(concatStrings("/", raw))) { diff --git a/src/libutil/include/nix/util/canon-path.hh b/src/libutil/include/nix/util/canon-path.hh index 2156b02fc..56aa4d038 100644 --- a/src/libutil/include/nix/util/canon-path.hh +++ b/src/libutil/include/nix/util/canon-path.hh @@ -62,7 +62,7 @@ public: struct unchecked_t {}; - CanonPath(unchecked_t _, std::string path) + constexpr CanonPath(unchecked_t _, std::string path) : path(std::move(path)) { } @@ -74,6 +74,12 @@ public: static const CanonPath root; + /** + * Construct a CanonPath from a single path segment. The segment + * must not contain any slashes and must not be `.` or `..`. + */ + static CanonPath fromFilename(std::string_view segment); + /** * If `raw` starts with a slash, return * `CanonPath(raw)`. Otherwise return a `CanonPath` representing diff --git a/src/libutil/unix/file-system.cc b/src/libutil/unix/file-system.cc index 3598bc8de..ca224f730 100644 --- a/src/libutil/unix/file-system.cc +++ b/src/libutil/unix/file-system.cc @@ -146,11 +146,10 @@ static void _deletePath( } #endif - std::string name(path.filename()); - assert(name != "." && name != ".." && !name.empty()); + auto name = CanonPath::fromFilename(path.filename().native()); PosixStat st; - if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { + if (fstatat(parentfd, name.rel_c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; throw SysError("getting status of %1%", PathFmt(path)); @@ -185,7 +184,7 @@ static void _deletePath( const auto PERM_MASK = S_IRUSR | S_IWUSR | S_IXUSR; if ((st.st_mode & PERM_MASK) != PERM_MASK) try { - unix::fchmodatTryNoFollow(parentfd, CanonPath(name), st.st_mode | PERM_MASK); + unix::fchmodatTryNoFollow(parentfd, name, st.st_mode | PERM_MASK); } catch (SysError & e) { e.addTrace({}, "while making directory %1% accessible for deletion", PathFmt(path)); if (e.errNo == EOPNOTSUPP) @@ -193,7 +192,7 @@ static void _deletePath( throw; } - int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW); + int fd = openat(parentfd, name.rel_c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW); if (fd == -1) throw SysError("opening directory %1%", PathFmt(path)); AutoCloseDir dir(fdopendir(fd)); @@ -213,7 +212,7 @@ static void _deletePath( } int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0; - if (unlinkat(parentfd, name.c_str(), flags) == -1) { + if (unlinkat(parentfd, name.rel_c_str(), flags) == -1) { if (errno == ENOENT) return; try { @@ -230,13 +229,14 @@ static void _deletePath( static void _deletePath(const std::filesystem::path & path, uint64_t & bytesFreed MOUNTEDPATHS_PARAM) { assert(path.is_absolute()); - assert(path.parent_path() != path); + auto parentDirPath = path.parent_path(); + assert(parentDirPath != path); - AutoCloseFD dirfd = toDescriptor(open(path.parent_path().string().c_str(), O_RDONLY)); + AutoCloseFD dirfd = openDirectory(parentDirPath); if (!dirfd) { if (errno == ENOENT) return; - throw SysError("opening directory %s", PathFmt(path.parent_path())); + throw SysError("opening directory %s", PathFmt(parentDirPath)); } std::exception_ptr ex;