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

Message ID 1486249715-5513-3-git-send-email-peter.maydell@linaro.org
State Superseded
Headers show
Series
  • slirp: fix 3 easy coverity warnings
Related show

Commit Message

Peter Maydell Feb. 4, 2017, 11:08 p.m.
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(-)

-- 
2.1.4

Comments

Philippe Mathieu-Daudé Feb. 11, 2017, 4:15 a.m. | #1
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;

>

Patch

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;