slirp updates

Peter Maydell (4):
   slirp: Don't pass possibly -1 fd to send()
   slirp: Use g_new() to allocate sockets in socreate()
   slirp: Remove code that handles socreate() failure
   slirp: fork_exec(): create and connect child socket before fork()
 -----BEGIN PGP SIGNATURE-----
 
 iQIyBAABCgAdFiEEM/p7ZGGVAfjOnI+X4/ZanpVg20wFAlvm5nUACgkQ4/ZanpVg
 20xlIg/4htdxJYMx8NbZeZhNQAFZwck0BMXqy7ZhTC9NMCupp1GRm0fmLObxXX4G
 2RwYfc0eGSObjQQ+bWE9ba1R0PEVciHqKZejPctsHLB0P0NaPwOBLC5KC3X7YX+n
 3vqa04R9HkXBSHmQAuP53qb5BCGtBTEp8yau88gUKGEJWdiXf2sWKGax0oivWLCY
 ffDoqPInOK+PoLrK/P+W5BuXNcjkibfXdMAKjUFvMO5JSdDMjsBXf/IwyddEU1Il
 uECqW0VxmB0O3B2gdWhpWl2RRuBzr9P1AQ81GLPSSoPQqSSkIJMwHxDelUbioTE9
 I19uqclUCOKWhyiLMqmhfcOxvRGGO3L87G+q0G8EqkKAMX+LrEfcjoYwYyQvuiyJ
 TGTAI566jprbmiOP2HHGS1davDoh4QrySi0HpOlr3LlHjHT6ZhZ576k7zOpzM9pv
 +eSnG9Y54wS9bsuAOvI3emyDIhRmyRvvXEV9WOZe2QxOvq9HaR80cNA+e8zGaFn+
 fdAW+bWX8IdxRxX+r9XPlXuJeAXY4tBCOxcpS2VoslZSxEJIU/e1KmSZQqIkR0yh
 BjIK/j93BXkSa/hA0+DDSrkdFVNpx6ImsitTU9D1WxJA87+vj6NrVWiEX4p/6CJe
 MIhy0fdl9maopG22xNZ8X3VLCsomDG52ZdApFiJ3ZUeZ0smP9w==
 =+5+Q
 -----END PGP SIGNATURE-----

Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into staging

slirp updates

Peter Maydell (4):
  slirp: Don't pass possibly -1 fd to send()
  slirp: Use g_new() to allocate sockets in socreate()
  slirp: Remove code that handles socreate() failure
  slirp: fork_exec(): create and connect child socket before fork()

# gpg: Signature made Sat 10 Nov 2018 14:08:53 GMT
# gpg:                using RSA key E3F65A9E9560DB4C
# gpg: Good signature from "Samuel Thibault <samuel.thibault@aquilenet.fr>"
# gpg:                 aka "Samuel Thibault <sthibault@debian.org>"
# gpg:                 aka "Samuel Thibault <samuel.thibault@gnu.org>"
# gpg:                 aka "Samuel Thibault <samuel.thibault@inria.fr>"
# gpg:                 aka "Samuel Thibault <samuel.thibault@labri.fr>"
# gpg:                 aka "Samuel Thibault <samuel.thibault@ens-lyon.org>"
# gpg:                 aka "Samuel Thibault <samuel.thibault@u-bordeaux.fr>"
# gpg: WARNING: This key is not certified with sufficiently trusted signatures!
# gpg:          It is not certain that the signature belongs to the owner.
# Primary key fingerprint: 900C B024 B679 31D4 0F82  304B D017 8C76 7D06 9EE6
#      Subkey fingerprint: 33FA 7B64 6195 01F8 CE9C  8F97 E3F6 5A9E 9560 DB4C

* remotes/thibault/tags/samuel-thibault:
  slirp: fork_exec(): create and connect child socket before fork()
  slirp: Remove code that handles socreate() failure
  slirp: Use g_new() to allocate sockets in socreate()
  slirp: Don't pass possibly -1 fd to send()

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

slirp/ip_icmp.c

@@ -160,7 +160,7 @@ icmp_input(struct mbuf *m, int hlen)
     } else {
       struct socket *so;
       struct sockaddr_storage addr;
-      if ((so = socreate(slirp)) == NULL) goto freeit;
+      so = socreate(slirp);
       if (icmp_send(so, m, hlen) == 0) {
         return;
       }

slirp/misc.c

@@ -85,9 +85,10 @@ 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;
+        int s, cs;
-	struct sockaddr_in addr;
+        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 */
@@ -120,10 +121,35 @@ 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;
 
@@ -131,21 +157,10 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                 setsid();
 
 		/* Set the DISPLAY */
-                getsockname(s, (struct sockaddr *)&addr, &addrlen);
                 close(s);
-                /*
+                dup2(cs, 0);
-                 * Connect to the socket
+                dup2(cs, 1);
-                 * XXX If any of these fail, we're in trouble!
+                dup2(cs, 2);
-                 */
-                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);
 
