While `execve()` might allow (probably malicious) users to execute a
program with an empty `argv` array, gspawn does not. It’s not actually
possible, as the path to the binary to execute is not specified
separately from the argument array.
Explicitly document and encode that in preconditions.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
If a seccomp policy is set up incorrectly so that it returns `EPERM` for
`close_range()` rather than `ENOSYS` due to it not being recognised, no
error would previously be reported from GLib, but some file descriptors
wouldn’t be closed, and that would cause a hung zombie process. The
zombie process would be waiting for one half of a socket to be closed.
Fix that by correctly propagating errors from `close_range()` back to the
parent process so they can be reported correctly.
Distributions which aren’t yet carrying the Docker fix to correctly
return `ENOSYS` from unrecognised syscalls may want to temporarily carry
an additional patch to fall back to `safe_fdwalk()` if `close_range()`
fails with `EPERM`. This change will not be accepted upstream as `EPERM`
is not the right error for `close_range()` to be returning.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2580
Although unlikely, these functions can fail, e.g. if we run out of file
descriptors. Check for errors to improve robustness. This is especially
important now that I changed our use of dupfd_cloexec() to avoid
returning fds smaller than the largest fd in target_fds. An application
that attempts to remap to the highest-allowed fd value deserves at least
some sort of attempt at error reporting, not silent failure.
We currently dup all source fds to avoid possible conflation with the
target fds, but fail to consider that the result of a dup might itself
conflict with one of the target fds. Solve this the easy way by duping
all source_fds to values that are greater than the largest fd in
target_fds.
Fixes#2503
In case child_err_report_fd conflicts with one of the target_fds, the
code here is careful to dup child_err_report_fd in order to avoid
conflating the two. It was a good idea, but evidently was not tested,
because the newly-created fd is not created with CLOEXEC set. This means
it stays open in the child process, causing the parent to hang forever
waiting to read from the other end of the pipe. Oops!
The fix is simple: just set CLOEXEC. This removes our only usage of the
safe_dup() function, so it can be dropped.
Fixes#2506
It’s a new flag added to `close_range()` in kernel 5.11, which will
allow us to speed up setting `CLOEXEC` on ranges of file descriptors.
This currently happens in some situations when executing a new binary
with `GSpawn`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
A reader might think "how would a process terminate without an exit
status?", or equivalently, "what harm would it do if I assume every
termination has an exit status?" without this reminder that termination
with a signal is also reasonably common.
Signed-off-by: Simon McVittie <smcv@collabora.com>
On Unix platforms, wait() and friends yield an integer that encodes
how the process exited. Confusingly, this is usually not the same as
the integer passed to exit() or returned from main(): conceptually it's
an integer encoding of this tagged union:
enum { EXITED, SIGNALLED, ... } tag;
union {
int exit_status; /* if EXITED */
struct {
int terminating_signal;
bool core_dumped;
} terminating_signal; /* if SIGNALLED */
...
} detail;
Meanwhile, on Windows, wait statuses and exit statuses are
interchangeable.
I find that it's clearer what is going on if we are consistent about
referring to the result of wait() as a "wait status", and the value
passed to exit() as an "exit status".
GSubprocess already gets this right: g_subprocess_get_status() returns
the wait status, while g_subprocess_get_exit_status() genuinely returns
the exit status. However, the GSpawn family of APIs has tended to
conflate the two.
Confusingly, g_spawn_check_exit_status() has always checked a wait
status, and it would not be correct to pass an exit status to it; so
let's deprecate it in favour of g_spawn_check_wait_status(), which
does the same thing that g_spawn_check_exit_status() always did.
Code that needs backwards-compatibility with older GLib can use:
#if !GLIB_CHECK_VERSION(2, 69, 0)
#define g_spawn_check_wait_status(x) (g_spawn_check_exit_status (x))
#endif
Signed-off-by: Simon McVittie <smcv@collabora.com>
This is basically glnx_steal_fd() from libglnx. We already had two
private implementations of it in GLib.
Signed-off-by: Simon McVittie <smcv@collabora.com>
This is a simple wrapper around the new source/target FD mapping
functionality in `fork_exec()`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
If `stdout_fd` was set to (say) 6, and `stderr_fd` was set to 1, the
`set_cloexec()` call for setting up `stderr` would set the new `stdout`
for the forked process to be closed in the pending `exec()`.
This would cause the child process to error when writing to `stdout`.
This situation happens when using `G_SUBPROCESS_FLAGS_STDERR_MERGE`.
Add some conditions to prevent setting `CLOEXEC` in such cases.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
It was previously possible to specify the FD number which
`child_err_report_fd` was assigned, as a target FD in the FD mapping set
up using `g_subprocess_launcher_take_fd()`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2097
This effectively moves some of the functionality of `GSubprocess`
(`g_subprocess_launcher_take_fd()`) into `g_spawn*()`, which should make
implementation a little simpler.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
This is an internal change which won’t affect the public API. It should
introduce no functional changes, but simplifies the code a little.
The arguments from `fork_exec_with_pipes()` have been added to
`fork_exec_with_fds()`. `child_close_fds` has been dropped since it’s
now an implementation detail within the function.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
We preallocate buffers that are used after forked. That is because
malloc()/free() are not async-signal-safe and must not be used between
fork() and exec().
However, for the child process that exits without fork, valgrind wrongly
reports these buffers as leaked.
That can be suppressed with "--child-silent-after-fork=yes", but it is
cumbersome.
Work around by trying to allocate the buffers on the stack. At
least in the common cases where the pointers are small enough
so that we can reasonably do that.
If the buffers happen to be large, we still allocate them on the heap
and the problem still happens. Maybe we could have also allocated them
as thread_local, but currently glib doesn't use that.
[smcv: Cosmetic adjustments to address review comments from pwithnall]
do_exec() and g_execute() rely on being passed a NULL search path
if we intend to avoid searching the PATH, but since the refactoring
in commit 62ce66d4, this was never done. This resulted in some spawn
calls searching the PATH when it was not intended.
Spawn calls that go through the posix_spawn fast-path were unaffected.
The deprecated gtester utility, as used in GTK 3, relies on the
ability to run an executable from the current working directory by
omitting the G_SPAWN_SEARCH_PATH flag. This *mostly* worked, because
our fallback PATH ends with ".". However, if an executable of the
same name existed in /usr/bin or /bin, it would run that instead of the
intended test: in particular, GTK 3's build-time tests failed if
ImageMagick happens to be installed, because gtester would accidentally
run display(1) instead of testsuite/gdk/display.
Fixes: 62ce66d4 "gspawn: Don’t use getenv() in async-signal-safe context"
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977961
It’s possible that GLib will eventually be compiled against a version of
libc which supports `close_range()` (hence `HAVE_CLOSE_RANGE` will be
defined), but then run against an older kernel which doesn’t support it.
In this case, we want to fall back to `fdwalk()`, which should work on
such systems.
This is what cpython does: 3529718925/Python/fileutils.c (L2227)
Spotted by Allison Karlitskaya in !1688.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
It’s landed in kernel 5.9: http://lkml.iu.edu/hypermail/linux/kernel/2008.0/02649.html
Note, this is untested because I currently don’t have kernel 5.9. We can
fix anything up if it breaks once the new syscall is wrapped in glibc.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This is very unlikely to happen, but add error handling to mirror the
other calls to `safe_open()`, and shut Coverity up.
Coverity CID: #1430611
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Use this to replace the much-hated `g_debug()` which told people that
`posix_spawn()` (the fast path) wasn’t being used for various reasons.
If people want to make their process spawning faster now, they’ll have
to use a profiling tool like sysprof to check their program’s
performance. Shocking.
I think I was wrong to put this `g_debug()` in there in the first place
— it hasn’t served its purpose of making people speed up their spawn
paths to use `posix_spawn()`, it’s only cluttered up logs and frustrated
people.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Allocate a working buffer before calling `fork()` to avoid calling
`malloc()` in the async-signal-safe context between `fork()` and
`exec()`, where it’s not safe to use.
In this case, the buffer is used to assemble a wrapper around `argv` so
it can be run under `/bin/sh`.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #2140
Allocate a working buffer before calling `fork()` to avoid calling
`malloc()` in the async-signal-safe context between `fork()` and
`exec()`, where it’s not safe to use.
In this case, the buffer is used to assemble elements from `PATH` with
the binary from `argv[0]` to try executing them.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
Query the environment before calling `fork()` so that it doesn’t have to
be called in the async-signal-safe context between `fork()` and
`exec()`.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
They’re not safe to call in an async-signal-safe context on Linux.
`sysconf()` is safe to call on FreeBSD and OpenBSD (at least), so
continue doing that.
This will reduce performance in the (already low performance) fallback
case where `/proc` is inaccessible to a forked process on Linux, while
spawning a subprocess.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
Use the error handling infrastructure which already exists for other
failures in the async-signal-safe context.
`g_assert()` is unlikely to have caused problems in practice because it
is only async-signal-unsafe when the assertion condition fails.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
While `g_ascii_isdigit()` *is* currently async-signal-safe, it’s going
to be hard to remember to keep it that way if the implementation changes
in future.
It seems more robust to just reimplement it here, given that it’s not
much code.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
Use normal `close()` instead, which is guaranteed to be
async-signal-safe.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
Functions called between `fork()` and `exec()` have to be
async-signal-safe.
Add a comment to each function which is called in that context, and
`FIXME` comments to the non-async-signal-safe functions which end up
being called as leaves of the call graph.
The following commits will fix those `FIXME`s.
See `man 7 signal-safety`.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #2140
