From f59404e1a619934b9430b3a5bf352dbfffef2c31 Mon Sep 17 00:00:00 2001 From: Linus Heckemann Date: Sat, 25 Apr 2020 13:18:39 +0200 Subject: [PATCH 1/6] nix-env: refactor uninstallDerivations Reduces the number of store queries it performs. Also prints a warning if any of the selectors did not match any installed derivations. UX Caveats: - Will print a warning that nothing matched if a previous selector already removed the path - Will not do anything if no selectors were provided (no change from before). Fixes #3531 --- src/nix-env/nix-env.cc | 45 ++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 1a2bb42a3..6c9cd9548 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -718,28 +718,39 @@ static void uninstallDerivations(Globals & globals, Strings & selectors, while (true) { string lockToken = optimisticLockProfile(profile); - DrvInfos installedElems = queryInstalled(*globals.state, profile); - DrvInfos newElems; + DrvInfos workingElems = queryInstalled(*globals.state, profile); - for (auto & i : installedElems) { - DrvName drvName(i.queryName()); - bool found = false; - for (auto & j : selectors) - /* !!! the repeated calls to followLinksToStorePath() - are expensive, should pre-compute them. */ - if ((isPath(j) && globals.state->store->parseStorePath(i.queryOutPath()) == globals.state->store->followLinksToStorePath(j)) - || DrvName(j).matches(drvName)) - { - printInfo("uninstalling '%s'", i.queryName()); - found = true; - break; - } - if (!found) newElems.push_back(i); + for (auto & selector : selectors) { + DrvInfos::iterator split = workingElems.begin(); + if (isPath(selector)) { + StorePath selectorStorePath = globals.state->store->followLinksToStorePath(selector); + split = std::partition( + workingElems.begin(), workingElems.end(), + [&selectorStorePath, globals](auto &elem) { + return selectorStorePath != globals.state->store->parseStorePath(elem.queryOutPath()); + } + ); + } else { + DrvName selectorName(selector); + split = std::partition( + workingElems.begin(), workingElems.end(), + [&selectorName](auto &elem){ + DrvName elemName(elem.queryName()); + return !selectorName.matches(elemName); + } + ); + } + if (split == workingElems.end()) + warn("Selector '%s' matched no installed paths", selector); + for (auto removedElem = split; removedElem != workingElems.end(); removedElem++) { + printInfo("uninstalling '%s'", removedElem->queryName()); + } + workingElems.erase(split, workingElems.end()); } if (globals.dryRun) return; - if (createUserEnv(*globals.state, newElems, + if (createUserEnv(*globals.state, workingElems, profile, settings.envKeepDerivations, lockToken)) break; } } From a3bc695e7dd60a0f1860942ba0343572b85e05e4 Mon Sep 17 00:00:00 2001 From: Matthew Kenigsberg Date: Mon, 27 Apr 2020 11:12:54 -0600 Subject: [PATCH 2/6] Set GCROOT to store path to prevent garbage collection --- src/nix/shell.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/nix/shell.cc b/src/nix/shell.cc index 71e640667..bd07104cf 100644 --- a/src/nix/shell.cc +++ b/src/nix/shell.cc @@ -200,13 +200,15 @@ struct Common : InstallableCommand, MixProfile } } - BuildEnvironment getBuildEnvironment(ref store) + std::pair getBuildEnvironment(ref store) { auto shellOutPath = getShellOutPath(store); + auto strPath = store->printStorePath(shellOutPath); + updateProfile(shellOutPath); - return readEnvironment(store->printStorePath(shellOutPath)); + return {readEnvironment(strPath), strPath}; } }; @@ -253,7 +255,7 @@ struct CmdDevShell : Common, MixEnvironment void run(ref store) override { - auto buildEnvironment = getBuildEnvironment(store); + auto [buildEnvironment, gcroot] = getBuildEnvironment(store); auto [rcFileFd, rcFilePath] = createTempFile("nix-shell"); @@ -276,6 +278,7 @@ struct CmdDevShell : Common, MixEnvironment auto shell = getEnv("SHELL").value_or("bash"); setEnviron(); + setenv("GCROOT", gcroot.data(), 1); auto args = Strings{std::string(baseNameOf(shell)), "--rcfile", rcFilePath}; @@ -307,7 +310,7 @@ struct CmdPrintDevEnv : Common void run(ref store) override { - auto buildEnvironment = getBuildEnvironment(store); + auto buildEnvironment = getBuildEnvironment(store).first; stopProgressBar(); From 9e95b95a5da59252d140fb61cb1177645935dcd6 Mon Sep 17 00:00:00 2001 From: Matthew Kenigsberg Date: Mon, 27 Apr 2020 13:18:26 -0600 Subject: [PATCH 3/6] comment --- src/nix/shell.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nix/shell.cc b/src/nix/shell.cc index bd07104cf..fa8fe2e9b 100644 --- a/src/nix/shell.cc +++ b/src/nix/shell.cc @@ -278,6 +278,7 @@ struct CmdDevShell : Common, MixEnvironment auto shell = getEnv("SHELL").value_or("bash"); setEnviron(); + // prevent garbage collection until shell exits setenv("GCROOT", gcroot.data(), 1); auto args = Strings{std::string(baseNameOf(shell)), "--rcfile", rcFilePath}; From c05e20daa1abb3446e378331697938b78af2b3d7 Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Mon, 27 Apr 2020 14:15:15 +0000 Subject: [PATCH 4/6] Fix long paths permanently breaking GC Suppose I have a path /nix/store/[hash]-[name]/a/a/a/a/a/[...]/a, long enough that everything after "/nix/store/" is longer than 4096 (MAX_PATH) bytes. Nix will happily allow such a path to be inserted into the store, because it doesn't look at all the nested structure. It just cares about the /nix/store/[hash]-[name] part. But, when the path is deleted, we encounter a problem. Nix will move the path to /nix/store/trash, but then when it's trying to recursively delete the trash directory, it will at some point try to unlink /nix/store/trash/[hash]-[name]/a/a/a/a/a/[...]/a. This will fail, because the path is too long. After this has failed, any store deletion operation will never work again, because Nix needs to delete the trash directory before recreating it to move new things to it. (I assume this is because otherwise a path being deleted could already exist in the trash, and then moving it would fail.) This means that if I can trick somebody into just fetching a tarball containing a path of the right length, they won't be able to delete store paths or garbage collect ever again, until the offending path is manually removed from /nix/store/trash. (And even fixing this manually is quite difficult if you don't understand the issue, because the absolute path that Nix says it failed to remove is also too long for rm(1).) This patch fixes the issue by making Nix's recursive delete operation use unlinkat(2). This function takes a relative path and a directory file descriptor. We ensure that the relative path is always just the name of the directory entry, and therefore its length will never exceed 255 bytes. This means that it will never even come close to AX_PATH, and Nix will therefore be able to handle removing arbitrarily deep directory hierachies. Since the directory file descriptor is used for recursion after being used in readDirectory, I made a variant of readDirectory that takes an already open directory stream, to avoid the directory being opened multiple times. As we have seen from this issue, the less we have to interact with paths, the better, and so it's good to reuse file descriptors where possible. I left _deletePath as succeeding even if the parent directory doesn't exist, even though that feels wrong to me, because without that early return, the linux-sandbox test failed. Reported-by: Alyssa Ross Thanks-to: Puck Meerburg Tested-by: Puck Meerburg Reviewed-by: Puck Meerburg --- src/libutil/util.cc | 54 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 332c1c43a..615a7656c 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -268,16 +268,13 @@ bool isLink(const Path & path) } -DirEntries readDirectory(const Path & path) +DirEntries readDirectory(DIR *dir, const Path & path) { DirEntries entries; entries.reserve(64); - AutoCloseDir dir(opendir(path.c_str())); - if (!dir) throw SysError(format("opening directory '%1%'") % path); - struct dirent * dirent; - while (errno = 0, dirent = readdir(dir.get())) { /* sic */ + while (errno = 0, dirent = readdir(dir)) { /* sic */ checkInterrupt(); string name = dirent->d_name; if (name == "." || name == "..") continue; @@ -294,6 +291,14 @@ DirEntries readDirectory(const Path & path) return entries; } +DirEntries readDirectory(const Path & path) +{ + AutoCloseDir dir(opendir(path.c_str())); + if (!dir) throw SysError(format("opening directory '%1%'") % path); + + return readDirectory(dir.get(), path); +} + unsigned char getFileType(const Path & path) { @@ -389,12 +394,14 @@ void writeLine(int fd, string s) } -static void _deletePath(const Path & path, unsigned long long & bytesFreed) +static void _deletePath(int parentfd, const Path & path, unsigned long long & bytesFreed) { checkInterrupt(); + string name(baseNameOf(path)); + struct stat st; - if (lstat(path.c_str(), &st) == -1) { + if (fstatat(parentfd, name.c_str(), &st, AT_SYMLINK_NOFOLLOW) == -1) { if (errno == ENOENT) return; throw SysError(format("getting status of '%1%'") % path); } @@ -406,20 +413,45 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed) /* Make the directory accessible. */ const auto PERM_MASK = S_IRUSR | S_IWUSR | S_IXUSR; if ((st.st_mode & PERM_MASK) != PERM_MASK) { - if (chmod(path.c_str(), st.st_mode | PERM_MASK) == -1) + if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) throw SysError(format("chmod '%1%'") % path); } - for (auto & i : readDirectory(path)) - _deletePath(path + "/" + i.name, bytesFreed); + int fd = openat(parentfd, path.c_str(), O_RDONLY); + if (!fd) + throw SysError(format("opening directory '%1%'") % path); + AutoCloseDir dir(fdopendir(fd)); + if (!dir) + throw SysError(format("opening directory '%1%'") % path); + for (auto & i : readDirectory(dir.get(), path)) + _deletePath(dirfd(dir.get()), path + "/" + i.name, bytesFreed); } - if (remove(path.c_str()) == -1) { + int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0; + if (unlinkat(parentfd, name.c_str(), flags) == -1) { if (errno == ENOENT) return; throw SysError(format("cannot unlink '%1%'") % path); } } +static void _deletePath(const Path & path, unsigned long long & bytesFreed) +{ + Path dir = dirOf(path); + if (dir == "") + dir = "/"; + + AutoCloseFD dirfd(open(dir.c_str(), O_RDONLY)); + if (!dirfd) { + // This really shouldn't fail silently, but it's left this way + // for backwards compatibility. + if (errno == ENOENT) return; + + throw SysError(format("opening directory '%1%'") % path); + } + + _deletePath(dirfd.get(), path, bytesFreed); +} + void deletePath(const Path & path) { From 52a3ca823dd8aca8da32a198be2b1238cb321e6c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 28 Apr 2020 17:56:01 +0200 Subject: [PATCH 5/6] Tweak warning message --- src/nix-env/nix-env.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc index 6c9cd9548..d62febaff 100644 --- a/src/nix-env/nix-env.cc +++ b/src/nix-env/nix-env.cc @@ -741,7 +741,7 @@ static void uninstallDerivations(Globals & globals, Strings & selectors, ); } if (split == workingElems.end()) - warn("Selector '%s' matched no installed paths", selector); + warn("selector '%s' matched no installed derivations", selector); for (auto removedElem = split; removedElem != workingElems.end(); removedElem++) { printInfo("uninstalling '%s'", removedElem->queryName()); } From 6d40fe573c859725cac6eb2c884ecf84965c8853 Mon Sep 17 00:00:00 2001 From: Matthew Kenigsberg Date: Tue, 28 Apr 2020 11:18:54 -0600 Subject: [PATCH 6/6] rename to NIX_GCROOT --- src/nix/shell.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/shell.cc b/src/nix/shell.cc index fa8fe2e9b..fbfe24bbe 100644 --- a/src/nix/shell.cc +++ b/src/nix/shell.cc @@ -279,7 +279,7 @@ struct CmdDevShell : Common, MixEnvironment setEnviron(); // prevent garbage collection until shell exits - setenv("GCROOT", gcroot.data(), 1); + setenv("NIX_GCROOT", gcroot.data(), 1); auto args = Strings{std::string(baseNameOf(shell)), "--rcfile", rcFilePath};