diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 24b21f7da..a4c84c526 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -614,8 +614,7 @@ Path resolveExprPath(Path path) // Basic cycle/depth limit to avoid infinite loops. if (++followCount >= maxFollow) throw Error("too many symbolic links encountered while traversing the path '%s'", path); - if (lstat(path.c_str(), &st)) - throw SysError("getting status of '%s'", path); + st = lstat(path); if (!S_ISLNK(st.st_mode)) break; path = absPath(readLink(path), dirOf(path)); } diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 07c5bceb2..db7dbc17e 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1675,7 +1675,34 @@ void DerivationGoal::tryLocalBuild() { } -void replaceValidPath(const Path & storePath, const Path tmpPath) +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); + + bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); + + if (changePerm) + chmod_(src, st.st_mode | S_IWUSR); + + if (rename(src.c_str(), dst.c_str())) + throw SysError("renaming '%1%' to '%2%'", src, dst); + + if (changePerm) + chmod_(dst, st.st_mode); +} + + +void replaceValidPath(const Path & storePath, const Path & tmpPath) { /* We can't atomically replace storePath (the original) with tmpPath (the replacement), so we have to move it out of the @@ -1683,11 +1710,20 @@ void replaceValidPath(const Path & storePath, const Path tmpPath) we're repairing (say) Glibc, we end up with a broken system. */ Path oldPath = (format("%1%.old-%2%-%3%") % storePath % getpid() % random()).str(); if (pathExists(storePath)) - rename(storePath.c_str(), oldPath.c_str()); - if (rename(tmpPath.c_str(), storePath.c_str()) == -1) { - rename(oldPath.c_str(), storePath.c_str()); // attempt to recover - throw SysError("moving '%s' to '%s'", tmpPath, storePath); + movePath(storePath, oldPath); + + try { + movePath(tmpPath, storePath); + } catch (...) { + try { + // attempt to recover + movePath(oldPath, storePath); + } catch (...) { + ignoreException(); + } + throw; } + deletePath(oldPath); } @@ -2005,13 +2041,6 @@ HookReply DerivationGoal::tryBuildHook() } -static void chmod_(const Path & path, mode_t mode) -{ - if (chmod(path.c_str(), mode) == -1) - throw SysError("setting permissions on '%s'", path); -} - - int childEntry(void * arg) { ((DerivationGoal *) arg)->runChild(); @@ -2367,10 +2396,7 @@ void DerivationGoal::startBuilder() for (auto & i : inputPaths) { auto p = worker.store.printStorePath(i); Path r = worker.store.toRealPath(p); - struct stat st; - if (lstat(r.c_str(), &st)) - throw SysError("getting attributes of path '%s'", p); - if (S_ISDIR(st.st_mode)) + if (S_ISDIR(lstat(r).st_mode)) dirsInChroot.insert_or_assign(p, r); else linkOrCopy(r, chrootRootDir + p); @@ -3144,9 +3170,7 @@ void DerivationGoal::addDependency(const StorePath & path) if (pathExists(target)) throw Error("store path '%s' already exists in the sandbox", worker.store.printStorePath(path)); - struct stat st; - if (lstat(source.c_str(), &st)) - throw SysError("getting attributes of path '%s'", source); + auto st = lstat(source); if (S_ISDIR(st.st_mode)) { @@ -3735,29 +3759,6 @@ void DerivationGoal::runChild() } -static void moveCheckToStore(const Path & src, const Path & dst) -{ - /* For the rename of directory to succeed, we must be running as root or - the directory must be made temporarily writable (to update the - directory's parent link ".."). */ - struct stat st; - if (lstat(src.c_str(), &st) == -1) { - throw SysError("getting attributes of path '%1%'", src); - } - - bool changePerm = (geteuid() && S_ISDIR(st.st_mode) && !(st.st_mode & S_IWUSR)); - - if (changePerm) - chmod_(src, st.st_mode | S_IWUSR); - - if (rename(src.c_str(), dst.c_str())) - throw SysError("renaming '%1%' to '%2%'", src, dst); - - if (changePerm) - chmod_(dst, st.st_mode); -} - - void DerivationGoal::registerOutputs() { /* When using a build hook, the build hook can register the output @@ -3858,7 +3859,7 @@ void DerivationGoal::registerOutputs() something like that. */ canonicalisePathMetaData(actualPath, buildUser ? buildUser->getUID() : -1, inodesSeen); - debug("scanning for references for output %1 in temp location '%1%'", outputName, actualPath); + debug("scanning for references for output '%s' in temp location '%s'", outputName, actualPath); /* Pass blank Sink as we are not ready to hash data at this stage. */ NullSink blank; @@ -3913,7 +3914,6 @@ void DerivationGoal::registerOutputs() outputRewrites[std::string { scratchPath.hashPart() }] = std::string { finalStorePath.hashPart() }; }; - bool rewritten = false; std::optional referencesOpt = std::visit(overloaded { [&](AlreadyRegistered skippedFinalPath) -> std::optional { finish(skippedFinalPath.path); @@ -3944,7 +3944,9 @@ void DerivationGoal::registerOutputs() StringSource source(*sink.s); restorePath(actualPath, source); - rewritten = true; + /* FIXME: set proper permissions in restorePath() so + we don't have to do another traversal. */ + canonicalisePathMetaData(actualPath, -1, inodesSeen); } }; @@ -4027,7 +4029,7 @@ void DerivationGoal::registerOutputs() [&](DerivationOutputInputAddressed output) { /* input-addressed case */ auto requiredFinalPath = output.path; - /* Preemtively add rewrite rule for final hash, as that is + /* Preemptively add rewrite rule for final hash, as that is what the NAR hash will use rather than normalized-self references */ if (scratchPath != requiredFinalPath) outputRewrites.insert_or_assign( @@ -4101,44 +4103,21 @@ void DerivationGoal::registerOutputs() else. No moving needed. */ assert(newInfo.ca); } else { - /* Temporarily add write perm so we can move, will be fixed - later. */ - { - struct stat st; - auto & mode = st.st_mode; - if (lstat(actualPath.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", actualPath); - mode |= 0200; - /* Try to change the perms, but only if the file isn't a - symlink as symlinks permissions are mostly ignored and - calling `chmod` on it will just forward the call to the - target of the link. */ - if (!S_ISLNK(st.st_mode)) - if (chmod(actualPath.c_str(), mode) == -1) - throw SysError("changing mode of '%1%' to %2$o", actualPath, mode); - } - if (rename( - actualPath.c_str(), - worker.store.toRealPath(finalDestPath).c_str()) == -1) - throw SysError("moving build output '%1%' from it's temporary location to the Nix store", finalDestPath); - actualPath = worker.store.toRealPath(finalDestPath); + auto destPath = worker.store.toRealPath(finalDestPath); + movePath(actualPath, destPath); + actualPath = destPath; } } - /* Get rid of all weird permissions. This also checks that - all files are owned by the build user, if applicable. */ - canonicalisePathMetaData(actualPath, - buildUser && !rewritten ? buildUser->getUID() : -1, inodesSeen); - if (buildMode == bmCheck) { if (!worker.store.isValidPath(newInfo.path)) continue; ValidPathInfo oldInfo(*worker.store.queryPathInfo(newInfo.path)); if (newInfo.narHash != oldInfo.narHash) { worker.checkMismatch = true; if (settings.runDiffHook || settings.keepFailed) { - Path dst = worker.store.toRealPath(finalDestPath + checkSuffix); + auto dst = worker.store.toRealPath(finalDestPath + checkSuffix); deletePath(dst); - moveCheckToStore(actualPath, dst); + movePath(actualPath, dst); handleDiffHook( buildUser ? buildUser->getUID() : getuid(), diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 08b53c702..518a357ef 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -663,9 +663,7 @@ void LocalStore::removeUnusedLinks(const GCState & state) if (name == "." || name == "..") continue; Path path = linksDir + "/" + name; - struct stat st; - if (lstat(path.c_str(), &st) == -1) - throw SysError("statting '%1%'", path); + auto st = lstat(path); if (st.st_nlink != 1) { actualSize += st.st_size; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index c91f3fbf7..ee997ef3a 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -114,8 +114,7 @@ LocalStore::LocalStore(const Params & params) Path path = realStoreDir; struct stat st; while (path != "/") { - if (lstat(path.c_str(), &st)) - throw SysError("getting status of '%1%'", path); + st = lstat(path); if (S_ISLNK(st.st_mode)) throw Error( "the path '%1%' is a symlink; " @@ -419,10 +418,7 @@ static void canonicaliseTimestampAndPermissions(const Path & path, const struct void canonicaliseTimestampAndPermissions(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); - canonicaliseTimestampAndPermissions(path, st); + canonicaliseTimestampAndPermissions(path, lstat(path)); } @@ -440,9 +436,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe } #endif - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = 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))) @@ -478,8 +472,7 @@ static void canonicalisePathMetaData_(const Path & path, uid_t fromUid, InodesSe ensure that we don't fail on hard links within the same build (i.e. "touch $out/foo; ln $out/foo $out/bar"). */ if (fromUid != (uid_t) -1 && st.st_uid != fromUid) { - assert(!S_ISDIR(st.st_mode)); - if (inodesSeen.find(Inode(st.st_dev, st.st_ino)) == inodesSeen.end()) + if (S_ISDIR(st.st_mode) || !inodesSeen.count(Inode(st.st_dev, st.st_ino))) throw BuildError("invalid ownership on file '%1%'", path); mode_t mode = st.st_mode & ~S_IFMT; assert(S_ISLNK(st.st_mode) || (st.st_uid == geteuid() && (mode == 0444 || mode == 0555) && st.st_mtime == mtimeStore)); @@ -522,9 +515,7 @@ void canonicalisePathMetaData(const Path & path, uid_t fromUid, InodesSeen & ino /* On platforms that don't have lchown(), the top-level path can't be a symlink, since we can't change its ownership. */ - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); if (st.st_uid != geteuid()) { assert(S_ISLNK(st.st_mode)); @@ -1455,7 +1446,7 @@ static void makeMutable(const Path & path) { checkInterrupt(); - struct stat st = lstat(path); + auto st = lstat(path); if (!S_ISDIR(st.st_mode) && !S_ISREG(st.st_mode)) return; diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index e4b4b6213..c032a5e22 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -17,9 +17,7 @@ namespace nix { static void makeWritable(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); if (chmod(path.c_str(), st.st_mode | S_IWUSR) == -1) throw SysError("changing writability of '%1%'", path); } @@ -94,9 +92,7 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, { checkInterrupt(); - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); #if __APPLE__ /* HFS/macOS has some undocumented security feature disabling hardlinking for @@ -187,9 +183,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. */ - struct stat stLink; - if (lstat(linkPath.c_str(), &stLink)) - throw SysError("getting attributes of path '%1%'", linkPath); + auto stLink = lstat(linkPath); if (st.st_ino == stLink.st_ino) { debug(format("'%1%' is already linked to '%2%'") % path % linkPath); diff --git a/src/libstore/profiles.cc b/src/libstore/profiles.cc index c20386e2b..c3809bad7 100644 --- a/src/libstore/profiles.cc +++ b/src/libstore/profiles.cc @@ -39,13 +39,10 @@ std::pair> findGenerations(Path pro for (auto & i : readDirectory(profileDir)) { if (auto n = parseName(profileName, i.name)) { auto path = profileDir + "/" + i.name; - struct stat st; - if (lstat(path.c_str(), &st) != 0) - throw SysError("statting '%1%'", path); gens.push_back({ .number = *n, .path = path, - .creationTime = st.st_mtime + .creationTime = lstat(path).st_mtime }); } } diff --git a/src/libutil/archive.cc b/src/libutil/archive.cc index cdf3d89ae..6ad9fa238 100644 --- a/src/libutil/archive.cc +++ b/src/libutil/archive.cc @@ -68,9 +68,7 @@ static void dump(const Path & path, Sink & sink, PathFilter & filter) { checkInterrupt(); - struct stat st; - if (lstat(path.c_str(), &st)) - throw SysError("getting attributes of path '%1%'", path); + auto st = lstat(path); sink << "("; diff --git a/src/resolve-system-dependencies/resolve-system-dependencies.cc b/src/resolve-system-dependencies/resolve-system-dependencies.cc index 434ad80a6..d30227e4e 100644 --- a/src/resolve-system-dependencies/resolve-system-dependencies.cc +++ b/src/resolve-system-dependencies/resolve-system-dependencies.cc @@ -111,11 +111,7 @@ std::set runResolver(const Path & filename) bool isSymlink(const Path & path) { - struct stat st; - if (lstat(path.c_str(), &st) == -1) - throw SysError("getting attributes of path '%1%'", path); - - return S_ISLNK(st.st_mode); + return S_ISLNK(lstat(path).st_mode); } Path resolveSymlink(const Path & path) diff --git a/tests/repair.sh b/tests/repair.sh index ec7ad5dca..ba019028d 100644 --- a/tests/repair.sh +++ b/tests/repair.sh @@ -13,14 +13,14 @@ hash=$(nix-hash $path2) chmod u+w $path2 touch $path2/bad -if nix-store --verify --check-contents -v; then - echo "nix-store --verify succeeded unexpectedly" >&2 - exit 1 -fi +(! nix-store --verify --check-contents -v) # The path can be repaired by rebuilding the derivation. nix-store --verify --check-contents --repair +(! [ -e $path2/bad ]) +(! [ -w $path2 ]) + nix-store --verify-path $path2 # Re-corrupt and delete the deriver. Now --verify --repair should @@ -30,10 +30,7 @@ touch $path2/bad nix-store --delete $(nix-store -qd $path2) -if nix-store --verify --check-contents --repair; then - echo "nix-store --verify --repair succeeded unexpectedly" >&2 - exit 1 -fi +(! nix-store --verify --check-contents --repair) nix-build dependencies.nix -o $TEST_ROOT/result --repair diff --git a/tests/simple.sh b/tests/simple.sh index 37631b648..15bd2bd16 100644 --- a/tests/simple.sh +++ b/tests/simple.sh @@ -10,13 +10,15 @@ outPath=$(nix-store -rvv "$drvPath") echo "output path is $outPath" +(! [ -w $outPath ]) + text=$(cat "$outPath"/hello) if test "$text" != "Hello World!"; then exit 1; fi # Directed delete: $outPath is not reachable from a root, so it should # be deleteable. nix-store --delete $outPath -if test -e $outPath/hello; then false; fi +(! [ -e $outPath/hello ]) outPath="$(NIX_REMOTE=local?store=/foo\&real=$TEST_ROOT/real-store nix-instantiate --readonly-mode hash-check.nix)" if test "$outPath" != "/foo/lfy1s6ca46rm5r6w4gg9hc0axiakjcnm-dependencies.drv"; then