Re: [Libguestfs] [libnbd PATCH v3 19/19] socket activation: set LISTEN_FDNAMES

[Eric Blake <eblake@xxxxxxxxxx>]

[attempting to loop in systemd folks; this started in libnbd at
https://listman.redhat.com/archives/libguestfs/2023-March/031178.html
- although I may have to retry since I'm not a usual subscriber of
systemd-devel]

On Fri, Mar 24, 2023 at 11:32:26AM +0100, Laszlo Ersek wrote:
> >> @@ -245,6 +245,9 @@  CONNECT_SA.START:
> >>                   "LISTEN_PID=", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
> >>    SACT_VAR_PUSH (sact_var, &num_vars,
> >>                   "LISTEN_FDS=", "1", NULL);
> >> +  if (h->sact_name != NULL)
> >> +    SACT_VAR_PUSH (sact_var, &num_vars,
> >> +                   "LISTEN_FDNAMES=", h->sact_name, NULL);
> >>    if (prepare_socket_activation_environment (&env, sact_var, num_vars) == -1)
> >
> > If I'm reading this correctly, this does wipe an inherited
> > LISTEN_FDNAMES from the environment in the case where the application
> > linked with libnbd started life with a (different) socket activation,
> > but now the user wants to connect to an nbd server without setting a
> > name (default usage, or explicitly requested a name of "").
> 
> Good observation; this is a functionality gap that goes back to v1 of
> this patch. (I've not investigated the specifics of systemd socket
> activation before; but see below.)
> 
> > Put another way, SACT_VAR_PUSH as written appears to be only additive
> > for replacement purposes (if I pushed a variable, I intend to override
> > it in the child process, so don't copy it from environ if one was
> > previously there), but not effective for deletion purposes (I don't
> > intend to set the variable, but if it is already set in environ, I
> > want it omitted in the child's copy).
> >
> > Is there a way to rework this so that you can pass NULL as the fourth
> > parameter as an indication of an unset request (vs. "" when you want
> > it set to the empty string)?  At which point, you would drop the 'if
> > (h->sact_name != NULL)', and just blindly use SACT_VAR_PUSH(,
> > h->sact_name,).  That has ripple effects earlier in the series to
> > support those semantics.
> 
> For understanding your point, I have had to read up on systemd socket
> activation:
> 
>   https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html
> 
> Let me quote a part:
> 
> > Under specific conditions, the following automatic file descriptor names are returned:
> >
> >   [...]
> >   "unknown" -- The process received no name for the specific file
> >                descriptor from the service manager.
> >   [...]
> 
> I've also checked the implementation of sd_listen_fds_with_names():
> 
>   https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-daemon.c
> 
...
> 
> (2) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and just "pass through"
>     an *inherited* LISTEN_FDNAMES variable, then it will (in the general
>     case) confuse the nbd server that we start. Namely, if
>     LISTEN_FDNAMES has multiple colon-separated elements (more than 1),
>     or is the empty string (= 0 elements), then
>     sd_listen_fds_with_names() in the nbd server will fail the "n_names
>     != n_fds" check, and return (-EINVAL). If LISTEN_FDNAMES happens to
>     have one element, then sd_listen_fds_with_names() will succeed, but
>     the returned name will confuse the nbd server.

...

Eww - this may be a bigger systematic issue caused by systemd itself,
and we should report it there (done by adding in cc here).  They did
not specify LISTEN_FDNAMES at the time LISTEN_PID was first
documented, so the likelihood of libnbd not being the only application
that happens to leak inherited LISTEN_FDNAMES through to the child
process is non-zero, where this sort of bug will bite more than one
client of systemd socket activation.  And it is this sort of
backwards-incompatibility caused by the systemd extension that they
will need to be more careful of avoiding if they ever add any future
LISTEN_* environment variables.


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