diff mbox series

[for-3.2,10/13] slirp: improve subprocess socket creation

Message ID 20181110134548.14741-11-marcandre.lureau@redhat.com
State New
Headers show
Series slirp: cleanups | expand

Commit Message

Marc-André Lureau Nov. 10, 2018, 1:45 p.m. UTC
Fix a bunch of error handling issues by creating the "TCP socketpair"
from the parent process. (the code is currently disabled on Windows:
at first I thought a socketpair() could be used, but I realized that
slirp calls MSG_OOB on the socket, which isn't implemented on Linux)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/misc.c | 133 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 72 insertions(+), 61 deletions(-)

Comments

Samuel Thibault Nov. 10, 2018, 1:53 p.m. UTC | #1
Marc-André Lureau, le sam. 10 nov. 2018 17:45:45 +0400, a ecrit:
> @@ -163,23 +183,14 @@ fork_exec(struct socket *so, const char *ex)
>  		exit(1);
>  
>  	 default:
> -		qemu_add_child_watch(pid);
> -                /*
> -                 * XXX this could block us...
> -                 * XXX Should set a timer here, and if accept() doesn't
> -                 * return after X seconds, declare it a failure
> -                 * The only reason this will block forever is if socket()
> -                 * of connect() fail in the child process
> -                 */

At least this got fixed by Peter lately, so please rebase.

Samuel
Peter Maydell Nov. 10, 2018, 2:04 p.m. UTC | #2
On 10 November 2018 at 13:53, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Marc-André Lureau, le sam. 10 nov. 2018 17:45:45 +0400, a ecrit:
>> @@ -163,23 +183,14 @@ fork_exec(struct socket *so, const char *ex)
>>               exit(1);
>>
>>        default:
>> -             qemu_add_child_watch(pid);
>> -                /*
>> -                 * XXX this could block us...
>> -                 * XXX Should set a timer here, and if accept() doesn't
>> -                 * return after X seconds, declare it a failure
>> -                 * The only reason this will block forever is if socket()
>> -                 * of connect() fail in the child process
>> -                 */
>
> At least this got fixed by Peter lately, so please rebase.

My patches aren't in master yet, though -- do you have a
public slirp tree that slirp patches should be based on?
(I'm assuming you're planning a pull request too.)

thanks
-- PMM
Samuel Thibault Nov. 10, 2018, 2:07 p.m. UTC | #3
Peter Maydell, le sam. 10 nov. 2018 14:04:10 +0000, a ecrit:
> On 10 November 2018 at 13:53, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> > Marc-André Lureau, le sam. 10 nov. 2018 17:45:45 +0400, a ecrit:
> >> @@ -163,23 +183,14 @@ fork_exec(struct socket *so, const char *ex)
> >>               exit(1);
> >>
> >>        default:
> >> -             qemu_add_child_watch(pid);
> >> -                /*
> >> -                 * XXX this could block us...
> >> -                 * XXX Should set a timer here, and if accept() doesn't
> >> -                 * return after X seconds, declare it a failure
> >> -                 * The only reason this will block forever is if socket()
> >> -                 * of connect() fail in the child process
> >> -                 */
> >
> > At least this got fixed by Peter lately, so please rebase.
> 
> My patches aren't in master yet, though -- do you have a
> public slirp tree that slirp patches should be based on?
> (I'm assuming you're planning a pull request too.)

Ah, right.
My tree is documented in MAINTAINERS :)
git https://people.debian.org/~sthibault/qemu.git slirp

Let me push that to master indeed.

Samuel
diff mbox series

Patch

diff --git a/slirp/misc.c b/slirp/misc.c
index 54edc0b0b9..d9804f2a6d 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -73,42 +73,75 @@  fork_exec(struct socket *so, const char *ex)
 
 #else
 
