10:03 a.m.
On 2/21/23 13:08, Daniel P. Berrangé wrote:
On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
> execvp() [01] is powerful:
>
> - it performs PATH search [02] if necessary,
>
> - it falls back to executing the file with the shell as a shell
> script in case the underlying execv() or execve() fails with
> ENOEXEC.
>
> However, execvp() is not async-signal-safe [03], and so we shouldn't
> call it in a child process forked from a multi-threaded parent
> process [04], which the libnbd client application may well be.
Is that actually a problem in the real world ? There are various
things not technically listed as async-signal-safe by POSIX, which
all sane impls none the less make safe, or are practically safe in
any sane program using them.
By my count, it's virtually impossible to implement exec*p*() and
friends without calling malloc() internally. That's the case even if you
can get PATH in there without any problems. In turn, malloc() relies on
libc-internal mutex(es) or other synchronization mechanisms, so that
multiple threads can call malloc() / free() etc simultaneously, for
managing the one shared address space of the process.
The problem is with calling fork() in a multi-threaded process. The
child process created by fork() only has one thread -- the thread
returning from fork(). All other threads simply "disappear", as viewed
from the child process's perspective. This means that, if one of those
"other" (disappearing) threads was in the middle of malloc(), holding
for example a mutex, then in the child process, that mutex will appear
as acquired, but no thread will own it -- so no thread will ever release
it. Then, when the one thread returning from fork() in the child process
calls exec*p*(), the malloc() call internal to exec*p*() will deadlock
on that mutex.
Rich mentioned that libnbd had actually encountered a bug of this kind,
just not specifically in exec*p*().
So the "async-signal-safe" term is a bit misleading here; I think what
POSIX actually means is a kind of general reentrancy. Anyway, the POSIX
language does use "async-signal-safe" when discussing this topic of
threads, in the fork() specification.
Now, the exec*p*() manual at
https://man7.org/linux/man-pages/man3/execvp.3.html
calls execlp(), execvp(), execvpe() "MT-Safe env", so you are right
about at least Linux/glibc. I've (intentionally) not looked at the glibc
implementation of execvp(), but I agree that, *if* we are happy with
getenv() just because the linux/glibc manual calls it "MT-Safe env",
*then* we could arguably be satisfied with execvp() as well.
I don't know if/how that applies to FreeBSD / OpenBSD though.
https://man.freebsd.org/cgi/man.cgi?query=execvp
https://man.openbsd.org/execvp.3
These man pages do not seem to contain any of the strings "mt",
"async",
"signal", "safe", "thread".
IIUC with execvp the risk would be that setenv makes any use of the
environment potentially unsafe,
to a smaller extent, yes
and possibly execvp might use malloc which is also technically
unsafe.
primarily this one, yes
Both of these factors would technically make your replacement unsafe
too.
No, they wouldn't. That's the whole point. Both getenv() and malloc()
are restricted to the _init API, which is called in the parent process,
before fork(). The necessary information is prepared in a context
structure, which the child only inherits. After fork(), the child calls
a second API -- the effective execvpe() replacement -- which operates on
the inherited context structure. The only functions called from this
second API are write() and abort() -- indirectly, in case the new
async-signal-safe assert() variant fails --, and execve(). All three of
these functions are async-signal-safe.
Apps simply shouldn't ever call setenv once threads are created,
and
malloc() is safe in any impl that is relevant these days.
FWIW, the linux/glibc manual states, in
<https://man7.org/linux/man-pages/man2/fork.2.html>;:
* After a fork() in a multithreaded program, the child can
safely call only async-signal-safe functions (see
signal-safety(7)) until such time as it calls execve(2).
and <https://man7.org/linux/man-pages/man7/signal-safety.7.html> does
not list either execvp() or malloc().
The glibc manual is more verbose
<https://www.gnu.org/software/libc/manual/html_node/Creating-a-Process.htm...;:
The _Fork function is similar to fork, but it does not invoke any
callbacks registered with pthread_atfork, nor does it reset any
internal state or locks (such as the malloc locks). In the new
subprocess, only async-signal-safe functions may be called, such as
dup2 or execve.
This implies that malloc() and fork() in glibc actually inter-operate,
so that fork() -- not _Fork() -- "reset malloc locks".
This confirms your point about latest glibc, but I don't know if/how the
same applies to the BSDs, or to earlier versions of glibc (for example
the one in RHEL7, which libnbd still cares about, IIUC).
(I'm unsure if it's wise for me to dig into the glibc commit history
with git-blame, for finding the earliest release that adds this extra
safety beyond POSIX.)
Laszlo