Skip to content
36 changes: 32 additions & 4 deletions src/libstore/build.cc
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,9 @@ class DerivationGoal : public Goal
/* The temporary directory. */
Path tmpDir;

/* The temporary directory file descriptor */
AutoCloseFD tmpDirFd;

/* The path of the temporary directory in the sandbox. */
Path tmpDirInSandbox;

Expand Down Expand Up @@ -985,9 +988,15 @@ class DerivationGoal : public Goal
/* Write a JSON file containing the derivation attributes. */
void writeStructuredAttrs();

/* Make a file owned by the builder. */
/* Make a file owned by the builder addressed by its path.
*
* SAFETY: this function is prone to TOCTOU as it receives a path and not a descriptor.
* It's only safe to call in a child of a directory only visible to the owner. */
void chownToBuilder(const Path & path);

/* Make a file owned by the builder addressed by its file descriptor. */
void chownToBuilder(const AutoCloseFD & fd);

/* Run the builder's process. */
void runChild();

Expand Down Expand Up @@ -1980,7 +1989,13 @@ void DerivationGoal::startBuilder()
auto drvName = storePathToName(drvPath);
tmpDir = createTempDir("", "nix-build-" + drvName, false, false, 0700);

chownToBuilder(tmpDir);
/* The TOCTOU between the previous mkdir call and this open call is unavoidable due to
* POSIX semantics.*/
tmpDirFd = AutoCloseFD{open(tmpDir.c_str(), O_RDONLY | O_NOFOLLOW | O_DIRECTORY)};
if (!tmpDirFd)
throw SysError("failed to open the build temporary directory descriptor '%1%'", tmpDir);

chownToBuilder(tmpDirFd);

/* Substitute output placeholders with the actual output paths. */
for (auto & output : drv->outputs)
Expand Down Expand Up @@ -2496,8 +2511,15 @@ void DerivationGoal::initTmpDir() {
} else {
string fn = ".attr-" + std::to_string(fileNr++);
Path p = tmpDir + "/" + fn;
writeFile(p, rewriteStrings(i.second, inputRewrites));
chownToBuilder(p);

AutoCloseFD passAsFileFd{openat(tmpDirFd.get(), fn.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC | O_EXCL | O_NOFOLLOW, 0666)};
if (!passAsFileFd) {
throw SysError("opening `passAsFile` file in the sandbox '%1%'", p);
}

writeFile(passAsFileFd, rewriteStrings(i.second, inputRewrites));
chownToBuilder(passAsFileFd);

env[i.first + "Path"] = tmpDirInSandbox + "/" + fn;
}
}
Expand Down Expand Up @@ -2692,6 +2714,12 @@ void DerivationGoal::chownToBuilder(const Path & path)
throw SysError(format("cannot change ownership of '%1%'") % path);
}

void DerivationGoal::chownToBuilder(const AutoCloseFD & fd)
{
if (!buildUser) return;
if (fchown(fd.get(), buildUser->getUID(), buildUser->getGID()) == -1)
throw SysError("cannot change ownership of file '%1%'", fd.guessOrInventPath());
}

void setupSeccomp()
{
Expand Down
4 changes: 2 additions & 2 deletions src/libstore/local-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ LocalStore::LocalStore(const Params & params)
else if (curSchema == 0) { /* new store */
curSchema = nixSchemaVersion;
openDB(*state, true);
writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str());
writeFileAndSync(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666);
}

else if (curSchema < nixSchemaVersion) {
Expand Down Expand Up @@ -207,7 +207,7 @@ LocalStore::LocalStore(const Params & params)
txn.commit();
}

writeFile(schemaPath, (format("%1%") % nixSchemaVersion).str());
writeFileAndSync(schemaPath, (format("%1%") % nixSchemaVersion).str(), 0666);

lockFile(globalLock.get(), ltRead, true);
}
Expand Down
137 changes: 123 additions & 14 deletions src/libutil/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,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;
Expand All @@ -292,6 +289,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)
{
Expand Down Expand Up @@ -338,9 +343,44 @@ void writeFile(const Path & path, const string & s, mode_t mode)
AutoCloseFD fd = open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode);
if (!fd)
throw SysError(format("opening file '%1%'") % path);

writeFile(fd, s, mode);

/* Close explicitly to propagate the exceptions. */
fd.close();
}

void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode)
{
assert(fd);
writeFull(fd.get(), s);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't the fd closed in here, but in the wrapping function? Is the calling function entitled to keep using the fd? A previous specifcally started closing the fd here to “propagate exceptions”, so I'm confused why it's being reverted here…

}

void writeFileAndSync(const Path & path, const std::string& s, mode_t mode)
{
{
AutoCloseFD fd{open(path.c_str(), O_WRONLY | O_TRUNC | O_CREAT | O_CLOEXEC, mode)};
if (!fd)
throw SysError("opening file '%1%'", path);

writeFile(fd, s, mode);
fd.fsync();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't closeForWrite be used here?

/* Close explicitly to ensure that exceptions are propagated. */
fd.close();
}

syncParent(path);
}

static void closeForWrite(const Path & path, AutoCloseFD & fd, bool sync)
{
if (sync)
fd.fsync();
// Explicitly close to make sure exceptions are propagated.
fd.close();
if (sync)
syncParent(path);
}

