Re: [Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()

On Tue, Feb 21, 2023 at 07:07:38PM +0100, Laszlo Ersek wrote: No worries. Some email clients make it easier to gloss over quoted text than others, but I tend to trim to try and focus the reader's attention on where I am actually adding to the conversation. At the other extreme, sometimes I trim too much context for someone jumping into the conversation late - in open source this is fine because there are public archives, but in other contexts it can have negative consequences. glibc marks execvp() and exevpe() as 'MT-Safe env', which means it does not modify 'environ' and presumably does not use 'malloc'. If the only reason POSIX marks execvp() as thread-unsafe is because of its interaction with getenv() for PATH and therefore unsafe to exec a child in one thread while another is calling putenv(), then it is no worse than our use of getenv() without locking on the grounds that no sane app will be doing setenv() after spawning threads. But you've gone to a lot of work on this series; I'm still in favor of including your work, even if decide our premise behind it is weaker than intended. As I understood it, the premise is that actively avoiding as many non-async-safe functions as possible between fork() and exec() is always a good idea to avoid the deadlock created when a multithreaded process fork()s in one thread while another thread holds a mutex on the non-async-safe resource (in the child process, the other thread no longer exists and therefore can never release the resource). Knowing WHICH resources are liable to be locked in other threads makes it easier to reason about which generically non-async-safe functions can be used if we make other limiting restrictions (such as glibc's 'MT-Safe env' designation), but that requires more thought than blindly avoiding all non-async-safe functions. https://www.austingroupbugs.net/view.php?id=1440 is where it came up in the Austin Group. Basically, having system("+s") be _required_ to invoke ["/path/to/sh", "-c", "+s"] is risky - "+s" is ambiguous between being the name of a real script and being a shell option (POSIX already warns that naming an executable with a leading "-" is unwise because of potential conflicts with options, but 'sh' has the special rules that + as well as - can introduce options for historical reasons). Calling ["/path/to/sh", "-c", "--", "+s"] is unambiguous for a POSIX sh that understands "--" as the end-of-options marker, but not all historical sh did that. It is also possible to use ["/path/to/sh", "-c", " +s"] (note the added leading space, which is ignored by the shell), but that requires space for injecting the leading space, and how do you do that concisely while still maintaining async-safety? Alas, this rings too close to home... Standards are only useful if they are likely to be followed, which in turn is harder if they are hard to read. It certainly got weakened. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org