[Qemu-devel] [PATCH 0/3] slirp: fix 3 easy coverity warnings

[Qemu-devel] [PATCH 0/3] slirp: fix 3 easy coverity warnings

[Peter Maydell]

This patchset fixes three easy-to-fix coverity warnings in the slirp
code (there are another 5 or so which are not quite so simple).

As usual, the preexisting tab-indent style for a lot of the slirp
code is well out of line with the QEMU/checkpatch preferences.
I opted to generally use QEMU style for the new lines but this
does look a bit of a mess in some ways; happy to adjust per
slirp maintainer preferences.

thanks
-- PMM

Peter Maydell (3):
  slirp: Check qemu_socket() return value in udp_listen()
  slirp: Convert mbufs to use g_malloc() and g_free()
  slirp: tcp_listen(): Don't try to close() an fd we never opened

 slirp/mbuf.c   | 30 ++++++++++++++----------------
 slirp/socket.c |  4 +++-
 slirp/udp.c    |  4 ++++
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] slirp: Check qemu_socket() return value in udp_listen()

[Peter Maydell]

Check the return value from qemu_socket() rather than trying to
pass it to bind() as an fd argument even if it's negative.
This wouldn't have caused any negative consequences, because
it won't be a valid fd number and the bind call will fail;
but Coverity complains (CID 1005723).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/udp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/slirp/udp.c b/slirp/udp.c
index 93d7224..227d779 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -335,6 +335,10 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	    return NULL;
 	}
 	so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
+        if (so->s < 0) {
+            sofree(so);
+            return NULL;
+        }
 	so->so_expire = curtime + SO_EXPIRE;
 	insque(so, &slirp->udb);
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/3] slirp: Check qemu_socket() return value in udp_listen()

[Philippe Mathieu-Daudé]

On 02/04/2017 08:08 PM, Peter Maydell wrote:
> Check the return value from qemu_socket() rather than trying to
> pass it to bind() as an fd argument even if it's negative.
> This wouldn't have caused any negative consequences, because
> it won't be a valid fd number and the bind call will fail;
> but Coverity complains (CID 1005723).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/udp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 93d7224..227d779 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -335,6 +335,10 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
>  	    return NULL;
>  	}
>  	so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
> +        if (so->s < 0) {
> +            sofree(so);
> +            return NULL;
> +        }
>  	so->so_expire = curtime + SO_EXPIRE;
>  	insque(so, &slirp->udb);
>
>

[Qemu-devel] [PATCH 2/3] slirp: Convert mbufs to use g_malloc() and g_free()

[Peter Maydell]

The mbuf code currently doesn't check the result of doing a malloc()
or realloc() of its data (spotted by Coverity, CID 1238946).
Since the m_inc() API assumes that extending an mbuf must succeed,
just convert to g_malloc() and g_free().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/mbuf.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index 7eddc21..5ff2455 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -10,7 +10,7 @@
  * FreeBSD.  They are fixed size, determined by the MTU,
  * so that one whole packet can fit.  Mbuf's cannot be
  * chained together.  If there's more data than the mbuf
- * could hold, an external malloced buffer is pointed to
+ * could hold, an external g_malloced buffer is pointed to
  * by m_ext (and the data pointers) and M_EXT is set in
  * the flags
  */
@@ -41,26 +41,26 @@ void m_cleanup(Slirp *slirp)
     while ((struct quehead *) m != &slirp->m_usedlist) {
         next = m->m_next;
         if (m->m_flags & M_EXT) {
-            free(m->m_ext);
+            g_free(m->m_ext);
         }
-        free(m);
+        g_free(m);
         m = next;
     }
     m = (struct mbuf *) slirp->m_freelist.qh_link;
     while ((struct quehead *) m != &slirp->m_freelist) {
         next = m->m_next;
-        free(m);
+        g_free(m);
         m = next;
     }
 }
 
 /*
  * Get an mbuf from the free list, if there are none
- * malloc one
+ * allocate one
  *
  * Because fragmentation can occur if we alloc new mbufs and
  * free old mbufs, we mark all mbufs above mbuf_thresh as M_DOFREE,
- * which tells m_free to actually free() it
+ * which tells m_free to actually g_free() it
  */
 struct mbuf *
 m_get(Slirp *slirp)
