Correctly report "asprintf" in the error message when asprintf() fails. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; --- Notes: v4: - pick up R-b's from Rich and Eric generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index e4b3b565ae2e..ddbccff8240d 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -127,7 +127,7 @@ CONNECT_SA.START: if (asprintf (&h->sact_sockpath, "%s/sock", h->sact_tmpdir) == -1) { SET_NEXT_STATE (%.DEAD); - set_error (errno, "strdup"); + set_error (errno, "asprintf"); return 0; }

F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC expands to an "int". In turn, it's bad hygiene to manipulate the sign bit of (signed) integers with bit operations: ~FD_CLOEXEC Convert FD_CLOEXEC to "unsigned int" for the bitwise complement operator: ~(unsigned)FD_CLOEXEC The bitwise complement then results in an "unsigned int". "Flags" (of type "int", and having, per POSIX, a non-negative value returned by fcntl(F_GETFD)) will be automatically converted to "unsigned int" by the usual arithmetic conversions for the bitwise AND operator: flags & ~(unsigned)FD_CLOEXEC We could pass the result of *that* to fcntl(), due to (a) the value being in range for "signed int" ("flags" is a non-negative "int", and we're only clearing a value bit), and (b) non-negative "int" values being represented identically by "unsigned int" (C99 6.2.5 p9). But, for clarity, let's cast the result to "int" explicitly: (int)(flags & ~(unsigned)FD_CLOEXEC) Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U5 generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 61d3d1900f45..b9845c11a221 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -182,11 +182,11 @@ CONNECT_SA.START: int flags = fcntl (s, F_GETFD, 0); if (flags == -1) { nbd_internal_fork_safe_perror ("fcntl: F_GETFD"); _exit (126); } - if (fcntl (s, F_SETFD, flags & ~FD_CLOEXEC) == -1) { + if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) { nbd_internal_fork_safe_perror ("fcntl: F_SETFD"); _exit (126); } }

