From 22ba4dc78d956020e06e0618f020e11700749823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Mon, 26 Aug 2024 21:14:20 +0200 Subject: [PATCH 1/5] builtins.readDir: fix nix error trace on filesystem errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: nix-env % ./src/nix/nix eval --impure --expr 'let f = builtins.readDir "/nix/store/hs3yxdq9knimwdm51gvbs4dvncz46f9d-hello-2.12.1/foo"; in f' --show-trace error: filesystem error: directory iterator cannot open directory: No such file or directory [/nix/store/hs3yxdq9knimwdm51gvbs4dvncz46f9d-hello-2.12.1/foo] After: error: … while calling the 'readDir' builtin at «string»:1:9: 1| let f = builtins.readDir "/nix/store/hs3yxdq9knimwdm51gvbs4dvncz46f9d-hello-2.12.1/foo"; in f | ^ error: reading directory '/nix/store/hs3yxdq9knimwdm51gvbs4dvncz46f9d-hello-2.12.1/foo': No such file or directory --- src/libutil/posix-source-accessor.cc | 42 +++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc index 8cec3388d7b..f26f74d5846 100644 --- a/src/libutil/posix-source-accessor.cc +++ b/src/libutil/posix-source-accessor.cc @@ -132,23 +132,24 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & { assertNoSymlinks(path); DirEntries res; - for (auto & entry : std::filesystem::directory_iterator{makeAbsPath(path)}) { - checkInterrupt(); - auto type = [&]() -> std::optional { - std::filesystem::file_type nativeType; - try { - nativeType = entry.symlink_status().type(); - } catch (std::filesystem::filesystem_error & e) { - // We cannot always stat the child. (Ideally there is no - // stat because the native directory entry has the type - // already, but this isn't always the case.) - if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted) - return std::nullopt; - else throw; - } - - // cannot exhaustively enumerate because implementation-specific - // additional file types are allowed. + try { + for (auto & entry : std::filesystem::directory_iterator{makeAbsPath(path)}) { + checkInterrupt(); + auto type = [&]() -> std::optional { + std::filesystem::file_type nativeType; + try { + nativeType = entry.symlink_status().type(); + } catch (std::filesystem::filesystem_error & e) { + // We cannot always stat the child. (Ideally there is no + // stat because the native directory entry has the type + // already, but this isn't always the case.) + if (e.code() == std::errc::permission_denied || e.code() == std::errc::operation_not_permitted) + return std::nullopt; + else throw; + } + + // cannot exhaustively enumerate because implementation-specific + // additional file types are allowed. #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wswitch-enum" switch (nativeType) { @@ -158,8 +159,11 @@ SourceAccessor::DirEntries PosixSourceAccessor::readDirectory(const CanonPath & default: return tMisc; } #pragma GCC diagnostic pop - }(); - res.emplace(entry.path().filename().string(), type); + }(); + res.emplace(entry.path().filename().string(), type); + } + } catch (std::filesystem::filesystem_error & e) { + throw SysError("reading directory %1%", showPath(path)); } return res; } From 05a1ffe23649950269999fd97fa351e78bd43dd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 5 Sep 2024 10:59:43 +0200 Subject: [PATCH 2/5] repl: wrap std::filesystem error into SysError /tmp/ecstatic-euler-mAFGV7 % /home/joerg/git/nix/build/subprojects/nix/nix repl Nix 2.25.0 Type :? for help. after doing rm /tmp/ecstatic-euler-mAFGV7 this will result in: nix-repl> :lf . error: cannot determine current working directory: No such file or directory Before it would make the repl crash /tmp/clever-hermann-MCm7A9 % /home/joerg/git/nix/build/subprojects/nix/nix repl Nix 2.25.0 Type :? for help. nix-repl> :lf . error: filesystem error: cannot get current path: No such file or directory --- src/libcmd/repl.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 63f6c1bdd0d..319dae3613b 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -2,6 +2,7 @@ #include #include +#include "error.hh" #include "repl-interacter.hh" #include "repl.hh" @@ -720,7 +721,14 @@ void NixRepl::loadFlake(const std::string & flakeRefS) if (flakeRefS.empty()) throw Error("cannot use ':load-flake' without a path specified. (Use '.' for the current working directory.)"); - auto flakeRef = parseFlakeRef(fetchSettings, flakeRefS, std::filesystem::current_path().string(), true); + std::filesystem::path cwd; + try { + cwd = std::filesystem::current_path(); + } catch (std::filesystem::filesystem_error & e) { + throw SysError("cannot determine current working directory"); + } + + auto flakeRef = parseFlakeRef(fetchSettings, flakeRefS, cwd.string(), true); if (evalSettings.pureEval && !flakeRef.input.isLocked()) throw Error("cannot use ':load-flake' on locked flake reference '%s' (use --impure to override)", flakeRefS); From 70c52d72f4ee93b68b57b12cd7892bba03446067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 5 Sep 2024 12:59:54 +0200 Subject: [PATCH 3/5] builtins.unpackChannel: wrap filesystem errors and sanitize channelName Otherwise these errors are not caught correctly --- src/libstore/builtins/unpack-channel.cc | 28 +++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc index a5f2b8e3adf..7f9a520eed3 100644 --- a/src/libstore/builtins/unpack-channel.cc +++ b/src/libstore/builtins/unpack-channel.cc @@ -13,21 +13,37 @@ void builtinUnpackChannel( return i->second; }; - auto out = outputs.at("out"); - auto channelName = getAttr("channelName"); + std::filesystem::path out(outputs.at("out")); + std::filesystem::path channelName(getAttr("channelName")); auto src = getAttr("src"); + if (channelName.filename() != channelName) { + throw Error("channelName is not allowed to contain filesystem seperators, got %1%", channelName); + } + createDirs(out); unpackTarfile(src, out); - auto entries = std::filesystem::directory_iterator{out}; - auto fileName = entries->path().string(); - auto fileCount = std::distance(std::filesystem::begin(entries), std::filesystem::end(entries)); + size_t fileCount; + std::string fileName; + try { + auto entries = std::filesystem::directory_iterator{out}; + fileName = entries->path().string(); + fileCount = std::distance(std::filesystem::begin(entries), std::filesystem::end(entries)); + } catch (std::filesystem::filesystem_error &e) { + throw SysError("failed to read directory %1%", out); + } + if (fileCount != 1) throw Error("channel tarball '%s' contains more than one file", src); - std::filesystem::rename(fileName, (out + "/" + channelName)); + std::filesystem::path target(out / channelName); + try { + std::filesystem::rename(fileName, target); + } catch (std::filesystem::filesystem_error &e) { + throw SysError("failed to rename %1% to %2%", fileName, target); + } } } From 04ce0e648aeac282b114cf426cea8a078c97e0a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Thu, 5 Sep 2024 14:08:20 +0200 Subject: [PATCH 4/5] add release notes for filesystem fixes Update doc/manual/rl-next/filesystem-errors.md Co-authored-by: John Ericson --- doc/manual/rl-next/filesystem-errors.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 doc/manual/rl-next/filesystem-errors.md diff --git a/doc/manual/rl-next/filesystem-errors.md b/doc/manual/rl-next/filesystem-errors.md new file mode 100644 index 00000000000..2d5b2622860 --- /dev/null +++ b/doc/manual/rl-next/filesystem-errors.md @@ -0,0 +1,14 @@ +--- +synopsis: wrap filesystem exceptions more correctly +issues: [] +prs: [11378] +--- + + +With the switch to `std::filesystem` in different places, Nix started to throw `std::filesystem::filesystem_error` in many places instead of its own exceptions. + +This lead to no longer generating error traces, for example when listing a non-existing directory, and can also lead to crashes inside the Nix REPL. + +This version catches these types of exception correctly and wrap them into Nix's own exeception type. + +Author: [**@Mic92**](https://github.com/Mic92) From 193dc490971b0435c7de7565b86110a59d515ff2 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Wed, 11 Sep 2024 11:59:11 -0400 Subject: [PATCH 5/5] tweak unpack channel built-in, std::filesystem::path for tarball --- src/libstore/builtins/unpack-channel.cc | 36 ++++++++++++++----------- src/libutil/tarfile.cc | 22 ++++++++------- src/libutil/tarfile.hh | 6 ++--- 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/libstore/builtins/unpack-channel.cc b/src/libstore/builtins/unpack-channel.cc index 7f9a520eed3..d30626a309b 100644 --- a/src/libstore/builtins/unpack-channel.cc +++ b/src/libstore/builtins/unpack-channel.cc @@ -3,46 +3,52 @@ namespace nix { +namespace fs { using namespace std::filesystem; } + void builtinUnpackChannel( const BasicDerivation & drv, const std::map & outputs) { - auto getAttr = [&](const std::string & name) { + auto getAttr = [&](const std::string & name) -> const std::string & { auto i = drv.env.find(name); if (i == drv.env.end()) throw Error("attribute '%s' missing", name); return i->second; }; - std::filesystem::path out(outputs.at("out")); - std::filesystem::path channelName(getAttr("channelName")); - auto src = getAttr("src"); + fs::path out{outputs.at("out")}; + auto & channelName = getAttr("channelName"); + auto & src = getAttr("src"); - if (channelName.filename() != channelName) { + if (fs::path{channelName}.filename().string() != channelName) { throw Error("channelName is not allowed to contain filesystem seperators, got %1%", channelName); } - createDirs(out); + try { + fs::create_directories(out); + } catch (fs::filesystem_error &) { + throw SysError("creating directory '%1%'", out.string()); + } unpackTarfile(src, out); size_t fileCount; std::string fileName; try { - auto entries = std::filesystem::directory_iterator{out}; + auto entries = fs::directory_iterator{out}; fileName = entries->path().string(); - fileCount = std::distance(std::filesystem::begin(entries), std::filesystem::end(entries)); - } catch (std::filesystem::filesystem_error &e) { - throw SysError("failed to read directory %1%", out); + fileCount = std::distance(fs::begin(entries), fs::end(entries)); + } catch (fs::filesystem_error &) { + throw SysError("failed to read directory %1%", out.string()); } - if (fileCount != 1) throw Error("channel tarball '%s' contains more than one file", src); - std::filesystem::path target(out / channelName); + + auto target = out / channelName; try { - std::filesystem::rename(fileName, target); - } catch (std::filesystem::filesystem_error &e) { - throw SysError("failed to rename %1% to %2%", fileName, target); + fs::rename(fileName, target); + } catch (fs::filesystem_error &) { + throw SysError("failed to rename %1% to %2%", fileName, target.string()); } } diff --git a/src/libutil/tarfile.cc b/src/libutil/tarfile.cc index 2e323629512..a8a22d283f8 100644 --- a/src/libutil/tarfile.cc +++ b/src/libutil/tarfile.cc @@ -8,6 +8,10 @@ namespace nix { +namespace fs { +using namespace std::filesystem; +} + namespace { int callback_open(struct archive *, void * self) @@ -102,14 +106,14 @@ TarArchive::TarArchive(Source & source, bool raw, std::optional com "Failed to open archive (%s)"); } -TarArchive::TarArchive(const Path & path) +TarArchive::TarArchive(const fs::path & path) : archive{archive_read_new()} , buffer(defaultBufferSize) { archive_read_support_filter_all(archive); enableSupportedFormats(archive); archive_read_set_option(archive, NULL, "mac-ext", NULL); - check(archive_read_open_filename(archive, path.c_str(), 16384), "failed to open archive: %s"); + check(archive_read_open_filename(archive, path.string().c_str(), 16384), "failed to open archive: %s"); } void TarArchive::close() @@ -123,7 +127,7 @@ TarArchive::~TarArchive() archive_read_free(this->archive); } -static void extract_archive(TarArchive & archive, const Path & destDir) +static void extract_archive(TarArchive & archive, const fs::path & destDir) { int flags = ARCHIVE_EXTRACT_TIME | ARCHIVE_EXTRACT_SECURE_SYMLINKS | ARCHIVE_EXTRACT_SECURE_NODOTDOT; @@ -140,7 +144,7 @@ static void extract_archive(TarArchive & archive, const Path & destDir) else archive.check(r); - archive_entry_copy_pathname(entry, (destDir + "/" + name).c_str()); + archive_entry_copy_pathname(entry, (destDir / name).string().c_str()); // sources can and do contain dirs with no rx bits if (archive_entry_filetype(entry) == AE_IFDIR && (archive_entry_mode(entry) & 0500) != 0500) @@ -149,7 +153,7 @@ static void extract_archive(TarArchive & archive, const Path & destDir) // Patch hardlink path const char * original_hardlink = archive_entry_hardlink(entry); if (original_hardlink) { - archive_entry_copy_hardlink(entry, (destDir + "/" + original_hardlink).c_str()); + archive_entry_copy_hardlink(entry, (destDir / original_hardlink).string().c_str()); } archive.check(archive_read_extract(archive.archive, entry, flags)); @@ -158,19 +162,19 @@ static void extract_archive(TarArchive & archive, const Path & destDir) archive.close(); } -void unpackTarfile(Source & source, const Path & destDir) +void unpackTarfile(Source & source, const fs::path & destDir) { auto archive = TarArchive(source); - createDirs(destDir); + fs::create_directories(destDir); extract_archive(archive, destDir); } -void unpackTarfile(const Path & tarFile, const Path & destDir) +void unpackTarfile(const fs::path & tarFile, const fs::path & destDir) { auto archive = TarArchive(tarFile); - createDirs(destDir); + fs::create_directories(destDir); extract_archive(archive, destDir); } diff --git a/src/libutil/tarfile.hh b/src/libutil/tarfile.hh index 0517177dbe6..5e29c6bbac3 100644 --- a/src/libutil/tarfile.hh +++ b/src/libutil/tarfile.hh @@ -15,7 +15,7 @@ struct TarArchive void check(int err, const std::string & reason = "failed to extract archive (%s)"); - explicit TarArchive(const Path & path); + explicit TarArchive(const std::filesystem::path & path); /// @brief Create a generic archive from source. /// @param source - Input byte stream. @@ -37,9 +37,9 @@ struct TarArchive int getArchiveFilterCodeByName(const std::string & method); -void unpackTarfile(Source & source, const Path & destDir); +void unpackTarfile(Source & source, const std::filesystem::path & destDir); -void unpackTarfile(const Path & tarFile, const Path & destDir); +void unpackTarfile(const std::filesystem::path & tarFile, const std::filesystem::path & destDir); time_t unpackTarfileToSink(TarArchive & archive, ExtendedFileSystemObjectSink & parseSink);