@@ -71,8 +71,7 @@ m_get(Slirp *slirp)
 	DEBUG_CALL("m_get");
 
 	if (slirp->m_freelist.qh_link == &slirp->m_freelist) {
-		m = (struct mbuf *)malloc(SLIRP_MSIZE);
-		if (m == NULL) goto end_error;
+                m = g_malloc(SLIRP_MSIZE);
 		slirp->mbuf_alloced++;
 		if (slirp->mbuf_alloced > MBUF_THRESH)
 			flags = M_DOFREE;
@@ -94,7 +93,6 @@ m_get(Slirp *slirp)
         m->m_prevpkt = NULL;
         m->resolution_requested = false;
         m->expiration_date = (uint64_t)-1;
-end_error:
 	DEBUG_ARG("m = %p", m);
 	return m;
 }
@@ -112,15 +110,15 @@ m_free(struct mbuf *m)
 	   remque(m);
 
 	/* If it's M_EXT, free() it */
-	if (m->m_flags & M_EXT)
-	   free(m->m_ext);
-
+        if (m->m_flags & M_EXT) {
+                g_free(m->m_ext);
+        }
 	/*
 	 * Either free() it or put it on the free list
 	 */
 	if (m->m_flags & M_DOFREE) {
 		m->slirp->mbuf_alloced--;
-		free(m);
+                g_free(m);
 	} else if ((m->m_flags & M_FREELIST) == 0) {
 		insque(m,&m->slirp->m_freelist);
 		m->m_flags = M_FREELIST; /* Clobber other flags */
@@ -130,7 +128,7 @@ m_free(struct mbuf *m)
 
 /*
  * Copy data from one mbuf to the end of
- * the other.. if result is too big for one mbuf, malloc()
+ * the other.. if result is too big for one mbuf, allocate
  * an M_EXT data segment
  */
 void
@@ -160,12 +158,12 @@ m_inc(struct mbuf *m, int size)
 
         if (m->m_flags & M_EXT) {
 	  datasize = m->m_data - m->m_ext;
-	  m->m_ext = (char *)realloc(m->m_ext,size);
+          m->m_ext = g_realloc(m->m_ext, size);
 	  m->m_data = m->m_ext + datasize;
         } else {
 	  char *dat;
 	  datasize = m->m_data - m->m_dat;
-	  dat = (char *)malloc(size);
+          dat = g_malloc(size);
 	  memcpy(dat, m->m_dat, m->m_size);
 
 	  m->m_ext = dat;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 2/3] slirp: Convert mbufs to use g_malloc() and g_free()

[Philippe Mathieu-Daudé]

On 02/04/2017 08:08 PM, Peter Maydell wrote:
> The mbuf code currently doesn't check the result of doing a malloc()
> or realloc() of its data (spotted by Coverity, CID 1238946).
> Since the m_inc() API assumes that extending an mbuf must succeed,
> just convert to g_malloc() and g_free().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/mbuf.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/slirp/mbuf.c b/slirp/mbuf.c
> index 7eddc21..5ff2455 100644
> --- a/slirp/mbuf.c
> +++ b/slirp/mbuf.c
> @@ -10,7 +10,7 @@
>   * FreeBSD.  They are fixed size, determined by the MTU,
>   * so that one whole packet can fit.  Mbuf's cannot be
>   * chained together.  If there's more data than the mbuf
> - * could hold, an external malloced buffer is pointed to
> + * could hold, an external g_malloced buffer is pointed to
>   * by m_ext (and the data pointers) and M_EXT is set in
>   * the flags
>   */
> @@ -41,26 +41,26 @@ void m_cleanup(Slirp *slirp)
>      while ((struct quehead *) m != &slirp->m_usedlist) {
>          next = m->m_next;
>          if (m->m_flags & M_EXT) {
> -            free(m->m_ext);
> +            g_free(m->m_ext);
>          }
> -        free(m);
> +        g_free(m);
>          m = next;
>      }
>      m = (struct mbuf *) slirp->m_freelist.qh_link;
>      while ((struct quehead *) m != &slirp->m_freelist) {
>          next = m->m_next;
> -        free(m);
> +        g_free(m);
>          m = next;
>      }
>  }
>
>  /*
>   * Get an mbuf from the free list, if there are none
> - * malloc one
> + * allocate one
>   *
>   * Because fragmentation can occur if we alloc new mbufs and
>   * free old mbufs, we mark all mbufs above mbuf_thresh as M_DOFREE,
> - * which tells m_free to actually free() it
> + * which tells m_free to actually g_free() it
>   */
>  struct mbuf *
>  m_get(Slirp *slirp)
> @@ -71,8 +71,7 @@ m_get(Slirp *slirp)
>  	DEBUG_CALL("m_get");
>
>  	if (slirp->m_freelist.qh_link == &slirp->m_freelist) {
> -		m = (struct mbuf *)malloc(SLIRP_MSIZE);
> -		if (m == NULL) goto end_error;
> +                m = g_malloc(SLIRP_MSIZE);
>  		slirp->mbuf_alloced++;
>  		if (slirp->mbuf_alloced > MBUF_THRESH)
>  			flags = M_DOFREE;
> @@ -94,7 +93,6 @@ m_get(Slirp *slirp)
>          m->m_prevpkt = NULL;
>          m->resolution_requested = false;
>          m->expiration_date = (uint64_t)-1;
> -end_error:
>  	DEBUG_ARG("m = %p", m);
>  	return m;
>  }
> @@ -112,15 +110,15 @@ m_free(struct mbuf *m)
>  	   remque(m);
>
>  	/* If it's M_EXT, free() it */
> -	if (m->m_flags & M_EXT)
> -	   free(m->m_ext);
> -
> +        if (m->m_flags & M_EXT) {
> +                g_free(m->m_ext);
> +        }
>  	/*
>  	 * Either free() it or put it on the free list
>  	 */
>  	if (m->m_flags & M_DOFREE) {
>  		m->slirp->mbuf_alloced--;
> -		free(m);
> +                g_free(m);
>  	} else if ((m->m_flags & M_FREELIST) == 0) {
>  		insque(m,&m->slirp->m_freelist);
>  		m->m_flags = M_FREELIST; /* Clobber other flags */
> @@ -130,7 +128,7 @@ m_free(struct mbuf *m)
>
>  /*
>   * Copy data from one mbuf to the end of
> - * the other.. if result is too big for one mbuf, malloc()
> + * the other.. if result is too big for one mbuf, allocate
>   * an M_EXT data segment
>   */
>  void
> @@ -160,12 +158,12 @@ m_inc(struct mbuf *m, int size)
>
>          if (m->m_flags & M_EXT) {
>  	  datasize = m->m_data - m->m_ext;
> -	  m->m_ext = (char *)realloc(m->m_ext,size);
> +          m->m_ext = g_realloc(m->m_ext, size);
>  	  m->m_data = m->m_ext + datasize;
>          } else {
>  	  char *dat;
>  	  datasize = m->m_data - m->m_dat;
> -	  dat = (char *)malloc(size);
> +          dat = g_malloc(size);
>  	  memcpy(dat, m->m_dat, m->m_size);
>
>  	  m->m_ext = dat;
>

[Qemu-devel] [PATCH 3/3] slirp: tcp_listen(): Don't try to close() an fd we never opened

[Peter Maydell]

Coverity points out (CID 1005725) that an error-exit path in tcp_listen()
will try to close(s) even if the reason it got there was that the
qemu_socket() failed and s was never opened.  Not only that, this isn't even
the right function to use, because we need closesocket() to do the right
thing on Windows.  Change to using the right function and only calling it if
needed.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 slirp/socket.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 6c18971..8692772 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -713,7 +713,9 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	    (listen(s,1) < 0)) {
 		int tmperrno = errno; /* Don't clobber the real reason we failed */
 
-		close(s);
+                if (s >= 0) {
+                    closesocket(s);
+                }
 		sofree(so);
 		/* Restore the real errno */
 #ifdef _WIN32
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/3] slirp: tcp_listen(): Don't try to close() an fd we never opened

[Philippe Mathieu-Daudé]

On 02/04/2017 08:08 PM, Peter Maydell wrote:
> Coverity points out (CID 1005725) that an error-exit path in tcp_listen()
> will try to close(s) even if the reason it got there was that the
> qemu_socket() failed and s was never opened.  Not only that, this isn't even
> the right function to use, because we need closesocket() to do the right
> thing on Windows.  Change to using the right function and only calling it if
> needed.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 6c18971..8692772 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -713,7 +713,9 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
>  	    (listen(s,1) < 0)) {
>  		int tmperrno = errno; /* Don't clobber the real reason we failed */
>
> -		close(s);
> +                if (s >= 0) {
> +                    closesocket(s);
> +                }
>  		sofree(so);
>  		/* Restore the real errno */
>  #ifdef _WIN32
>