[Qemu-devel] [PATCH for-3.1 0/4] slirp: fix coverity issues

[Qemu-devel] [PATCH for-3.1 0/4] slirp: fix coverity issues

[Peter Maydell]

There are three outstanding Coverity issues for the slirp code.
This patchset fixes them:
 * easiest first, we explicitly check for so->s == -1 in slirp_send()
   to avoid possibly passing -1 to the send() function.
   In an ideal world we could assert() it wasn't -1, but the
   slirp code is too complicated to be sure...
 * we render a bug in an error-handling codepath moot by
   switching socreate() to use g_new() rather than malloc(),
   which means it can no longer fail and we can delete all the
   error-handling code
 * last and most complicated, we fix a lack of error handling
   and possible deadlock in fork_exec() by moving the child
   socket creation and connect to before fork(), where we can
   handle the errors appropriately

Tested with an aarch64 Linux guest which I got to do various
networky things including an 'apt-get update' and downloading
some packages. Also tested the '-net user,smb=/some/dir' which
is the most common use of the fork_exec() codepath.

I've taken the usual approach of "fix up the indentation
only locally so as to placate checkpatch" with this patchset.
Happy to adjust if you have some other preference.
(I'm veering towards the opinion that at some point we should
just say "globally reindent slirp/, because the piecemeal
approach hasn't in practice resulted in a very good outcome";
but that's up to you.)

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()

 slirp/ip_icmp.c   |  2 +-
 slirp/misc.c      | 55 +++++++++++++++++++++++++++++------------------
 slirp/slirp.c     | 14 +++++++++---
 slirp/socket.c    | 17 ++++++---------
 slirp/tcp_input.c |  7 +++---
 slirp/tcp_subr.c  |  7 +-----
 slirp/udp.c       |  6 ------
 slirp/udp6.c      |  3 ---
 8 files changed, 56 insertions(+), 55 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-3.1 1/4] slirp: Don't pass possibly -1 fd to send()

[Peter Maydell]

Coverity complains (CID 1005726) that we might pass -1 as the fd
argument to send() in slirp_send(), because we previously checked for
"so->s == -1 && so->extra".  The case of "so->s == -1 but so->extra
NULL" should not in theory happen, but it is hard to guarantee
because various places in the code do so->s = qemu_socket(...) and so
will end up with so->s == -1 on failure, and not all the paths which
call that always throw away the socket in that case (eg
tcp_fconnect()).  So just check specifically for the condition and
fail slirp_send().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This is to some extent just placating Coverity.
---
 slirp/slirp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 51de41fc021..3c3c03b22f7 100644
--- a/slirp/slirp.c
+++ b/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);
 }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 1/4] slirp: Don't pass possibly -1 fd to send()

[Samuel Thibault]

Peter Maydell, le mar. 06 nov. 2018 15:13:20 +0000, a ecrit:
> Coverity complains (CID 1005726) that we might pass -1 as the fd
> argument to send() in slirp_send(), because we previously checked for
> "so->s == -1 && so->extra".  The case of "so->s == -1 but so->extra
> NULL" should not in theory happen, but it is hard to guarantee
> because various places in the code do so->s = qemu_socket(...) and so
> will end up with so->s == -1 on failure, and not all the paths which
> call that always throw away the socket in that case (eg
> tcp_fconnect()).  So just check specifically for the condition and
> fail slirp_send().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This is to some extent just placating Coverity.

Applied, thanks!

[Qemu-devel] [PATCH for-3.1 2/4] slirp: Use g_new() to allocate sockets in socreate()

[Peter Maydell]

The slirp socreate() function can only fail if the attempt
to malloc() the struct socket fails. Switch to using
g_new() instead, which will allow us to remove the
error-handling code from its callers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We already use g_new/g_malloc in slirp, including for
mbuf buffers which are larger than these socket structs.
The motivation here is that we can render moot a Coverity
complaint about an issue in an error-handling path.

As usual, indenting in slirp code is a bit of a mess;
I've opted for "keep checkpatch happy".
---
 slirp/socket.c    | 14 ++++++--------
 slirp/tcp_input.c |  4 ++--
 slirp/tcp_subr.c  |  2 +-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 322383a1f9f..35a9a145659 100644
