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 <ler...@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 <rjo...@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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