@@ -178,12 +193,10 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 
 	 default:
 		qemu_add_child_watch(pid);
+                closesocket(cs);
                 /*
-                 * XXX this could block us...
+                 * This should never block, because we already connect()ed
-                 * XXX Should set a timer here, and if accept() doesn't
+                 * on the child end before we forked.
-                 * 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);

slirp/slirp.c

@@ -1091,6 +1091,17 @@ ssize_t slirp_send(struct socket *so, const void *buf, size_t len, int flags)
         return len;
     }
 
+    if (so->s == -1) {
+        /*
+         * This should in theory not happen but it is hard to be
+         * sure because some code paths will end up with so->s == -1
+         * on a failure but don't dispose of the struct socket.
+         * Check specifically, so we don't pass -1 to send().
+         */
+        errno = EBADF;
+        return -1;
+    }
+
     return send(so->s, buf, len, flags);
 }
 
@@ -1466,9 +1477,6 @@ static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
         int ret;
         struct socket *so = socreate(slirp);
 
-        if (!so)
-            return -ENOMEM;
-
         ret = vmstate_load_state(f, &vmstate_slirp_socket, so, version_id);
 
         if (ret < 0)

slirp/socket.c

@@ -46,17 +46,15 @@ struct socket *solookup(struct socket **last, struct socket *head,
 struct socket *
 socreate(Slirp *slirp)
 {
-  struct socket *so;
+    struct socket *so = g_new(struct socket, 1);
 
-  so = (struct socket *)malloc(sizeof(struct socket));
-  if(so) {
     memset(so, 0, sizeof(struct socket));
     so->so_state = SS_NOFDREF;
     so->s = -1;
     so->slirp = slirp;
     so->pollfds_idx = -1;
-  }
+
-  return(so);
+    return so;
 }
 
 /*
@@ -110,7 +108,7 @@ sofree(struct socket *so)
   if (so->so_tcpcb) {
       free(so->so_tcpcb);
   }
-  free(so);
+  g_free(so);
 }
 
 size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np)
@@ -715,13 +713,10 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	DEBUG_ARG("flags = %x", flags);
 
 	so = socreate(slirp);
-	if (!so) {
-	  return NULL;
-	}
 
 	/* Don't tcp_attach... we don't need so_snd nor so_rcv */
 	if ((so->so_tcpcb = tcp_newtcpcb(so)) == NULL) {
-		free(so);
+            g_free(so);
             return NULL;
 	}
 	insque(so, &slirp->tcb);

slirp/tcp_input.c

@@ -429,10 +429,9 @@ findso:
 	  if ((tiflags & (TH_SYN|TH_FIN|TH_RST|TH_URG|TH_ACK)) != TH_SYN)
 	    goto dropwithreset;
 
-	  if ((so = socreate(slirp)) == NULL)
+          so = socreate(slirp);
-	    goto dropwithreset;
 	  if (tcp_attach(so) < 0) {
-	    free(so); /* Not sofree (if it failed, it's not insqued) */
+            g_free(so); /* Not sofree (if it failed, it's not insqued) */
             goto dropwithreset;
 	  }
 

slirp/tcp_subr.c

@@ -469,13 +469,8 @@ void tcp_connect(struct socket *inso)
         so = inso;
     } else {
         so = socreate(slirp);
-        if (so == NULL) {
-            /* If it failed, get rid of the pending connection */
-            closesocket(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
-            return;
-        }
         if (tcp_attach(so) < 0) {
-            free(so); /* NOT sofree */
+            g_free(so); /* NOT sofree */
             return;
         }
         so->lhost = inso->lhost;

slirp/udp.c

@@ -171,9 +171,6 @@ udp_input(register struct mbuf *m, int iphlen)
 	   * create one
 	   */
 	  so = socreate(slirp);
-	  if (!so) {
-	      goto bad;
-	  }
 	  if (udp_attach(so, AF_INET) == -1) {
 	    DEBUG_MISC((dfd," udp_attach errno = %d-%s\n",
 			errno,strerror(errno)));
@@ -331,9 +328,6 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	socklen_t addrlen = sizeof(struct sockaddr_in);
 
 	so = socreate(slirp);
-	if (!so) {
-	    return NULL;
-	}
 	so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
         if (so->s < 0) {
             sofree(so);

slirp/udp6.c

@@ -91,9 +91,6 @@ void udp6_input(struct mbuf *m)
     if (so == NULL) {
         /* If there's no socket for this packet, create one. */
         so = socreate(slirp);
-        if (!so) {
-            goto bad;
-        }
         if (udp_attach(so, AF_INET6) == -1) {
             DEBUG_MISC((dfd, " udp6_attach errno = %d-%s\n",
                         errno, strerror(errno)));