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

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.

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


(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.

(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 :)


(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.

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.

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?


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

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