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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs