From b469c6509ba616da6df8a27e4ccb205a877c66c9 Mon Sep 17 00:00:00 2001 From: K900 Date: Fri, 12 Apr 2024 12:29:10 +0300 Subject: [PATCH] libstore/build: just copy the magic /etc files into the sandbox Saves us a bunch of thinking about how to handle symlinks, and prevents the DNS config from changing on the fly under the build, which may or may not be a good thing? Change-Id: I071e6ae7e220884690b788d94f480866f428db71 --- src/libstore/build/local-derivation-goal.cc | 25 +++++++++++++++--- src/libutil/filesystem.cc | 22 ++++++++++------ src/libutil/util.hh | 15 ++++++++++- tests/nixos/default.nix | 2 ++ tests/nixos/symlink-resolvconf.nix | 28 +++++++++++++++++++++ 5 files changed, 79 insertions(+), 13 deletions(-) create mode 100644 tests/nixos/symlink-resolvconf.nix diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 588fe33ba..193fc598e 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -410,7 +410,7 @@ static void doBind(const Path & source, const Path & target, bool optional = fal } else if (S_ISLNK(st.st_mode)) { // Symlinks can (apparently) not be bind-mounted, so just copy it createDirs(dirOf(target)); - copyFile(source, target, /* andDelete */ false); + copyFile(source, target, {}); } else { createDirs(dirOf(target)); writeFile(target, ""); @@ -1811,8 +1811,25 @@ void LocalDerivationGoal::runChild() happens when testing Nix building fixed-output derivations within a pure derivation. */ for (auto & path : { "/etc/resolv.conf", "/etc/services", "/etc/hosts" }) - if (pathExists(path)) - ss.push_back(path); + if (pathExists(path)) { + // Copy the actual file, not the symlink, because we don't know where + // the symlink is pointing, and we don't want to chase down the entire + // chain. + // + // This means if your network config changes during a FOD build, + // the DNS in the sandbox will be wrong. However, this is pretty unlikely + // to actually be a problem, because FODs are generally pretty fast, + // and machines with often-changing network configurations probably + // want to run resolved or some other local resolver anyway. + // + // There's also just no simple way to do this correctly, you have to manually + // inotify watch the files for changes on the outside and update the sandbox + // while the build is running (or at least that's what Flatpak does). + // + // I also just generally feel icky about modifying sandbox state under a build, + // even though it really shouldn't be a big deal. -K900 + copyFile(path, chrootRootDir + path, { .followSymlinks = true }); + } if (settings.caFile != "") pathsInChroot.try_emplace("/etc/ssl/certs/ca-certificates.crt", settings.caFile, true); @@ -2542,7 +2559,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() // that there's no stale file descriptor pointing to it Path tmpOutput = actualPath + ".tmp"; movePath(actualPath, tmpOutput); - copyFile(tmpOutput, actualPath, true); + copyFile(tmpOutput, actualPath, { .deleteAfter = true }); auto newInfo0 = newInfoFromCA(DerivationOutput::CAFloating { .method = dof.ca.method, diff --git a/src/libutil/filesystem.cc b/src/libutil/filesystem.cc index ea934ffaf..b637281d0 100644 --- a/src/libutil/filesystem.cc +++ b/src/libutil/filesystem.cc @@ -103,31 +103,37 @@ void setWriteTime(const fs::path & p, const struct stat & st) throw SysError("changing modification time of '%s'", p); } -void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) +void copy(const fs::directory_entry & from, const fs::path & to, CopyFileFlags flags) { // TODO: Rewrite the `is_*` to use `symlink_status()` auto statOfFrom = lstat(from.path().c_str()); auto fromStatus = from.symlink_status(); // Mark the directory as writable so that we can delete its children - if (andDelete && fs::is_directory(fromStatus)) { + if (flags.deleteAfter && fs::is_directory(fromStatus)) { fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); } if (fs::is_symlink(fromStatus) || fs::is_regular_file(fromStatus)) { - fs::copy(from.path(), to, fs::copy_options::copy_symlinks | fs::copy_options::overwrite_existing); + auto opts = fs::copy_options::overwrite_existing; + + if (!flags.followSymlinks) { + opts |= fs::copy_options::copy_symlinks; + } + + fs::copy(from.path(), to, opts); } else if (fs::is_directory(fromStatus)) { fs::create_directory(to); for (auto & entry : fs::directory_iterator(from.path())) { - copy(entry, to / entry.path().filename(), andDelete); + copy(entry, to / entry.path().filename(), flags); } } else { throw Error("file '%s' has an unsupported type", from.path()); } setWriteTime(to, statOfFrom); - if (andDelete) { + if (flags.deleteAfter) { if (!fs::is_symlink(fromStatus)) fs::permissions(from.path(), fs::perms::owner_write, fs::perm_options::add | fs::perm_options::nofollow); fs::remove(from.path()); @@ -135,9 +141,9 @@ void copy(const fs::directory_entry & from, const fs::path & to, bool andDelete) } -void copyFile(const Path & oldPath, const Path & newPath, bool andDelete) +void copyFile(const Path & oldPath, const Path & newPath, CopyFileFlags flags) { - return copy(fs::directory_entry(fs::path(oldPath)), fs::path(newPath), andDelete); + return copy(fs::directory_entry(fs::path(oldPath)), fs::path(newPath), flags); } void renameFile(const Path & oldName, const Path & newName) @@ -160,7 +166,7 @@ void moveFile(const Path & oldName, const Path & newName) if (e.code().value() == EXDEV) { fs::remove(newPath); warn("Can’t rename %s as %s, copying instead", oldName, newName); - copy(fs::directory_entry(oldPath), tempCopyTarget, true); + copy(fs::directory_entry(oldPath), tempCopyTarget, { .deleteAfter = true }); renameFile(tempCopyTarget, newPath); } } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 61df6c4f8..9c2385e84 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -279,13 +279,26 @@ void renameFile(const Path & src, const Path & dst); */ void moveFile(const Path & src, const Path & dst); +struct CopyFileFlags +{ + /** + * Delete the file after copying. + */ + bool deleteAfter = false; + + /** + * Follow symlinks and copy the eventual target. + */ + bool followSymlinks = false; +}; + /** * Recursively copy the content of `oldPath` to `newPath`. If `andDelete` is * `true`, then also remove `oldPath` (making this equivalent to `moveFile`, but * with the guaranty that the destination will be “fresh”, with no stale inode * or file descriptor pointing to it). */ -void copyFile(const Path & oldPath, const Path & newPath, bool andDelete); +void copyFile(const Path & oldPath, const Path & newPath, CopyFileFlags flags); /** * Wrappers arount read()/write() that read/write exactly the diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 547290188..14fc0da81 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -158,4 +158,6 @@ in ca-fd-leak = runNixOSTestFor "x86_64-linux" ./ca-fd-leak; fetch-git = runNixOSTestFor "x86_64-linux" ./fetch-git; + + symlinkResolvconf = runNixOSTestFor "x86_64-linux" ./symlink-resolvconf.nix; } diff --git a/tests/nixos/symlink-resolvconf.nix b/tests/nixos/symlink-resolvconf.nix new file mode 100644 index 000000000..47df27524 --- /dev/null +++ b/tests/nixos/symlink-resolvconf.nix @@ -0,0 +1,28 @@ +{ pkgs, ... }: +let + checkResolvconfInSandbox = pkgs.runCommand "resolvconf-works-in-sandbox" { + # must be an FOD to have a resolv.conf in the first place + outputHash = "sha256-47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU="; + outputHashAlgo = "sha256"; + outputHashType = "flat"; + } '' + cat /etc/resolv.conf + touch $out + ''; +in { + name = "symlink-resolvconf"; + + nodes.machine = { + # Enabling resolved makes /etc/resolv.conf a symlink to /etc/static/resolv.conf, which is itself a symlink to /run. + # If this works, most other things probably will too. + services.resolved.enable = true; + + virtualisation.additionalPaths = [checkResolvconfInSandbox.drvPath]; + }; + + testScript = { nodes }: '' + start_all() + + machine.succeed('nix-build --check ${checkResolvconfInSandbox.drvPath}') + ''; +}