On Sun, Jun 29, 2025 at 12:03:59AM -0400, Daniel Kahn Gillmor wrote: > By recording a copy of LISTEN_FDNAMES, we make it possible to learn > mappings from file descriptor labels (e.g., as set by > FileDescriptorName= in systemd.socket(5)). >
Just now noticing that this hasn't been reviewed yet... In general, it's okay to ping the list after a week or so if you feel like your patch has been overlooked. > This also makes it possible to invoke check_socket_activation() more > than once and have it return the same value each time. > > This is one step toward addressing > https://gitlab.com/qemu-project/qemu/-/issues/3011 > > Since we can't count on the buffer returned from getenv > persisting (getenv is documented as non-re-entrant), we need to keep a > copy of it around in case multiple subsystems want to interrogate it. getenv()'s result is non-reliable IF the application is calling setenv()/putenv() to be changing the environment. But if you treat the environment as a read-only block of memory, then the getenv() result has a lifetime equivalent to the entire process. And a quick grep: $ git grep '\b[sp][eu]tenv(' tests/tcg/multiarch/linux/linux-sigrtminmax.c: setenv("QEMU_RTSIG_MAP", rt_sigmap, 0); util/envlist.c: * than putenv(3). shows that we aren't generally modifying qemu's environ. Okay, there ARE a few calls to unsetenv() in util/systemd.c, but that's the file you're modifying, and before this patch it was already using getenv() without worrying about copying its results. We _do_ need to modify the environment after reading it, at which point, you might be right that calling getenv(), then unsetenv(), then relying on the former pointer may be unsafe; but if this is the only file manipulating the environment we have quite a bit of control over safety aspects, and may still be able to design something that doesn't have to copy getenv() results. > > This proposed implementation uses a static buffer, and breaks socket > activation with a visible error_report if LISTEN_FDNAMES is too large. > Another approach would be to g_strdup the value returned by getenv, > which would have failure modes on heap exhaustion, and would introduce > a memory leak as there's no clear opportunity to g_free the copy. A single g_strdup at process startup is not a memory leak, even if it doesn't get explicitly freed, if you can't trigger it more than once. An inability to g_free the copy is not fatal, as long as you are not repeatedly duplicating the string each time a different caller wants to also check what you already learned from getenv() on the first caller. > > Signed-off-by: Daniel Kahn Gillmor <d...@fifthhorseman.net> > --- > util/systemd.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/util/systemd.c b/util/systemd.c > index ced518f771..1eca2bd69f 100644 > --- a/util/systemd.c > +++ b/util/systemd.c > @@ -16,37 +16,62 @@ > #include "qemu/error-report.h" > > #ifndef _WIN32 > +static char fdnames[256]; Does systemd guarantee this particular size? Should it be a #define? > + > unsigned int check_socket_activation(void) > { > + static unsigned int nr_fds = -1; > const char *s; > unsigned long pid; > - unsigned long nr_fds; > + unsigned long nr_fdsl; > unsigned int i; > int fd; > int f; > int err; > > + if (nr_fds != -1) { > + return nr_fds; > + } > s = getenv("LISTEN_PID"); > if (s == NULL) { > + nr_fds = 0; > return 0; > } > err = qemu_strtoul(s, NULL, 10, &pid); > if (err) { > + nr_fds = 0; > return 0; > } > if (pid != getpid()) { > + nr_fds = 0; > return 0; > } > > s = getenv("LISTEN_FDS"); > if (s == NULL) { > + nr_fds = 0; > return 0; > } > - err = qemu_strtoul(s, NULL, 10, &nr_fds); > + err = qemu_strtoul(s, NULL, 10, &nr_fdsl); > if (err) { > + nr_fds = 0; > return 0; > } > - assert(nr_fds <= UINT_MAX); > + assert(nr_fdsl <= UINT_MAX); I can put a value in the environment for LISTEN_FDS that will trip this assertion, on a 64-bit platform. While an early exit due to garbage in the environment is okay, it should probably be cleaner than by an assertion failure. > + nr_fds = (unsigned int) nr_fdsl; > + s = getenv("LISTEN_FDNAMES"); > + if (s != NULL) { > + size_t fdnames_len = strlen(s); > + if (fdnames_len + 1 > sizeof(fdnames)) { > + error_report("LISTEN_FDNAMES is larger than %ldu bytes, " > + "ignoring socket activation.", error_report() should not use trailing '.'. A g_strdup'd buffer won't suffer from this arbitrary length limitation. > + sizeof(fdnames)); > + nr_fds = 0; > + return 0; > + } else { > + memcpy(fdnames, s, fdnames_len + 1); > + } > + } > > /* So these are not passed to any child processes we might start. */ > unsetenv("LISTEN_FDS"); > @@ -69,7 +94,7 @@ unsigned int check_socket_activation(void) > } > } > > - return (unsigned int) nr_fds; > + return nr_fds; > } > > #else /* !_WIN32 */ > -- > 2.47.2 > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
