7:10 a.m.
This patch implements the same ideas as those presented in the prior patch
socket activation: centralize resource release
of this same series, only for CONNECT_COMMAND.START.
The one temporary resource we have here is sv[1].
Similarly to the above-cited patch, the present patch also exposes a
resource leak on the error path. Namely, when any construction step fails
in the parent after fork(), the child process is leaked. (The child
process may well be killed and reaped once the application decides to call
nbd_close(), but until then, after the CONNECT_COMMAND.START handler
returns with failure, unusable (half-constructed) resources linger around
indefinitely. A library API should either fully succeed or fully fail.) We
are going to plug this leak in a subsequent patch.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
---
Notes:
v4:
- pick up Rich's R-b
context:-W
generator/states-connect.c | 52 +++++++++++---------
1 file changed, 30 insertions(+), 22 deletions(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index 750f8b765578..0fe44f6d44d5 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -238,96 +238,104 @@ CONNECT_TCP.NEXT_ADDRESS:
return 0;
CONNECT_COMMAND.START:
+ enum state next;
int sv[2];
pid_t pid;
int flags;
+ struct socket *sock;
assert (!h->sock);
assert (h->argv.ptr);
assert (h->argv.ptr[0]);
+
+ next = %.DEAD;
+
if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "socketpair");
- return 0;
+ goto done;
}
/* A process is effectively in an unusable state if any of STDIN_FILENO
* (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they
* exist however, then the socket pair created above will not intersect with
* the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic
* below.
*/
assert (sv[0] > STDERR_FILENO);
assert (sv[1] > STDERR_FILENO);
pid = fork ();
if (pid == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "fork");
- close (sv[0]);
- close (sv[1]);
- return 0;
+ goto close_socket_pair;
}
+
if (pid == 0) { /* child - run command */
if (close (sv[0]) == -1) {
nbd_internal_fork_safe_perror ("close");
_exit (126);
}
if (dup2 (sv[1], STDIN_FILENO) == -1 ||
dup2 (sv[1], STDOUT_FILENO) == -1) {
nbd_internal_fork_safe_perror ("dup2");
_exit (126);
}
NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
if (close (sv[1]) == -1) {
nbd_internal_fork_safe_perror ("close");
_exit (126);
}
/* Restore SIGPIPE back to SIG_DFL. */
if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) {
nbd_internal_fork_safe_perror ("signal");
_exit (126);
}
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 (sv[1]);
-
- h->pid = pid;
-
- /* Only the parent-side end of the socket pair must be set to non-blocking,
+ /* Parent.
+ *
+ * Only the parent-side end of the socket pair must be set to non-blocking,
* because the child may not be expecting a non-blocking socket.
*/
flags = fcntl (sv[0], F_GETFL, 0);
if (flags == -1 ||
fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) {
- SET_NEXT_STATE (%.DEAD);
set_error (errno, "fcntl");
- close (sv[0]);
- return 0;
+ goto close_socket_pair;
}
- h->sock = nbd_internal_socket_create (sv[0]);
- if (!h->sock) {
- SET_NEXT_STATE (%.DEAD);
+ sock = nbd_internal_socket_create (sv[0]);
+ if (!sock)
/* nbd_internal_socket_create() calls set_error() internally */
- close (sv[0]);
- return 0;
- }
+ goto close_socket_pair;
+
+ /* Commit. */
+ h->pid = pid;
+ h->sock = sock;
/* The sockets are connected already, we can jump directly to
* receiving the server magic.
*/
- SET_NEXT_STATE (%^MAGIC.START);
+ next = %^MAGIC.START;
+
+ /* fall through, for releasing the temporaries */
+
+close_socket_pair:
+ if (next == %.DEAD)
+ close (sv[0]);
+ close (sv[1]);
+
+done:
+ SET_NEXT_STATE (next);
return 0;
} /* END STATE MACHINE */