Libguestfs - Mar 2023 - [libnbd PATCH v3 05/19] 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.

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 at redhat.com>
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
---

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 Thu, Mar 23, 2023 at 01:10:02PM +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.
Yes, this style has gronw on me as I've seen it in more places.
However,
> --- 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;
I've also come to appreciate __attribute__((cleanup)) handling for
even less effort.  If we decide to cross-port that from nbdkit (which
effectively limits libnbd to being build only by gcc and clang), we
could mark these two variables as cleanup and initialized to NULL,
then have even less code below because they get auto-freed on all
error paths where they were set.

I'm _not_ asking you do to that in this series (no need to respin and
fight even more rebase churn), so much as a question to Rich on
whether we think limiting libnbd to the same platforms as where nbdkit
currently compiles, instead of theoretically allowing libnbd to
compile in a wider set of compilers (although we haven't actually
tested that), is worth doing.
>  
> -  /* Parent. */
> -  close (s);
> -  string_vector_empty (&env);
> +  /* Parent -- we're done; commit. */
> +  h->sact_tmpdir = tmpdir;
> +  h->sact_sockpath = sockpath;
>    h->pid = pid;
If we do a followup series to introduce cleanup attributes, the
initial declaration of tmpdir and sockpath must be also initialized to
NULL; then here, where we transfer the values to permanent storage, we
must assign the temporaries back to NULL.  Going one step further,
note that libvirt has copied ideas from the glib project of having
macros that make it obvious when you are doing transfer semantics -
changing ownership of a string from one variable to another by setting
the original to NULL, all in one line of code.
>  
>    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);
With transfer semantics, you can unconditionally call free (sockpath)
here (without probing 'next') provided that 'sockpath' is
initialized
to NULL, and then re-set back to NULL upon the commit portion
transferring the pointer to the permanent storage.  What's more, if
you combine transfer semantics with attribute cleanup, the 'free
(sockpath)' is automatic on all exit paths without needing this goto
label or code.
> +
> +rmdir_tmpdir:
> +  if (next == %.DEAD)
> +    rmdir (tmpdir);
With transfer semantics, this code would still have to be conditional,
but the condition would change to 'if (tmpdir)' instead of depending
on 'next'.
> +
> +free_tmpdir:
> +  if (next == %.DEAD)
> +    free (tmpdir);
And with transfer semantics and attribute cleanup, this is another
label we could elide.
> +
> +done:
> +  SET_NEXT_STATE (next);
>    return 0;
>  } /* END STATE MACHINE */
At any rate, while there are still more ways to rewrite this even more
compactly (if we decide we want to pull in attribute cleanup), your
refactor is fine for now.

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org