[Libguestfs] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation

[Eric Blake]

On Tue, Jan 31, 2023 at 01:07:53PM +0000, Richard W.M. Jones wrote:
> > 
> > I really didn't want to obsess about this -- I spent like 10+ minutes on
> > curbing my enthusiasm! :) --, but I believe there's a semantic bug in
> > the patch, one that's directly related to my "hidden" thoughts.
> > 
> > (1) In the last hunk, strlen() is applied to "LISTEN_FDS=". However, the
> > zero-index element of the env array holds
> > "LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx". In other words, the patch
> > only gets lucky because "PID" and "FDS" are both three characters long.
> 
> Yup that is totally wrong :-(
> 
> > Relatedly, my hidden thought was that we shouldn't use so many naked
> > string literals all over the code.
> > 
> > May I take a stab at rewriting this? I feel that's the easiest way for
> > me to express what I'd propose. Basically I'd propose:
> > 
> > - an enum for listing the "keys" we need,
> > 
> > - a static array of structure elements, for expressing the environment
> >   variables (name=value), *and* the prefix lengths,
> > 
> > - a macro for populating an element of the array -- use "sizeof" for
> >   grabbing the prefix length
> 
> Sure, go for it please.

More structure will pay off if we have more variables to handle in the
long run.  For just 3 (instead of 2), it's still a toss-up in my mind
if the extra structure is worth it, but I'm not opposed to seeing an
attempt at a patch along these lines.

> 
> > (2) Pre-patch, at commit 5a02c7d2cc6a, the error handling tail of
> > prepare_socket_activation_environment() is less than ideal, IMO. Namely,
> > we have (excerpt)
> > 
> > >  err:
> > >   set_error (errno, "malloc");
> > >   string_vector_iter (env, (void *) free);
> > >   free (env->ptr);
> > >   return -1;
> > 
> > (2a) we free the vector's pointer field, but don't NULL it, nor do we
> > zero the len or cap fields.
> > 
> > We should call string_vector_reset() instead.
> 
> Yup.

The lone caller doesn't utilize env after error, but I agree that we
are better off not leaving this function to expose an incomplete 'env'
back to the caller in case we start using this function in more
places.

> 
> > (2b) Casting the address of the free() function to (void*) makes me
> > uncomfortable. It is undefined behavior by ISO C.
> > 
> > Now, I seem to remember that POSIX says in various places that pointers
> > to functions and pointers to void have identical representation, and
> > also that pointers to void and pointers to structures have identical
> > representation. One of those locations is the dlsym() spec
> > <https://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html>.
> > The other locations elude me, unfortunately. I think at least one of
> > those "other" locations may be in one of the Conformance sections; Eric
> > will know better.

I recall it being discussed in the Austin Group that POSIX explicitly
requires void* and function pointers to be cross-assignable, but only
_because_ of dlsym().  After searching for variations of 'void *',
'void pointer', 'function pointer' and 'pointer to function', I
couldn't locate anything besides the dlsym() section that elaborates
on the requirement that a POSIX compiler must allow conversion of a
function pointer into void* and back.

> > 
> > Regardless, casting "free" to a pointer-to-object, just because
> > string_vector_iter() takes a (void(*)(char*)), and not a
> > (void(*)(void*)), is questionable style, IMO.
> > 
> > I've grepped the tree for this pattern:
> > 
> >   git grep -E '\(void ?\*\) ?free'
> > 
> > and there are eleven hits.
> > 
> > Furthermore, there are *no other* _vector_iter() calls -- and not just
> > string_vector_iter() calls, but in general, _vector_iter() ones! -- than
> > these eleven.
> > 
> > I think it's time we designed either a general freeing iterator API for
> > vector, or at least added a trivial (stop-gap) wrapper function like
> > this:
> > 
> > > diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
> > > index 80d7311debfb..5221c70e3f78 100644
> > > --- a/common/utils/string-vector.h
> > > +++ b/common/utils/string-vector.h
> > > @@ -39,4 +39,10 @@
> > >
> > >  DEFINE_VECTOR_TYPE(string_vector, char *);
> > >
> > > +static inline void
> > > +string_free (char *string)
> > > +{
> > > +  free (string);
> > > +}
> > > +
> > >  #endif /* STRING_VECTOR_H */
> > 
> > Comments please :)
> 
> Agreed.

I'm also in agreement that tweaking our vector interface for the ways
in which we actively use it (without having to spell out explicit
(void*) casts) would be welcome.

> 
> > (3) At the last hunk, the code suggests we're between fork() and exec().
> > Per POSIX
> > <https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html>,
> > there we can only call async-signal-safe functions:
> > 
> > > the child process may only execute async-signal-safe operations until
> > > such time as one of the exec functions is called
> > 
> > The list of async-signal-safe functions can be found at
> > <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03>.
> > snprintf() and sprintf() are not on that list, so it makes sense for
> > nbd_internal_fork_safe_itoa() to exist.
> 
> Yes, in the past we have actually hit real bugs because of this.  One
> I recall is:
> 
> https://github.com/libguestfs/libguestfs/commit/e1c9bbb3d1d5ef81490977060120dda0963eb567
> 
> They are very hard to diagnose.  This one only happened when a certain
> glibc feature was enabled.
> 
> > The remaining functions we call in this context also seem to be on the
> > list... except for execvp().
> > 
> > execvp() scans PATH, and is not safe to use in this concept.
> 
> That's quite annoying.
> 
> > I think we should call execve() instead. First, it is async-signal-safe.
> > Second, it could take "env.ptr" directly; I do find the "environ"
> > assignment a bit dubious, even if it happens to conform to POSIX.
> > 
> > What image are we executing here, to begin with? Do we really depend on
> > PATH searching? Or do we rely on execvp() transparently launching shell
> > scripts?
> 
> Yes we do depend on path search.  It is usually called in cases such
> as this:
> 
> https://gitlab.com/nbdkit/libnbd/-/blob/5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2/examples/open-qcow2.c#L35
> 
> I wonder if we can just ignore this one until someone complains
> about the bug.

The alternative to relying on execvp() to scan PATH is to pre-scan
PATH ourselves before fork().  I wish there were a helper function in
glibc that would quickly return the absolute path that execvp() would
otherwise utilize.  fexecve() also comes in handy for avoiding TOCTTOU
races, but it is newer and not as widely available.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org