Bug 1481978 - Change IPC CloseSuperfluousFds to prevent accidentally heap-allocating closures. r=glandium

Closures are nice but -- as pointed out in bug 1481978 comment #2 --
it's a footgun to take a std::function argument in a context where heap
allocation isn't safe.

Fortunately, non-capturing closures convert to C function pointers,
so a C-style interface with a void* context can still be relatively
ergonomic.
This commit is contained in:
Jed Davis 2018-08-15 19:08:40 -06:00
parent fcbb856357
commit 5e630f7ccf
4 changed files with 11 additions and 8 deletions

View File

@ -93,7 +93,7 @@ void SetAllFDsToCloseOnExec();
// for which the given function returns true. Only call this function
// in a child process where you know that there aren't any other
// threads.
void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve);
void CloseSuperfluousFds(void* aCtx, bool (*aShouldPreserve)(void*, int));
typedef std::vector<std::pair<int, int> > file_handle_mapping_vector;
typedef std::map<std::string, std::string> environment_map;

View File

@ -57,9 +57,9 @@ bool LaunchApp(const std::vector<std::string>& argv,
}
}
auto fdIsUsed = [&shuffle](int fd) { return shuffle.MapsTo(fd); };
// Constructing a std::function from a reference_wrapper won't allocate.
CloseSuperfluousFds(std::ref(fdIsUsed));
CloseSuperfluousFds(&shuffle, [](void* aCtx, int aFd) {
return static_cast<decltype(&shuffle)>(aCtx)->MapsTo(aFd);
});
for (size_t i = 0; i < argv.size(); i++)
argv_cstr[i] = const_cast<char*>(argv[i].c_str());

View File

@ -121,7 +121,8 @@ class ScopedDIRClose {
typedef mozilla::UniquePtr<DIR, ScopedDIRClose> ScopedDIR;
void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve) {
void CloseSuperfluousFds(void* aCtx, bool (*aShouldPreserve)(void*, int))
{
// DANGER: no calls to malloc (or locks, etc.) are allowed from now on:
// https://crbug.com/36678
// Also, beware of STL iterators: https://crbug.com/331459
@ -162,7 +163,7 @@ void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve) {
for (rlim_t i = 0; i < max_fds; ++i) {
const int fd = static_cast<int>(i);
if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO ||
should_preserve(fd)) {
aShouldPreserve(aCtx, fd)) {
continue;
}
@ -188,7 +189,7 @@ void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve) {
if (fd == dir_fd)
continue;
if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO ||
should_preserve(fd)) {
aShouldPreserve(aCtx, fd)) {
continue;
}

View File

@ -590,7 +590,9 @@ SandboxFork::StartChrootServer()
MOZ_DIAGNOSTIC_ASSERT(false);
}
base::CloseSuperfluousFds([this](int fd) { return fd == mChrootServer; });
base::CloseSuperfluousFds(this, [](void* aCtx, int aFd) {
return aFd == static_cast<decltype(this)>(aCtx)->mChrootServer;
});
char msg;
ssize_t msgLen = HANDLE_EINTR(read(mChrootServer, &msg, 1));