On 2/15/23 17:53, Richard W.M. Jones wrote:
> 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.
Ah, thank you for pointing that out! I'll capture this in the commit
message.
Laszlo
> Reviewed-by: Richard W.M. Jones <rjo...@redhat.com>
>
> Rich.
>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