The CONNECT_SA.START handler constructs several resources; some of those are needed permanently if the entire handler succeeds, while others are needed only temporarily, for constructing the permanent objects. Currently, the various error branches in the handler deal with resource release individually, leading to code duplication and dispersed responsibilities; if we want to construct a new resource, then, under the existent pattern, we'll have to modify multiple error branches to release it as well. In my opinion, the best pattern for dealing with this scenario is the following structure: - Place cascading error handling labels at the end of the function -- an error handling slide in effect. The order of destruction on this slide is the inverse of construction. - Whenever an operation fails mid-construction, enter the slide such that precisely the thus far constructed objects be freed. Note that the goto statements and the destination labels nest properly. - The very last construction step needs no rollback, because the last successful step completes the entire function / handler. When this happens, *commit* by (a) outputting the permanent resources, (b) setting a top-level "final result" variable to "success" (aka "ownership of permanent resources transferred to the caller"), and (c) falling through to the slide. On the slide, use the "final result" variable to skip the release of those resources that have been output as permanent ones. This way, each resource is freed in exactly one location, and whenever an error occurs, no resource is even *checked* for rollback if its construction could not be tried, or completed, in the first place. The introduction of a new resource needs one properly placed construction step and one matchingly placed error handling label. Rich notes that "state machine labels match the basic regexp pattern: ^ \\([A-Z0-9][A-Z0-9_\\.]*\\):$ (see generator/state_machine_generator.ml), and because of the all-capitalization of state names, they won't match the lower case ordinary C labels we're using here". The restructuring highlights the following leak: whenever any construction step after bind() fails, the UNIX domain socket address (i.e., the pathname in the filesystem) is leaked. (The filesystem object may well be removed once the application decides to call nbd_close(), but until then, after the CONNECT_SA.START handler returns with failure, unusable (half-constructed) resources linger around indefinitely. A library API should either fully succeed or fully fail.) We'll plug the leak later in this series. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b - remark in the commit message that the generated state machine labels, and the manual labels introduced by the patch, belong to distinct "namespaces" [Rich] - cumbersome rebase conflict resolution near the prepare_socket_activation_environment() call site, due to keeping set_error() internal to that callee, in patch "socket activation: clean up responsibilities of prep.sock.act.env.()" context:-W generator/states-connect-socket-activation.c | 85 +++++++++++--------- 1 file changed, 49 insertions(+), 36 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index e3f7d05670b0..b20c3f1f10bb 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -97,132 +97,145 @@ prepare_socket_activation_environment (string_vector *env) STATE_MACHINE { CONNECT_SA.START: + enum state next; + char *tmpdir; + char *sockpath; int s; struct sockaddr_un addr; string_vector env; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); + next = %.DEAD; + /* Use /tmp instead of TMPDIR because we must ensure the path is * short enough to store in the sockaddr_un. On some platforms this * may cause problems so we may need to revisit it. XXX */ - h->sact_tmpdir = strdup ("/tmp/libnbdXXXXXX"); - if (h->sact_tmpdir == NULL) { - SET_NEXT_STATE (%.DEAD); + tmpdir = strdup ("/tmp/libnbdXXXXXX"); + if (tmpdir == NULL) { set_error (errno, "strdup"); - return 0; + goto done; } - if (mkdtemp (h->sact_tmpdir) == NULL) { - SET_NEXT_STATE (%.DEAD); + + if (mkdtemp (tmpdir) == NULL) { set_error (errno, "mkdtemp"); - /* Avoid cleanup in nbd_close. */ - free (h->sact_tmpdir); - h->sact_tmpdir = NULL; - return 0; + goto free_tmpdir; } - if (asprintf (&h->sact_sockpath, "%s/sock", h->sact_tmpdir) == -1) { - SET_NEXT_STATE (%.DEAD); + if (asprintf (&sockpath, "%s/sock", tmpdir) == -1) { set_error (errno, "asprintf"); - return 0; + goto rmdir_tmpdir; } s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false); if (s == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "socket"); - return 0; + goto free_sockpath; } addr.sun_family = AF_UNIX; - memcpy (addr.sun_path, h->sact_sockpath, strlen (h->sact_sockpath) + 1); + memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1); if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) { - SET_NEXT_STATE (%.DEAD); - set_error (errno, "bind: %s", h->sact_sockpath); - close (s); - return 0; + set_error (errno, "bind: %s", sockpath); + goto close_socket; } if (listen (s, SOMAXCONN) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "listen"); - close (s); - return 0; + goto close_socket; } - if (prepare_socket_activation_environment (&env) == -1) { - SET_NEXT_STATE (%.DEAD); + if (prepare_socket_activation_environment (&env) == -1) /* prepare_socket_activation_environment() calls set_error() internally */ - close (s); - return 0; - } + goto close_socket; pid = fork (); if (pid == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); - close (s); - string_vector_empty (&env); - return 0; + goto empty_env; } + if (pid == 0) { /* child - run command */ if (s != FIRST_SOCKET_ACTIVATION_FD) { if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) { nbd_internal_fork_safe_perror ("dup2"); _exit (126); } if (close (s) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } } else { /* We must unset CLOEXEC on the fd. (dup2 above does this * implicitly because CLOEXEC is set on the fd, not on the * socket). */ int flags = fcntl (s, F_GETFD, 0); if (flags == -1) { nbd_internal_fork_safe_perror ("fcntl: F_GETFD"); _exit (126); } if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) { nbd_internal_fork_safe_perror ("fcntl: F_SETFD"); _exit (126); } } char buf[32]; const char *v = nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf); strcpy (&env.ptr[0][PREFIX_LENGTH], v); /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } environ = env.ptr; execvp (h->argv.ptr[0], h->argv.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } - /* Parent. */ - close (s); - string_vector_empty (&env); + /* Parent -- we're done; commit. */ + h->sact_tmpdir = tmpdir; + h->sact_sockpath = sockpath; h->pid = pid; h->connaddrlen = sizeof addr; memcpy (&h->connaddr, &addr, h->connaddrlen); - SET_NEXT_STATE (%^CONNECT.START); + next = %^CONNECT.START; + + /* fall through, for releasing the temporaries */ + +empty_env: + string_vector_empty (&env); + +close_socket: + close (s); + +free_sockpath: + if (next == %.DEAD) + free (sockpath); + +rmdir_tmpdir: + if (next == %.DEAD) + rmdir (tmpdir); + +free_tmpdir: + if (next == %.DEAD) + free (tmpdir); + +done: + SET_NEXT_STATE (next); return 0; } /* END STATE MACHINE */

On 3/23/23 15:29, Eric Blake wrote: Yes, "g_steal_pointer". (Either you or Dan has mentioned it recently.) Thanks!