--- a/slirp/socket.c
+++ b/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)
@@ -721,8 +719,8 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 
 	/* Don't tcp_attach... we don't need so_snd nor so_rcv */
 	if ((so->so_tcpcb = tcp_newtcpcb(so)) == NULL) {
-		free(so);
-		return NULL;
+            g_free(so);
+            return NULL;
 	}
 	insque(so, &slirp->tcb);
 
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 07bcbdb2ddd..4f79c95fdb4 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -432,8 +432,8 @@ findso:
 	  if ((so = socreate(slirp)) == NULL)
 	    goto dropwithreset;
 	  if (tcp_attach(so) < 0) {
-	    free(so); /* Not sofree (if it failed, it's not insqued) */
-	    goto dropwithreset;
+            g_free(so); /* Not sofree (if it failed, it's not insqued) */
+            goto dropwithreset;
 	  }
 
 	  sbreserve(&so->so_snd, TCP_SNDSPACE);
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 8d0f94b75fc..0270c89ae3a 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -475,7 +475,7 @@ void tcp_connect(struct socket *inso)
             return;
         }
         if (tcp_attach(so) < 0) {
-            free(so); /* NOT sofree */
+            g_free(so); /* NOT sofree */
             return;
         }
         so->lhost = inso->lhost;
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 2/4] slirp: Use g_new() to allocate sockets in socreate()

[Samuel Thibault]

Peter Maydell, le mar. 06 nov. 2018 15:13:21 +0000, a ecrit:
> The slirp socreate() function can only fail if the attempt
> to malloc() the struct socket fails. Switch to using
> g_new() instead, which will allow us to remove the
> error-handling code from its callers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> We already use g_new/g_malloc in slirp, including for
> mbuf buffers which are larger than these socket structs.
> The motivation here is that we can render moot a Coverity
> complaint about an issue in an error-handling path.
> 
> As usual, indenting in slirp code is a bit of a mess;
> I've opted for "keep checkpatch happy".

Applied, thanks!

Samuel

[Qemu-devel] [PATCH for-3.1 3/4] slirp: Remove code that handles socreate() failure

[Peter Maydell]

Now that socreate() can never fail, we can remove the code
that was trying to handle that situation.

In particular this removes code in tcp_connect() that
provoked Coverity to complain (CID 1005724): in
 closesocket(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
if the accept() call fails then we pass closesocket() -1
instead of a valid file descriptor.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/ip_icmp.c   | 2 +-
 slirp/slirp.c     | 3 ---
 slirp/socket.c    | 3 ---
 slirp/tcp_input.c | 3 +--
 slirp/tcp_subr.c  | 5 -----
 slirp/udp.c       | 6 ------
 slirp/udp6.c      | 3 ---
 7 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index da100d1f55f..9210eef3f35 100644
--- a/slirp/ip_icmp.c
+++ b/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;
       }
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 3c3c03b22f7..322edf51eb8 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1477,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)
diff --git a/slirp/socket.c b/slirp/socket.c
index 35a9a145659..c01d8696afb 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -713,9 +713,6 @@ 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) {
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 4f79c95fdb4..d073ef95256 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -429,8 +429,7 @@ findso:
 	  if ((tiflags & (TH_SYN|TH_FIN|TH_RST|TH_URG|TH_ACK)) != TH_SYN)
 	    goto dropwithreset;
 
-	  if ((so = socreate(slirp)) == NULL)
-	    goto dropwithreset;
+          so = socreate(slirp);
 	  if (tcp_attach(so) < 0) {
             g_free(so); /* Not sofree (if it failed, it's not insqued) */
             goto dropwithreset;
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 0270c89ae3a..fa61349cbb0 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -469,11 +469,6 @@ 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) {
             g_free(so); /* NOT sofree */
             return;
diff --git a/slirp/udp.c b/slirp/udp.c
index e5bf065bf23..c47870a61b4 100644
--- a/slirp/udp.c
+++ b/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);
diff --git a/slirp/udp6.c b/slirp/udp6.c
index 7c4a6b003a8..986010f0d3a 100644
--- a/slirp/udp6.c
+++ b/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)));
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 3/4] slirp: Remove code that handles socreate() failure

[Samuel Thibault]

Peter Maydell, le mar. 06 nov. 2018 15:13:22 +0000, a ecrit:
> Now that socreate() can never fail, we can remove the code
> that was trying to handle that situation.
> 
> In particular this removes code in tcp_connect() that
> provoked Coverity to complain (CID 1005724): in
>  closesocket(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
> if the accept() call fails then we pass closesocket() -1
> instead of a valid file descriptor.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Applied, thanks!

[Qemu-devel] [PATCH for-3.1 4/4] slirp: fork_exec(): create and connect child socket before fork()

[Peter Maydell]

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>
---
 slirp/misc.c | 55 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index 260187b6b62..57bdd808e2d 100644
--- a/slirp/misc.c
+++ b/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;
-	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 */
@@ -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);
-                /*
-                 * 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);
 
@@ -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...
-                 * 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);
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 4/4] slirp: fork_exec(): create and connect child socket before fork()

[Samuel Thibault]

Peter Maydell, le mar. 06 nov. 2018 15:13:23 +0000, a ecrit:
> 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>

Applied, thanks!

Samuel