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

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