Per POSIX, execvp() is not safe to call in a child process forked from a multi-threaded process. We can now replace the execvp() call in the child process with a call to our fork-safe (async-signal-safe) variant. Prepare our internal execvpe context on the parent's construction path, use the context in the child, and release the context in the parent on the way out, regardless of whether the handler as a whole succeeded or not. (The context is only a temporary resource.) Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b - resolve rebase conflict near the prepare_socket_activation_environment() call site, due to keeping set_error() internal to that callee, in patch "socket activation: clean up responsibilities of prep.sock.act.env.()" context:-U11 generator/states-connect-socket-activation.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 752ee73bb62f..a214ffd5a6e4 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -94,22 +94,23 @@ prepare_socket_activation_environment (string_vector *env) string_vector_empty (env); return -1; } STATE_MACHINE { CONNECT_SA.START: enum state next; char *tmpdir; char *sockpath; int s; struct sockaddr_un addr; + struct execvpe execvpe_ctx; string_vector env; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); next = %.DEAD; /* Use /tmp instead of TMPDIR because we must ensure the path is * short enough to store in the sockaddr_un. On some platforms this @@ -141,25 +142,31 @@ CONNECT_SA.START: memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1); if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) { set_error (errno, "bind: %s", sockpath); goto close_socket; } if (listen (s, SOMAXCONN) == -1) { set_error (errno, "listen"); goto unlink_sockpath; } + if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0], h->argv.len) == + -1) { + set_error (errno, "nbd_internal_execvpe_init"); + goto unlink_sockpath; + } + if (prepare_socket_activation_environment (&env) == -1) /* prepare_socket_activation_environment() calls set_error() internally */ - goto unlink_sockpath; + goto uninit_execvpe; pid = fork (); if (pid == -1) { set_error (errno, "fork"); goto empty_env; } if (pid == 0) { /* child - run command */ if (s != FIRST_SOCKET_ACTIVATION_FD) { if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) { nbd_internal_fork_safe_perror ("dup2"); @@ -189,45 +196,47 @@ 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); /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } - environ = env.ptr; - execvp (h->argv.ptr[0], h->argv.ptr); + (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv, env.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } /* Parent -- we're done; commit. */ h->sact_tmpdir = tmpdir; h->sact_sockpath = sockpath; h->pid = pid; h->connaddrlen = sizeof addr; memcpy (&h->connaddr, &addr, h->connaddrlen); next = %^CONNECT.START; /* fall through, for releasing the temporaries */ empty_env: string_vector_empty (&env); +uninit_execvpe: + nbd_internal_execvpe_uninit (&execvpe_ctx); + unlink_sockpath: if (next == %.DEAD) unlink (sockpath); close_socket: close (s); free_sockpath: if (next == %.DEAD) free (sockpath);

