Libguestfs - Feb 2023 - [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 at redhat.com>
---

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

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 at redhat.com>
> ---
> 
> 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 at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit