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

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 */