void writeFile(const Path & path, Source & source, mode_t mode)
{
Expand All @@ -356,8 +396,17 @@ void writeFile(const Path & path, Source & source, mode_t mode)
writeFull(fd.get(), (unsigned char *) buf.data(), n);
} catch (EndOfFile &) { break; }
}

closeForWrite(path, fd, false);
}

void syncParent(const Path & path)
{
AutoCloseFD fd = open(dirOf(path).c_str(), O_RDONLY, 0);
if (!fd)
throw SysError("opening file '%1%'", path);
fd.fsync();
}

string readLine(int fd)
{
Expand Down Expand Up @@ -387,14 +436,16 @@ void writeLine(int fd, string s)
}


static void _deletePath(const Path & path, unsigned long long & bytesFreed)
static void _deletePath(int parentfd, const Path& name, unsigned long long & bytesFreed)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we continuing to use Path here?

{
/* This ensures that `name` is an immediate child of `parentfd`. */
assert(!name.empty() && name.find('/') == std::string::npos && "`name` is an immediate child to `parentfd`");
checkInterrupt();

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);
throw SysError("getting status of '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
}

if (!S_ISDIR(st.st_mode) && st.st_nlink == 1)
Expand All @@ -404,18 +455,41 @@ 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)
throw SysError(format("chmod '%1%'") % path);
if (fchmodat(parentfd, name.c_str(), st.st_mode | PERM_MASK, 0) == -1) {
throw SysError("chmod '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
}
}

for (auto & i : readDirectory(path))
_deletePath(path + "/" + i.name, bytesFreed);
int fd = openat(parentfd, name.c_str(), O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
if (fd = -1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (fd = -1)
if (fd == -1)

throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
AutoCloseDir dir(fdopendir(fd));
if (!dir)
throw SysError("opening directory '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
for (auto & i : readDirectory(dir.get(), name))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here we don't have a flag for interruptibility.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Apparently related to cgroups in Lix, unnecessary.)

_deletePath(dirfd(dir.get()), i.name, bytesFreed);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here…

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Apparently related to cgroups in Lix, unnecessary.)

}

int flags = S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0;
if (unlinkat(parentfd, name.c_str(), flags) == -1) {
if (errno == ENOENT) return;
throw SysError("cannot unlink '%1%' in directory '%2%'", name, guessOrInventPathFromFD(parentfd));
}
}

if (remove(path.c_str()) == -1) {
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) {
if (errno == ENOENT) return;
throw SysError(format("cannot unlink '%1%'") % path);
throw SysError(format("opening directory '%1%'") % path);
}

_deletePath(dirfd.get(), baseNameOf(path).data(), bytesFreed);
}


Expand Down Expand Up @@ -747,6 +821,28 @@ int AutoCloseFD::get() const
return fd;
}

std::string guessOrInventPathFromFD(int fd)
{
assert(fd >= 0);
/* On Linux, there's no F_GETPATH available.
* But we can read /proc/ */
#if __linux__
try {
return readLink(fmt("/proc/self/fd/%1%", fd).c_str());
} catch (...) {
}
#elif defined (HAVE_F_GETPATH) && HAVE_F_GETPATH

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CPP symbol is set by meson in Lix, so I think this is effectively dead code here unless we add it to the autoconf machinery.

std::string fdName(PATH_MAX, '\0');
if (fcntl(fd, F_GETPATH, fdName.data()) != -1) {
fdName.resize(strlen(fdName.c_str()));
return fdName;
}
#else
#error "No implementation for retrieving file descriptors path."
#endif

return fmt("<fd %i>", fd);
}

void AutoCloseFD::close()
{
Expand All @@ -758,6 +854,19 @@ void AutoCloseFD::close()
}
}

void AutoCloseFD::fsync()
{
if (fd != -1) {
int result;
#if __APPLE__
result = ::fcntl(fd, F_FULLFSYNC);
#else
result = ::fsync(fd);
#endif
if (result == -1)
throw SysError("fsync file descriptor %1%", fd);
}
}

AutoCloseFD::operator bool() const
{
Expand Down
25 changes: 25 additions & 0 deletions src/libutil/util.hh
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ void writeFile(const Path & path, const string & s, mode_t mode = 0666);

void writeFile(const Path & path, Source & source, mode_t mode = 0666);

class AutoCloseFD; // forward-declaration needed because this moved around in Lix
void writeFile(AutoCloseFD & fd, const std::string& s, mode_t mode = 0666);

/* Write a string to a file and flush the file and its parent directory to disk. */
void writeFileAndSync(const Path & path, const std::string& s, mode_t mode = 0666);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know the significance of writeFileUninterruptible? Is that something we also need?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently related to cgroups in Lix, unnecessary.


/* Flush a file's parent directory to disk */
void syncParent(const Path & path);

/* Read a line from a file descriptor. */
string readLine(int fd);

Expand Down Expand Up @@ -186,6 +195,13 @@ public:
operator Path() const { return path; }
};

/*
* Will attempt to guess *A* path associated that might lead to the same file as used by this
* file descriptor.
*
* The returned string should NEVER be used as a valid path.
*/
std::string guessOrInventPathFromFD(int fd);

class AutoCloseFD
{
Expand All @@ -202,6 +218,15 @@ public:
explicit operator bool() const;
int release();
void close();
void fsync();

/*
* Will attempt to guess *A* path associated that might lead to the same file as used by this
* file descriptor.
*
* The returned string should NEVER be used as a valid path.
*/
std::string guessOrInventPath() const { return guessOrInventPathFromFD(fd); }
};


Expand Down