A comment currently states that "*the* socket must be set to non-blocking only in the parent" (emphasis mine). This is a thinko: it implies that we have just one file descriptor, and that the underlying file description (which is manipulated by fcntl()'s F_GETFL and F_SETFL) is shared by the parent and the child processes. That's not the case: we have *two* sockets here (a socket pair), one of which is used by the parent exclusively, and the other is used by the child exclusively. Clarify the comment: say that we set the parent-side end of the socket pair to non-blocking. This clarification will play a role in a subsequent patch. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b context:-U5 generator/states-connect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index 503f3ebe8819..137350cc77e6 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -281,11 +281,11 @@ CONNECT_COMMAND.START: /* Parent. */ close (sv[1]); h->pid = pid; - /* The socket must be set to non-blocking only in the parent, + /* Only the parent-side end of the socket pair must be set to non-blocking, * because the child may not be expecting a non-blocking socket. */ flags = fcntl (sv[0], F_GETFL, 0); if (flags == -1 || fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {

Refer to fd#0 and fd#1 with the symbolic constants STDIN_FILENO and STDOUT_FILENO from <unistd.h>, for better readability. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b generator/states-connect.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index 2fd7429ae056..5612da53f59f 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -260,11 +260,11 @@ CONNECT_COMMAND.START: return 0; } if (pid == 0) { /* child - run command */ - close (0); - close (1); + close (STDIN_FILENO); + close (STDOUT_FILENO); close (sv[0]); - dup2 (sv[1], 0); - dup2 (sv[1], 1); + dup2 (sv[1], STDIN_FILENO); + dup2 (sv[1], STDOUT_FILENO); close (sv[1]); /* Restore SIGPIPE back to SIG_DFL. */

This patch implements the same ideas as those presented in the prior patch socket activation: centralize resource release of this same series, only for CONNECT_COMMAND.START. The one temporary resource we have here is sv[1]. Similarly to the above-cited patch, the present patch also exposes a resource leak on the error path. Namely, when any construction step fails in the parent after fork(), the child process is leaked. (The child process may well be killed and reaped once the application decides to call nbd_close(), but until then, after the CONNECT_COMMAND.START handler returns with failure, unusable (half-constructed) resources linger around indefinitely. A library API should either fully succeed or fully fail.) We are going to plug this leak in a subsequent patch. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b context:-W generator/states-connect.c | 52 +++++++++++--------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index 750f8b765578..0fe44f6d44d5 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -238,96 +238,104 @@ CONNECT_TCP.NEXT_ADDRESS: return 0; CONNECT_COMMAND.START: + enum state next; int sv[2]; pid_t pid; int flags; + struct socket *sock; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); + + next = %.DEAD; + if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); - return 0; + goto done; } /* A process is effectively in an unusable state if any of STDIN_FILENO * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they * exist however, then the socket pair created above will not intersect with * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic * below. */ assert (sv[0] > STDERR_FILENO); assert (sv[1] > STDERR_FILENO); pid = fork (); if (pid == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); - close (sv[0]); - close (sv[1]); - return 0; + goto close_socket_pair; } + if (pid == 0) { /* child - run command */ if (close (sv[0]) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } if (dup2 (sv[1], STDIN_FILENO) == -1 || dup2 (sv[1], STDOUT_FILENO) == -1) { nbd_internal_fork_safe_perror ("dup2"); _exit (126); } NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO); NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO); if (close (sv[1]) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } execvp (h->argv.ptr[0], h->argv.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } - /* Parent. */ - close (sv[1]); - - h->pid = pid; - - /* Only the parent-side end of the socket pair must be set to non-blocking, + /* Parent. + * + * Only the parent-side end of the socket pair must be set to non-blocking, * because the child may not be expecting a non-blocking socket. */ flags = fcntl (sv[0], F_GETFL, 0); if (flags == -1 || fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fcntl"); - close (sv[0]); - return 0; + goto close_socket_pair; } - h->sock = nbd_internal_socket_create (sv[0]); - if (!h->sock) { - SET_NEXT_STATE (%.DEAD); + sock = nbd_internal_socket_create (sv[0]); + if (!sock) /* nbd_internal_socket_create() calls set_error() internally */ - close (sv[0]); - return 0; - } + goto close_socket_pair; + + /* Commit. */ + h->pid = pid; + h->sock = sock; /* The sockets are connected already, we can jump directly to * receiving the server magic. */ - SET_NEXT_STATE (%^MAGIC.START); + next = %^MAGIC.START; + + /* fall through, for releasing the temporaries */ + +close_socket_pair: + if (next == %.DEAD) + close (sv[0]); + close (sv[1]); + +done: + SET_NEXT_STATE (next); return 0; } /* END STATE MACHINE */

In the future, we'd like to add systemd socket activation environment variables such that: - their values may not be constants (not even for pre-allocating space), - they may be optional, - regardless of some optional variables being added or not, the positions of those that we do add can be captured, for later reference. Generalize prepare_socket_activation_environment() accordingly. Write the child PID to "LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" with the new "capturing" interface. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b - resolve rebase conflicts (a) inside prepare_socket_activation_environment(), (b) near the prepare_socket_activation_environment() call site -- due to keeping set_error() internal to prepare_socket_activation_environment(), in patch "socket activation: clean up responsibilities of prep.sock.act.env.()" context:-U6 generator/states-connect-socket-activation.c | 160 +++++++++++++++----- 1 file changed, 125 insertions(+), 35 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index a214ffd5a6e4..10ccc3119299 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -27,85 +27,169 @@ #include <errno.h> #include <assert.h> #include <sys/socket.h> #include <sys/un.h> #include "internal.h" +#include "compiler-macros.h" +#include "unique-name.h" +#include "array-size.h" +#include "checked-overflow.h" /* 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 +/* Describes a systemd socket activation environment variable. */ +struct sact_var { + const char *prefix; /* variable name and equal sign */ + size_t prefix_len; + const char *value; + size_t value_len; +}; + +/* Determine the length of a string, using "sizeof" whenever possible. + * + * Do not use this macro on an argument that has side effects, as no guarantees + * are given regarding the number of times the argument may be evaluated. + * TYPE_IS_ARRAY(s) itself may contribute a different number of evaluations + * dependent on whether "s" has variably modified type, and then the conditional + * operator either evaluates "sizeof s" (which contributes 0 or 1 evaluations, + * dependent on whether "s" has variably modified type) or strlen(s) (which + * contributes 1 evaluation). Also note that the argument of the "sizeof" + * operator is *only* parenthesized because "s" is a macro parameter here. +*/ +#define STRLEN1(s) ((TYPE_IS_ARRAY (s) ? sizeof (s) - 1 : strlen (s))) + +/* Push a new element to an array of "sact_var" structures. + * + * "vars" is the array to extend. "num_vars" (of type (size_t *)) points to the + * number of elements that the array, on input, contains; (*num_vars) is + * increased by one on output. "prefix" and "value" serve as the values for + * setting the fields in the new element. "ofs" (of type (size_t *)) may be + * NULL; if it isn't, then on output, (*ofs) is set to the input value of + * (*num_vars): the offset of the just-pushed element. + * + * Avoid arguments with side-effects here as well. + */ +#define SACT_VAR_PUSH(vars, num_vars, prefix, value, ofs) \ + SACT_VAR_PUSH1 ((vars), (num_vars), (prefix), (value), (ofs), \ + NBDKIT_UNIQUE_NAME (_ofs)) +#define SACT_VAR_PUSH1(vars, num_vars, prefix, value, ofs, ofs1) \ + do { \ + size_t *ofs1; \ + \ + assert (*(num_vars) < ARRAY_SIZE (vars)); \ + ofs1 = (ofs); \ + if (ofs1 != NULL) \ + *ofs1 = *(num_vars); \ + (vars)[(*(num_vars))++] = (struct sact_var){ (prefix), STRLEN1 (prefix), \ + (value), STRLEN1 (value) }; \ + } while (0) 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". +/* Prepare environment for calling execvp when doing systemd socket activation. + * Takes the current environment and copies it. Removes any existing socket + * activation variables and replaces them with new ones. Variables in "sact_var" + * will be placed at the front of "env", preserving the order from "sact_var". */ static int -prepare_socket_activation_environment (string_vector *env) +prepare_socket_activation_environment (string_vector *env, + const struct sact_var *sact_var, + size_t num_vars) { - char *p; + const struct sact_var *var_end; + char *new_var; + const struct sact_var *var; size_t i; *env = (string_vector)empty_vector; - /* Reserve slots env[0] and env[1]. */ - p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); - if (p == NULL) - goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; - } - p = strdup ("LISTEN_FDS=1"); - if (p == NULL) - goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; + /* Set the exclusive limit for loops over "sact_var". */ + var_end = sact_var + num_vars; + + /* New environment variable being constructed for "env". */ + new_var = NULL; + + /* Copy "sact_var" to the front of "env". */ + for (var = sact_var; var < var_end; ++var) { + size_t new_var_size; + char *p; + + /* Calculate the size of the "NAME=value" string. */ + if (ADD_OVERFLOW (var->prefix_len, var->value_len, &new_var_size) || + ADD_OVERFLOW (new_var_size, 1u, &new_var_size)) { + errno = EOVERFLOW; + goto err; + } + + /* Allocate and format "NAME=value". */ + new_var = malloc (new_var_size); + if (new_var == NULL) + goto err; + p = new_var; + + memcpy (p, var->prefix, var->prefix_len); + p += var->prefix_len; + + memcpy (p, var->value, var->value_len); + p += var->value_len; + + *p++ = '\0'; + + /* Push "NAME=value" to the vector. */ + if (string_vector_append (env, new_var) == -1) + goto err; + /* Ownership transferred. */ + new_var = NULL; } - /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */ + /* Append the current environment to "env", but remove "sact_var". */ for (i = 0; environ[i] != NULL; ++i) { - if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 && - strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) { - char *copy = strdup (environ[i]); - if (copy == NULL) - goto err; - if (string_vector_append (env, copy) == -1) { - free (copy); - goto err; - } + for (var = sact_var; var < var_end; ++var) { + if (strncmp (environ[i], var->prefix, var->prefix_len) == 0) + break; } + /* Drop known socket activation variable from the current environment. */ + if (var < var_end) + continue; + + new_var = strdup (environ[i]); + if (new_var == NULL) + goto err; + + if (string_vector_append (env, new_var) == -1) + goto err; + /* Ownership transferred. */ + new_var = NULL; } /* The environ must be NULL-terminated. */ if (string_vector_append (env, NULL) == -1) goto err; return 0; err: set_error (errno, "malloc"); + free (new_var); string_vector_empty (env); return -1; } STATE_MACHINE { CONNECT_SA.START: enum state next; char *tmpdir; char *sockpath; int s; struct sockaddr_un addr; struct execvpe execvpe_ctx; + size_t num_vars; + struct sact_var sact_var[2]; + size_t pid_ofs; string_vector env; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); @@ -153,13 +237,18 @@ CONNECT_SA.START: if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0], h->argv.len) == -1) { set_error (errno, "nbd_internal_execvpe_init"); goto unlink_sockpath; } - if (prepare_socket_activation_environment (&env) == -1) + num_vars = 0; + SACT_VAR_PUSH (sact_var, &num_vars, + "LISTEN_PID=", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs); + SACT_VAR_PUSH (sact_var, &num_vars, + "LISTEN_FDS=", "1", NULL); + if (prepare_socket_activation_environment (&env, sact_var, num_vars) == -1) /* prepare_socket_activation_environment() calls set_error() internally */ goto uninit_execvpe; pid = fork (); if (pid == -1) { set_error (errno, "fork"); @@ -193,13 +282,14 @@ 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); + NBD_INTERNAL_FORK_SAFE_ASSERT (strlen (v) <= sact_var[pid_ofs].value_len); + strcpy (env.ptr[pid_ofs] + sact_var[pid_ofs].prefix_len, v); /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); }

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; [Laszlo's note: "ascii-ctype.h" and "test-ascii-ctype.c" match those of nbdkit at commit 45b72f5bd8fc. Originally posted by Rich at <https://listman.redhat.com/archives/libguestfs/2023-January/030559.html>.] Message-Id: <20230130225521.1771496-3-rjones(a)redhat.com&gt; Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b - refresh patch and commit message against nbdkit 45b72f5bd8fc common/include/ascii-ctype.h | 75 +++++++++++++++++ common/include/Makefile.am | 6 ++ common/include/test-ascii-ctype.c | 88 ++++++++++++++++++++ .gitignore | 1 + 4 files changed, 170 insertions(+) diff --git a/common/include/ascii-ctype.h b/common/include/ascii-ctype.h new file mode 100644 index 000000000000..191a0f27e19d --- /dev/null +++ b/common/include/ascii-ctype.h @@ -0,0 +1,75 @@ +/* nbdkit + * Copyright Red Hat + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +/* Normal ctype functions are affected by the current locale. For + * example isupper() might recognize Ä in some but not all locales. + * These functions match only 7 bit ASCII characters. + */ + +#ifndef NBDKIT_ASCII_CTYPE_H +#define NBDKIT_ASCII_CTYPE_H + +#define ascii_isalnum(c) (ascii_isalpha (c) || ascii_isdigit (c)) + +#define ascii_isalpha(c) \ + (((c) >= 'a' && (c) <= 'z') || ((c) >= 'A' && (c) <= 'Z')) + +#define ascii_isdigit(c) \ + ((c) >= '0' && (c) <= '9') + +#define ascii_isspace(c) \ + ((c) == '\t' || (c) == '\n' || (c) == '\f' || (c) == '\r' || (c) == ' ') + +#define ascii_isupper(c) \ + ((c) >= 'A' && (c) <= 'Z') + +#define ascii_islower(c) \ + ((c) >= 'a' && (c) <= 'z') + +/* See also hexdigit.h */ +#define ascii_isxdigit(c) \ + ((c) == '0' || (c) == '1' || (c) == '2' || (c) == '3' || (c) == '4' || \ + (c) == '5' || (c) == '6' || (c) == '7' || (c) == '8' || (c) == '9' || \ + (c) == 'a' || (c) == 'b' || (c) == 'c' || \ + (c) == 'd' || (c) == 'e' || (c) == 'f' || \ + (c) == 'A' || (c) == 'B' || (c) == 'C' || \ + (c) == 'D' || (c) == 'E' || (c) == 'F') + +#define ascii_tolower(c) \ + (ascii_isupper ((c)) ? (c) + 32 : (c)) + +#define ascii_toupper(c) \ + (ascii_islower ((c)) ? (c) - 32 : (c)) + +#define ascii_isprint(c) ((c) >= 32 && (c) <= 126) + +#endif /* NBDKIT_ASCII_CTYPE_H */ diff --git a/common/include/Makefile.am b/common/include/Makefile.am index 8c99f76b8e73..1acc9fa41311 100644 --- a/common/include/Makefile.am +++ b/common/include/Makefile.am @@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk EXTRA_DIST = \ ansi-colours.h \ array-size.h \ + ascii-ctype.h \ byte-swapping.h \ checked-overflow.h \ compiler-macros.h \ @@ -36,6 +37,7 @@ EXTRA_DIST = \ TESTS = \ test-array-size \ + test-ascii-ctype \ test-byte-swapping \ test-checked-overflow \ test-isaligned \ @@ -49,6 +51,10 @@ test_array_size_SOURCES = test-array-size.c array-size.h test_array_size_CPPFLAGS = -I$(srcdir) test_array_size_CFLAGS = $(WARNINGS_CFLAGS) +test_ascii_ctype_SOURCES = test-ascii-ctype.c ascii-ctype.h +test_ascii_ctype_CPPFLAGS = -I$(srcdir) +test_ascii_ctype_CFLAGS = $(WARNINGS_CFLAGS) + test_byte_swapping_SOURCES = test-byte-swapping.c byte-swapping.h test_byte_swapping_CPPFLAGS = -I$(srcdir) test_byte_swapping_CFLAGS = $(WARNINGS_CFLAGS) diff --git a/common/include/test-ascii-ctype.c b/common/include/test-ascii-ctype.c new file mode 100644 index 000000000000..aefe1cc63640 --- /dev/null +++ b/common/include/test-ascii-ctype.c @@ -0,0 +1,88 @@ +/* nbdkit + * Copyright Red Hat + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */ +#include <assert.h> + +#include "ascii-ctype.h" + +int +main (void) +{ + assert (ascii_isspace (' ')); + assert (ascii_isspace ('\t')); + assert (ascii_isspace ('\n')); + assert (! ascii_isspace ('a')); + + assert (ascii_isalpha ('a')); + assert (ascii_isalpha ('Z')); + assert (ascii_isalpha ('z')); + assert (! ascii_isalpha (' ')); + assert (! ascii_isalpha ('0')); + { const char *s = "Ä"; assert (! ascii_isalpha (s[0])); } + { const char *s = "®"; assert (! ascii_isalpha (s[0])); } + + assert (ascii_isdigit ('0')); + assert (ascii_isdigit ('9')); + { const char *s = "Ø"; assert (! ascii_isdigit (s[0])); } /* U+00D8 */ + { const char *s = "９"; assert (! ascii_isdigit (s[0])); } /* U+FF19 */ + + assert (ascii_islower ('a')); + assert (ascii_islower ('z')); + assert (! ascii_islower ('Z')); + { const char *s = "Ä"; assert (! ascii_islower (s[0])); } + + assert (ascii_isupper ('A')); + assert (ascii_isupper ('Z')); + assert (! ascii_isupper ('z')); + { const char *s = "Ä"; assert (! ascii_isupper (s[0])); } + + assert (ascii_tolower ('A') == 'a'); + assert (ascii_tolower ('Z') == 'z'); + assert (ascii_tolower ('a') == 'a'); + assert (ascii_tolower ('z') == 'z'); + assert (ascii_tolower ('0') == '0'); + { const char *s = "Ä"; assert (ascii_tolower (s[0]) == s[0]); } + + assert (ascii_toupper ('a') == 'A'); + assert (ascii_toupper ('z') == 'Z'); + assert (ascii_toupper ('A') == 'A'); + assert (ascii_toupper ('Z') == 'Z'); + assert (ascii_toupper ('0') == '0'); + { const char *s = "à"; assert (ascii_toupper (s[0]) == s[0]); } + + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index 9df5d9f9db4c..df4b6ca249c6 100644 --- a/.gitignore +++ b/.gitignore @@ -34,6 +34,7 @@ Makefile.in /bash-completion/nbdinfo /bash-completion/nbdublk /common/include/test-array-size +/common/include/test-ascii-ctype /common/include/test-byte-swapping /common/include/test-checked-overflow /common/include/test-isaligned

On Thu, Mar 23, 2023 at 01:10:15PM +0100, Laszlo Ersek wrote: I recommend adding permitted_states = [ Created ]; on this one - it makes no sense to change the socket name after connection. qemu-storage-daemon doesn't do it yet, but there's a strong likelihood of it moving that direction, so I don't mind leaving this statement as-is describing what we hope is the future state of things. Unlike set_ which only makes sense in Created, get_ makes sense over the entire life of the handle, so no permitted_state entry needed here. My suggested one-line addition is easy enough to add that I'm fine if you do that without needing a v5. Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

When the user calls nbd_set_socket_activation_name before calling nbd_connect_system_socket_activation, pass the name down to the server through LISTEN_FDNAMES. This has no effect unless the new API has been called to set the socket name to a non-empty string. https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html [Original commit message and upstream discussion reference by Rich Jones; at <https://listman.redhat.com/archives/libguestfs/2023-January/030558.html> / msgid <20230130225521.1771496-5-rjones(a)redhat.com&gt;.] Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; --- Notes: v4: - pick up Rich's R-b - resolve rebase conflict near the prepare_socket_activation_environment() call site, due to keeping set_error() internal to that callee, in patch "socket activation: clean up responsibilities of prep.sock.act.env.()" generator/states-connect-socket-activation.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 10ccc3119299..f9dbd433ce00 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -185,7 +185,7 @@ CONNECT_SA.START: struct sockaddr_un addr; struct execvpe execvpe_ctx; size_t num_vars; - struct sact_var sact_var[2]; + struct sact_var sact_var[3]; size_t pid_ofs; string_vector env; pid_t pid; @@ -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) /* prepare_socket_activation_environment() calls set_error() internally */ goto uninit_execvpe;

On 3/23/23 20:27, Eric Blake wrote: 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.) 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: I've also checked the implementation of sd_listen_fds_with_names(): https://github.com/systemd/systemd/blob/main/src/libsystemd/sd-daemon/sd-... Let me quote the function: Based on those: (1) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and don't pass LISTEN_FDNAMES at all, then sd_listen_fds_with_names() will function in the nbd server properly, and will return a "names" array with a single non-NULL element "unknown", and a terminating null pointer. The "unknown" value is specified behavior that socket-activated services can rely upon. (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. (3) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in this patch to pass LISTEN_FDNAMES="" in case "h->sact_name" is NULL, then sd_listen_fds_with_names() will succeed in the nbd server, but the returned single name ("") will most likely confuse it. (4) If we pass LISTEN_PID=xxx and LISTEN_FDS=1, and rework the logic in this patch to pass LISTEN_FDNAMES="unknown" in case "h->sact_name" is NULL, then sd_listen_fds_with_names() will succeed in the nbd server, and the single returned name ("unknown") will merge into case (1), i.e., as if we had not passed (or had removed) LISTEN_FDNAMES in the environment. Therefore I propose that we implement (4). We're already populating the LISTEN_* variables ourselves (that is, not relying on systemd library or daemon logic to fill them in). I see nothing wrong with setting LISTEN_FDNAMES="unknown" ourselves; again that value is publicly specified behavior. Case (4) also appears consistent with repeated calls to sd_listen_fds_with_names(). If "unset_environment" is set to nonzero in one of those calls, then further calls will see the internal sd_listen_fds() call return 0, and behave as expected. Whereas if "unset_environment" is never set to zero in those repeated calls, then "unknown" will continue to be returned from LISTEN_FDNAMES (as if via case (1)). Under case (4), we should also update the API documentation in the previous patch ("generator: Add APIs to get/set the socket activation socket name"): we can say there, 'then the name "unknown" will be seen by the server'. Thanks for the careful review! Laszlo

On 3/24/23 15:52, Eric Blake wrote: I wonder if this is what the "unset_environment" parameter of sd_listen_fds() and sd_listen_fds_with_names() exists for. If any socket-activated application (such as a libnbd client application) is *required* to call one of these functions at least once with a nonzero "unset_environment" argument (reasonably early after startup), then that eliminates the problem. For example (specifically), by the time we got to prepare_socket_activation_environment(), we'd find (or leak!) no inherited LISTEN_* variable (so the "conflict handling" logic could be removed altogether from prepare_socket_activation_environment()) . The above-linked spec <https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html> writes, "If the unset_environment parameter is non-zero [...] the variables are no longer inherited by child processes." So, if a libnbd client application (a) is socket activated, and (b) plans on calling nbd_connect_systemd_socket_activation() itself, then this application *MAY* already be responsible for passing a nonzero "unset_environment" argument, when it calls sd_listen_fds() or sd_listen_fds_with_names() early on. I'm emphasizing "may", because the spec does not explain what use case "unset_environment" is specifically meant for. Laszlo