[Libguestfs] [PATCH libnbd] generator: Pass LISTEN_FDNAMES=nbd with systemd socket activation
Richard W.M. Jones
rjones at redhat.com
Tue Jan 31 13:07:53 UTC 2023
On Tue, Jan 31, 2023 at 01:49:53PM +0100, Laszlo Ersek wrote:
> On 1/28/23 13:47, Richard W.M. Jones wrote:
> > systemd allows sockets passed through socket activation to be named
> > with the protocol they require. We only ever pass one socket, name
> > it. This environment variable is currently ignored by qemu-nbd and
> > nbdkit, but might be used by qemu-storage-daemon:
> >
> > https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
> > ---
> > generator/states-connect-socket-activation.c | 41 +++++++++++---------
> > 1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c
> > index 9a83834915..22f06d4fd3 100644
> > --- a/generator/states-connect-socket-activation.c
> > +++ b/generator/states-connect-socket-activation.c
> > @@ -34,16 +34,18 @@
> > /* This is baked into the systemd socket activation API. */
> > #define FIRST_SOCKET_ACTIVATION_FD 3
> >
> > -/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */
> > -#define PREFIX_LENGTH 11
> > -
> > extern char **environ;
> >
> > /* Prepare environment for calling execvp when doing systemd socket
> > * activation. Takes the current environment and copies it. Removes
> > - * any existing LISTEN_PID or LISTEN_FDS and replaces them with new
> > - * variables. env[0] is "LISTEN_PID=..." which is filled in by
> > - * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1".
> > + * any existing LISTEN_PID, LISTEN_FDS or LISTEN_FDNAMES, and replaces
> > + * them with new variables.
> > + *
> > + * env[0] is "LISTEN_PID=..." which is filled in by CONNECT_SA.START
> > + *
> > + * env[1] is "LISTEN_FDS=1"
> > + *
> > + * env[2] is "LISTEN_FDNAMES=nbd"
> > */
> > static int
> > prepare_socket_activation_environment (string_vector *env)
> > @@ -53,26 +55,29 @@ prepare_socket_activation_environment (string_vector *env)
> >
> > assert (env->len == 0);
> >
> > - /* Reserve slots env[0] and env[1]. */
> > + /* Reserve slots env[0]..env[2] */
> > + if (string_vector_reserve (env, 3) == -1)
> > + goto err;
> > p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
> > if (p == NULL)
> > goto err;
> > - if (string_vector_append (env, p) == -1) {
> > - free (p);
> > - goto err;
> > - }
> > + string_vector_append (env, p);
> > p = strdup ("LISTEN_FDS=1");
> > if (p == NULL)
> > goto err;
> > - if (string_vector_append (env, p) == -1) {
> > - free (p);
> > + string_vector_append (env, p);
> > + p = strdup ("LISTEN_FDNAMES=nbd");
> > + if (p == NULL)
> > goto err;
> > - }
> > + string_vector_append (env, p);
> >
> > - /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */
> > + /* Append the current environment, but remove the special
> > + * environment variables.
> > + */
> > for (i = 0; environ[i] != NULL; ++i) {
> > - if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 &&
> > - strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) {
> > + if (strncmp (environ[i], "LISTEN_PID=", 11) != 0 &&
> > + strncmp (environ[i], "LISTEN_FDS=", 11) != 0 &&
> > + strncmp (environ[i], "LISTEN_FDNAMES=", 15) != 0) {
> > char *copy = strdup (environ[i]);
> > if (copy == NULL)
> > goto err;
> > @@ -194,7 +199,7 @@ CONNECT_SA.START:
> > char buf[32];
> > const char *v =
> > nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf);
> > - strcpy (&env.ptr[0][PREFIX_LENGTH], v);
> > + strcpy (&env.ptr[0][strlen ("LISTEN_FDS=")], v);
> >
> > /* Restore SIGPIPE back to SIG_DFL. */
> > signal (SIGPIPE, SIG_DFL);
>
> 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.
> (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.
> (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.
>
> 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.
> (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.
Rich.
>
> All that said, I think we can stick with this patch; the only "actual"
> problem I see with it is the "LISTEN_FDS" reference in the last hunk.
>
> Thanks,
> Laszlo
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
More information about the Libguestfs
mailing list