8:11 a.m.
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>
---
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 */