-/*
- * XXX This is ugly
- * We create and bind a socket, then fork off to another
- * process, which connects to this socket, after which we
- * exec the wanted program.  If something (strange) happens,
- * the accept() call could block us forever.
- */
+static int
+slirp_socketpair_with_oob(int sv[2])
+{
+    struct sockaddr_in addr = {
+        .sin_family = AF_INET,
+        .sin_port = 0,
+        .sin_addr.s_addr = INADDR_ANY,
+    };
+    socklen_t addrlen = sizeof(addr);
+    int ret, s;
+
+    sv[1] = -1;
+    s = qemu_socket(AF_INET, SOCK_STREAM, 0);
+    if (s < 0 || bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
+        listen(s, 1) < 0 ||
+        getsockname(s, (struct sockaddr *)&addr, &addrlen) < 0) {
+        goto err;
+    }
+
+    sv[1] = qemu_socket(AF_INET, SOCK_STREAM, 0);
+    if (sv[1] < 0) {
+        goto err;
+    }
+
+    qemu_set_nonblock(sv[1]);
+    do {
+        ret = connect(sv[1], (struct sockaddr *)&addr, addrlen);
+    } while (ret < 0 && errno == EINTR);
+    if (ret == -1 && errno != EINPROGRESS) {
+        goto err;
+    }
+    qemu_set_block(sv[1]);
+
+    do {
+        sv[0] = accept(s, (struct sockaddr *)&addr, &addrlen);
+    } while (sv[0] < 0 && errno == EINTR);
+    if (sv[0] < 0) {
+        goto err;
+    }
+
+    closesocket(s);
+    return 0;
+
+err:
+    error_report("Error: slirp_socketpair(): %s", strerror(errno));
+    if (s >= 0) {
+        closesocket(s);
+    }
+    if (sv[1] >= 0) {
+        closesocket(sv[1]);
+    }
+    return -1;
+}
+
 int
 fork_exec(struct socket *so, const char *ex)
 {
-	int s;
-	struct sockaddr_in addr;
-	socklen_t addrlen = sizeof(addr);
-	int opt;
 	const char *argv[256];
 	/* don't want to clobber the original */
 	char *bptr;
 	const char *curarg;
-	int c, i, ret;
+	int opt, c, i, sp[2];
 	pid_t pid;
 
 	DEBUG_CALL("fork_exec");
 	DEBUG_ARG("so = %p", so);
 	DEBUG_ARG("ex = %p", ex);
 
-    addr.sin_family = AF_INET;
-    addr.sin_port = 0;
-    addr.sin_addr.s_addr = INADDR_ANY;
-
-    s = qemu_socket(AF_INET, SOCK_STREAM, 0);
-    if (s < 0 || bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
-        listen(s, 1) < 0) {
-        error_report("Error: inet socket: %s", strerror(errno));
-        if (s >= 0) {
-            closesocket(s);
-        }
+    if (slirp_socketpair_with_oob(sp) < 0) {
         return 0;
     }
 
@@ -116,30 +149,17 @@  fork_exec(struct socket *so, const char *ex)
 	switch(pid) {
 	 case -1:
 		error_report("Error: fork failed: %s", strerror(errno));
-		close(s);
+		closesocket(sp[0]);
+		closesocket(sp[1]);
 		return 0;
 
 	 case 0:
-                setsid();
-
-		/* Set the DISPLAY */
-                getsockname(s, (struct sockaddr *)&addr, &addrlen);
-                close(s);
-                /*
-                 * Connect to the socket
-                 * XXX If any of these fail, we're in trouble!
-                 */
-                s = qemu_socket(AF_INET, SOCK_STREAM, 0);
-                addr.sin_addr = loopback_addr;
-                do {
-                    ret = connect(s, (struct sockaddr *)&addr, addrlen);
-                } while (ret < 0 && errno == EINTR);
-
-		dup2(s, 0);
-		dup2(s, 1);
-		dup2(s, 2);
-		for (s = getdtablesize() - 1; s >= 3; s--)
-		   close(s);
+         setsid();
+		dup2(sp[1], 0);
+		dup2(sp[1], 1);
+		dup2(sp[1], 2);
+		for (c = getdtablesize() - 1; c >= 3; c--)
+		   close(c);
 
 		i = 0;
 		bptr = g_strdup(ex); /* No need to free() this */
@@ -163,23 +183,14 @@  fork_exec(struct socket *so, const char *ex)
 		exit(1);
 
 	 default:
-		qemu_add_child_watch(pid);
-                /*
-                 * XXX this could block us...
-                 * XXX Should set a timer here, and if accept() doesn't
-                 * return after X seconds, declare it a failure
-                 * The only reason this will block forever is if socket()
-                 * of connect() fail in the child process
-                 */
-                do {
-                    so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
-                } while (so->s < 0 && errno == EINTR);
-                closesocket(s);
-                socket_set_fast_reuse(so->s);
-                opt = 1;
-                qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
-		qemu_set_nonblock(so->s);
-		return 1;
+         so->s = sp[0];
+         closesocket(sp[1]);
+         qemu_add_child_watch(pid);
+         socket_set_fast_reuse(so->s);
+         opt = 1;
+         qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
+         qemu_set_nonblock(so->s);
+         return 1;
 	}
 }
 #endif