Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

std/process: Default to libc closefrom in spawnProcessPosix #9048

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

std/process: Default to libc closefrom in spawnProcessPosix #9048

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 102 additions & 47 deletions std/process.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,7 @@ version (Posix) private enum InternalError : ubyte
doubleFork,
malloc,
preExec,
closefds_dup2,
}

/*
Expand Down Expand Up @@ -1008,7 +1009,7 @@ private Pid spawnProcessPosix(scope const(char[])[] args,
if (config.flags & Config.Flags.detached)
close(pidPipe[0]);
close(forkPipe[0]);
immutable forkPipeOut = forkPipe[1];
auto forkPipeOut = forkPipe[1];
immutable pidPipeOut = pidPipe[1];

// Set the working directory.
Expand Down Expand Up @@ -1042,56 +1043,106 @@ private Pid spawnProcessPosix(scope const(char[])[] args,

if (!(config.flags & Config.Flags.inheritFDs))
{
// NOTE: malloc() and getrlimit() are not on the POSIX async
// signal safe functions list, but practically this should
// not be a problem. Java VM and CPython also use malloc()
// in its own implementation via opendir().
import core.stdc.stdlib : malloc;
import core.sys.posix.poll : pollfd, poll, POLLNVAL;
import core.sys.posix.sys.resource : rlimit, getrlimit, RLIMIT_NOFILE;

// Get the maximum number of file descriptors that could be open.
rlimit r;
if (getrlimit(RLIMIT_NOFILE, &r) != 0)
{
abortOnError(forkPipeOut, InternalError.getrlimit, .errno);
}
immutable maxDescriptors = cast(int) r.rlim_cur;

// The above, less stdin, stdout, and stderr
immutable maxToClose = maxDescriptors - 3;
version (FreeBSD)
import core.sys.freebsd.unistd : closefrom;
else version (OpenBSD)
import core.sys.openbsd.unistd : closefrom;

// Call poll() to see which ones are actually open:
auto pfds = cast(pollfd*) malloc(pollfd.sizeof * maxToClose);
if (pfds is null)
{
abortOnError(forkPipeOut, InternalError.malloc, .errno);
}
foreach (i; 0 .. maxToClose)
static if (!__traits(compiles, closefrom))
{
pfds[i].fd = i + 3;
pfds[i].events = 0;
pfds[i].revents = 0;
}
if (poll(pfds, maxToClose, 0) >= 0)
{
foreach (i; 0 .. maxToClose)
{
// don't close pipe write end
if (pfds[i].fd == forkPipeOut) continue;
// POLLNVAL will be set if the file descriptor is invalid.
if (!(pfds[i].revents & POLLNVAL)) close(pfds[i].fd);
}
}
else
{
// Fall back to closing everything.
foreach (i; 3 .. maxDescriptors)
{
if (i == forkPipeOut) continue;
close(i);
// FIXME: This implementation crashes the system when RLIMIT_NOFILE
// has a big value. For a possible solution see:
// https://github.com/dlang/phobos/pull/8990
void fallback (int lowfd) {
// NOTE: malloc() and getrlimit() are not on the POSIX async
// signal safe functions list, but practically this should
// not be a problem. Java VM and CPython also use malloc()
// in its own implementation via opendir().
import core.stdc.stdlib : malloc;
import core.sys.posix.poll : pollfd, poll, POLLNVAL;
import core.sys.posix.sys.resource : rlimit, getrlimit, RLIMIT_NOFILE;

// Get the maximum number of file descriptors that could be open.
rlimit r;
if (getrlimit(RLIMIT_NOFILE, &r) != 0)
{
abortOnError(forkPipeOut, InternalError.getrlimit, .errno);
}
immutable maxDescriptors = cast(int) r.rlim_cur;

immutable maxToClose = maxDescriptors - lowfd;

// Call poll() to see which ones are actually open:
auto pfds = cast(pollfd*) malloc(pollfd.sizeof * maxToClose);
if (pfds is null)
{
abortOnError(forkPipeOut, InternalError.malloc, .errno);
}
foreach (i; 0 .. maxToClose)
{
pfds[i].fd = i + lowfd;
pfds[i].events = 0;
pfds[i].revents = 0;
}
if (poll(pfds, maxToClose, 0) >= 0)
{
foreach (i; 0 .. maxToClose)
{
// POLLNVAL will be set if the file descriptor is invalid.
if (!(pfds[i].revents & POLLNVAL)) close(pfds[i].fd);
}
}
else
{
// Fall back to closing everything.
foreach (i; lowfd .. maxDescriptors)
{
close(i);
}
}
}

// closefrom may not be available on the version of glibc we build against.
// Until we find a way to perform this check we will try to use dlsym to
// check for the function. See: https://github.com/dlang/phobos/pull/9048
version (CRuntime_Glibc)
void closefrom (int lowfd) {
static bool tryGlibcClosefrom (int lowfd) {
import core.sys.posix.dlfcn : dlopen, dlclose, dlsym, dlerror, RTLD_LAZY;

void *handle = dlopen("libc.so.6", RTLD_LAZY);
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why libc.so.6 and not libc.so?
  2. What do you think about using RTLD_NOLOAD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Why libc.so.6 and not libc.so?

If dlopen("libc.so") ever finds something different than dlopen("libc.so.6") then we definitely don't want to continue trying to load symbols. Very unlikely that this will ever happen but better safe then sorry.

  1. What do you think about using RTLD_NOLOAD?

Looking only at the man page of dlopen I can't tell what those values actually do so I went with the value they showed in the example.

Copy link
Member

@CyberShadow CyberShadow Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking is that in the unlikely case that the current binary is actually using a libc that's not libc.so.6 then we probably don't want to actually load a second libc. I understand that RTLD_NOLOAD should achieve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RTLD_NOLOAD makes it so that closefrom is no longer found so I'll keep RTLD_LAZY.

Copy link

Choose a reason for hiding this comment

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

  1. This debate could be solved by avoiding dlopen/dlsym
  2. I find it vanishingly unlikely in the event Cruntime_Glibc is true but the soversion of glibc has advanced past "6", that you'll also have libc.so.6 available

if (!handle)
return false;
scope(exit) dlclose(handle);

// Clear errors
dlerror();
alias closefromT = extern(C) void function(int) @nogc @system nothrow;
auto closefrom = cast(closefromT) dlsym(handle, "closefrom");
if (dlerror())
CyberShadow marked this conversation as resolved.
Show resolved Hide resolved
return false;

closefrom(lowfd);
return true;
}

if (!tryGlibcClosefrom(lowfd))
fallback(lowfd);
}
else
alias closefrom = fallback;
}

// We need to close all open file descriptors excluding std{in,out,err}
// and forkPipeOut because we still need it.
// Since the various libc's provide us with `closefrom` move forkPipeOut
// to position 3, right after STDERR_FILENO, and close all FDs following that.
if (dup2(forkPipeOut, 3) == -1)
abortOnError(forkPipeOut, InternalError.closefds_dup2, .errno);
forkPipeOut = 3;
// forkPipeOut needs to be closed after we call `exec`.
setCLOEXEC(forkPipeOut, true);
closefrom(forkPipeOut + 1);
}
else // This is already done if we don't inherit descriptors.
{
Expand Down Expand Up @@ -1205,6 +1256,10 @@ private Pid spawnProcessPosix(scope const(char[])[] args,
case InternalError.preExec:
errorMsg = "Failed to execute preExecFunction or preExecDelegate";
break;
case InternalError.closefds_dup2:
assert(!(config.flags & Config.Flags.inheritFDs));
errorMsg = "Failed to close inherited file descriptors";
break;
case InternalError.noerror:
assert(false);
}
Expand Down
Loading