lix-project/lix
20
58
Fork 31
Code Issues 359 Code Review (Gerrit) Projects 3 Releases Wiki Activity

Fix long paths permanently breaking GC

Browse source 
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 <hi@alyssa.is>
Thanks-to: Puck Meerburg <puck@puckipedia.com>
Tested-by: Puck Meerburg <puck@puckipedia.com>
Reviewed-by: Puck Meerburg <puck@puckipedia.com>
This commit is contained in:
Alyssa Ross 2020-04-27 14:15:15 +00:00
parent c9d0cf7e02
commit c05e20daa1
No known key found for this signature in database
GPG key ID: F9DBED4859B271C0
1 changed files with 43 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

54
src/libutil/util.cc
View file

@ -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)
{