From d8730f8cec648952ba22935d73c2484fc88c9424 Mon Sep 17 00:00:00 2001
From: Peter Maydell <peter.maydell@linaro.org>
Date: Tue, 6 Nov 2018 15:13:23 +0000
Subject: slirp: fork_exec(): create and connect child socket before fork()

Currently fork_exec() fork()s, and then creates and connects the
child socket which it uses for communication with the parent in
the child process. This is awkward because the child has no
mechanism to report failure back to the parent, which might end
up blocked forever in accept(). The child code also has an issue
pointed out by Coverity (CID 1005727), where if the qemu_socket()
call fails it will pass -1 as a file descriptor to connect().

Fix these issues by moving the creation of the child's end of
the socket to before the fork(), where we are in a position to
handle a possible failure.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 misc.c | 55 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/misc.c b/misc.c
index 0edb993..92a7bb4 100644
--- a/misc.c
+++ b/misc.c
@@ -80,9 +80,10 @@ int fork_exec(struct socket *so, const char *ex, int do_pty)
  */
 int fork_exec(struct socket *so, const char *ex, int do_pty)
 {
-    int s;
-    struct sockaddr_in addr;
+    int s, cs;
+    struct sockaddr_in addr, csaddr;
     socklen_t addrlen = sizeof(addr);
+    socklen_t csaddrlen = sizeof(csaddr);
     int opt;
     const char *argv[256];
     /* don't want to clobber the original */
@@ -115,10 +116,35 @@ int fork_exec(struct socket *so, const char *ex, int do_pty)
         }
     }
 
+    if (getsockname(s, (struct sockaddr *)&csaddr, &csaddrlen) < 0) {
+        closesocket(s);
+        return 0;
+    }
+    cs = qemu_socket(AF_INET, SOCK_STREAM, 0);
+    if (cs < 0) {
+        closesocket(s);
+        return 0;
+    }
+    csaddr.sin_addr = loopback_addr;
+    /*
+     * This connect won't block because we've already listen()ed on
+     * the server end (even though we won't accept() the connection
+     * until later on).
+     */
+    do {
+        ret = connect(cs, (struct sockaddr *)&csaddr, csaddrlen);
+    } while (ret < 0 && errno == EINTR);
+    if (ret < 0) {
+        closesocket(s);
+        closesocket(cs);
+        return 0;
+    }
+
     pid = fork();
     switch (pid) {
     case -1:
         error_report("Error: fork failed: %s", strerror(errno));
+        closesocket(cs);
         close(s);
         return 0;
 
@@ -126,21 +152,10 @@ int fork_exec(struct socket *so, const char *ex, int do_pty)
         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);
+        dup2(cs, 0);
+        dup2(cs, 1);
+        dup2(cs, 2);
         for (s = getdtablesize() - 1; s >= 3; s--)
             close(s);
 
@@ -175,12 +190,10 @@ int fork_exec(struct socket *so, const char *ex, int do_pty)
 
     default:
         qemu_add_child_watch(pid);
+        closesocket(cs);
         /*
-         * 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
+         * This should never block, because we already connect()ed
+         * on the child end before we forked.
          */
         do {
             so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
-- 
cgit v1.1