Re: [Libguestfs] [libnbd PATCH v3 15/29] socket activation: centralize resource release

On Wed, Feb 15, 2023 at 03:11:44PM +0100, Laszlo Ersek wrote: > 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. > > 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; > --- > > Notes: > context:-W > > generator/states-connect-socket-activation.c | 82 ++++++++++++-------- > 1 file changed, 48 insertions(+), 34 deletions(-) > > diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c > index 766f46177bf3..164b8e6828f2 100644 > --- a/generator/states-connect-socket-activation.c > +++ b/generator/states-connect-socket-activation.c > @@ -96,132 +96,146 @@ 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); > set_error (errno, "prepare_socket_activation_environment"); > - 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 */ A note 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.

Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Rich.