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

On 2/21/23 18:06, Eric Blake wrote:
> On Tue, Feb 21, 2023 at 12:55:49PM +0100, Laszlo Ersek wrote:
>> On 2/15/23 23:28, Eric Blake wrote:
>>> On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
>>>> execvp() [01] is powerful:
>>>
>>> You KNOW this is going to be a juicy commit message when the first
>>> line contains a 2-digit footnote ;)
>>
>> Haha, yes :) Clearly I originally started the commit message with
>> single-digit footnotes, but I ended up needing another digit :)
> 
> Trimming my responses to just your open questions...
Sorry, I should have been trimming too.
>>> [1] And this simplifies if empty PATH can imply searching in
".".
>>
>> If we ever get practical problems with PATH="", let's
update the code
>> then, incrementally.
> 
> Indeed.  No need to fret about a corner case that no one will hit; or
> if they can prove that it matters, it's an easy incremental patch at
> that time.
Well, given Daniel's comments meanwhile, it seems like the original
execvp() is something we shouldn't fret about. :/
> Again agreed - any user desperate enough to pass in an atypical binary
> name or put atypical relative directory names in PATH gets what they
> deserve; as long as we don't think our code misbehaving on argv[0] of
> "+s" can be abused as a security hole, then not worrying about
the
> corner case in our code is just fine for this patch, and an
> incremental patch on top if we can think of why it is
> security-sensitive after all.
Can you please elaborate on "+s"? (I'd like to understand your
point
regardless of this patch, too.)
>> How does "//binary" differ from "/binary", in
pathname resolution?
> 
> POSIX intentionally says that file names beginning with leading
"//"
> but not "///" have implementation-defined semantics.  Linux's
> definition is that "//" is a synonym for "/", so it
makes no
> difference there.  But on Cygwin, "/machine" is a file (or
directory)
> living in the root directory, while "//machine/share" is a
network
> share access path that locates the shared resource "share" on
> network-addressable "machine" (think Windows \\MACHINE\SHARE
syntax).
> Native windows does not let you access bare \\ or \\MACHINE, but
> Cygwin has further generalized those so that "ls //" shows you a
list
> of all machines currently advertising shares in your local subnet (do
> not try it on a large enterprise subnet - it can take a LOONG time).
> Cygwin also defaults to having "/cygdrive/c" be a synonym to
Windows
> C:\, but has an option for you to re-spell it as "//c" (that is,
> expose all of your drive letters the same as remote-access machines by
> setting the cygdrive prefix to empty instead of "cygdrive").
Ah, found it:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13

"""
If a pathname begins with two successive <slash> characters, the first
component following the leading <slash> characters may be interpreted in
an implementation-defined manner, although more than two leading <slash>
characters shall be treated as a single <slash> character.
"""

(... I wanted to say that "there's no replacement for reading POSIX in
full", but now I'm wondering if reading POSIX *at all* makes sense...)
> And thanks again for a detailed commit message and code comments for
> something that would otherwise be hard to maintain if we didn't have
> strong rationale for why we are doing it.
... That rationale may just have fallen away.

Laszlo

On Tue, Feb 21, 2023 at 07:07:38PM +0100, Laszlo Ersek
wrote:
> On 2/21/23 18:06, Eric Blake wrote:
> > On Tue, Feb 21, 2023 at 12:55:49PM +0100, Laszlo Ersek wrote:
> >> On 2/15/23 23:28, Eric Blake wrote:
> >>> On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote:
> >>>> execvp() [01] is powerful:
> >>>
> >>> You KNOW this is going to be a juicy commit message when the
first
> >>> line contains a 2-digit footnote ;)
> >>
> >> Haha, yes :) Clearly I originally started the commit message with
> >> single-digit footnotes, but I ended up needing another digit :)
> > 
> > Trimming my responses to just your open questions...
> 
> Sorry, I should have been trimming too.
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.
> 
> >>> [1] And this simplifies if empty PATH can imply searching in
".".
> >>
> >> If we ever get practical problems with PATH="",
let's update the code
> >> then, incrementally.
> > 
> > Indeed.  No need to fret about a corner case that no one will hit; or
> > if they can prove that it matters, it's an easy incremental patch
at
> > that time.
> 
> Well, given Daniel's comments meanwhile, it seems like the original
> execvp() is something we shouldn't fret about. :/
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.
> 
> > Again agreed - any user desperate enough to pass in an atypical binary
> > name or put atypical relative directory names in PATH gets what they
> > deserve; as long as we don't think our code misbehaving on argv[0]
of
> > "+s" can be abused as a security hole, then not worrying
about the
> > corner case in our code is just fine for this patch, and an
> > incremental patch on top if we can think of why it is
> > security-sensitive after all.
> 
> Can you please elaborate on "+s"? (I'd like to understand
your point
> regardless of this patch, too.)
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?
> 
> >> How does "//binary" differ from "/binary", in
pathname resolution?
> > 
> > POSIX intentionally says that file names beginning with leading
"//"
> > but not "///" have implementation-defined semantics. 
Linux's
> > definition is that "//" is a synonym for "/", so
it makes no
> > difference there.  But on Cygwin, "/machine" is a file (or
directory)
> > living in the root directory, while "//machine/share" is a
network
> > share access path that locates the shared resource "share"
on
> > network-addressable "machine" (think Windows \\MACHINE\SHARE
syntax).
> > Native windows does not let you access bare \\ or \\MACHINE, but
> > Cygwin has further generalized those so that "ls //" shows
you a list
> > of all machines currently advertising shares in your local subnet (do
> > not try it on a large enterprise subnet - it can take a LOONG time).
> > Cygwin also defaults to having "/cygdrive/c" be a synonym to
Windows
> > C:\, but has an option for you to re-spell it as "//c" (that
is,
> > expose all of your drive letters the same as remote-access machines by
> > setting the cygdrive prefix to empty instead of "cygdrive").
> 
> Ah, found it:
> 
>
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13
> 
> """
> If a pathname begins with two successive <slash> characters, the
first
> component following the leading <slash> characters may be interpreted
in
> an implementation-defined manner, although more than two leading
<slash>
> characters shall be treated as a single <slash> character.
> """
> 
> (... I wanted to say that "there's no replacement for reading
POSIX in
> full", but now I'm wondering if reading POSIX *at all* makes
sense...)
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.
> 
> > And thanks again for a detailed commit message and code comments for
> > something that would otherwise be hard to maintain if we didn't
have
> > strong rationale for why we are doing it.
> 
> ... That rationale may just have fallen away.
It certainly got weakened.
> 
> Laszlo
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org