From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/5] xc/tmem: Free temporary buffer used during migration. Date: Mon, 25 Nov 2013 17:09:15 +0000 Message-ID: <5293843B.4000101@citrix.com> References: <1385398842-8247-1-git-send-email-konrad.wilk@oracle.com> <1385398842-8247-2-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vkzfw-0001OX-Gy for xen-devel@lists.xenproject.org; Mon, 25 Nov 2013 17:10:20 +0000 In-Reply-To: <1385398842-8247-2-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote: > CID 1090388. > > Within the loop reading the pool_id we set the buf: > > if ( (buf = realloc(buf,bufsize)) == NULL ) > > and then continue on using it without freeing. Worst yet > there are multiple 'if (..) return -1' which do not free > the buffer. > > As such insert a 'fail' goto label to free the buffer > and also add on the OK path a mechanism to free the buffer. > > Replace all of the 'return -1' with a jump to the failed > label. > > CC: Bob Liu > Signed-off-by: Konrad Rzeszutek Wilk Reviewed-by: Andrew Cooper > --- > tools/libxc/xc_tmem.c | 47 ++++++++++++++++++++++++++++------------------- > 1 file changed, 28 insertions(+), 19 deletions(-) > > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c > index 61e1549..0918615 100644 > --- a/tools/libxc/xc_tmem.c > +++ b/tools/libxc/xc_tmem.c > @@ -225,6 +225,7 @@ int xc_tmem_save(xc_interface *xch, > uint32_t pool_id; > uint32_t minusone = -1; > struct tmem_handle *h; > + char *buf = NULL; > > if ( xc_tmem_control(xch,0,TMEMC_SAVE_BEGIN,dom,live,0,0,NULL) <= 0 ) > return 0; > @@ -259,7 +260,6 @@ int xc_tmem_save(xc_interface *xch, > uint64_t uuid[2]; > uint32_t n_pages; > uint32_t pagesize; > - char *buf = NULL; > int bufsize = 0; > int checksum = 0; > > @@ -273,13 +273,13 @@ int xc_tmem_save(xc_interface *xch, > n_pages = 0; > (void)xc_tmem_control(xch,i,TMEMC_SAVE_GET_POOL_UUID,dom,sizeof(uuid),0,0,&uuid); > if ( write_exact(io_fd, &pool_id, sizeof(pool_id)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &flags, sizeof(flags)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &n_pages, sizeof(n_pages)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &uuid, sizeof(uuid)) ) > - return -1; > + goto failed; > if ( n_pages == 0 ) > continue; > > @@ -289,7 +289,7 @@ int xc_tmem_save(xc_interface *xch, > { > bufsize = pagesize + sizeof(struct tmem_handle); > if ( (buf = realloc(buf,bufsize)) == NULL ) > - return -1; > + goto failed; > } > for ( j = n_pages; j > 0; j-- ) > { > @@ -300,21 +300,22 @@ int xc_tmem_save(xc_interface *xch, > { > h = (struct tmem_handle *)buf; > if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) > - return -1; > + goto failed; > if ( write_exact(io_fd, &h->index, sizeof(h->index)) ) > - return -1; > + goto failed; > h++; > checksum += *(char *)h; > if ( write_exact(io_fd, h, pagesize) ) > - return -1; > + goto failed; > } else if ( ret == 0 ) { > continue; > } else { > /* page list terminator */ > h = (struct tmem_handle *)buf; > h->oid[0] = h->oid[1] = h->oid[2] = -1L; > - if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) > - return -1; > + if ( write_exact(io_fd, &h->oid, sizeof(h->oid)) ) { > + goto failed; > + } > break; > } > } > @@ -325,9 +326,13 @@ int xc_tmem_save(xc_interface *xch, > /* pool list terminator */ > minusone = -1; > if ( write_exact(io_fd, &minusone, sizeof(minusone)) ) > - return -1; > + goto failed; > > + free(buf); > return 1; > +failed: > + free(buf); > + return -1; > } > > /* only called for live migration */ > @@ -396,6 +401,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > uint32_t minusone; > uint32_t weight, cap, flags; > int checksum = 0; > + char *buf = NULL; > > save_version = xc_tmem_control(xch,0,TMEMC_SAVE_GET_VERSION,dom,0,0,0,NULL); > if ( save_version == -1 ) > @@ -433,7 +439,6 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > { > uint64_t uuid[2]; > uint32_t n_pages; > - char *buf = NULL; > int bufsize = 0, pagesize; > int j; > > @@ -455,7 +460,7 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > { > bufsize = pagesize; > if ( (buf = realloc(buf,bufsize)) == NULL ) > - return -1; > + goto failed; > } > for ( j = n_pages; j > 0; j-- ) > { > @@ -463,20 +468,20 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > uint32_t index; > int rc; > if ( read_exact(io_fd, &oid, sizeof(oid)) ) > - return -1; > + goto failed; > if ( oid.oid[0] == -1L && oid.oid[1] == -1L && oid.oid[2] == -1L ) > break; > if ( read_exact(io_fd, &index, sizeof(index)) ) > - return -1; > + goto failed; > if ( read_exact(io_fd, buf, pagesize) ) > - return -1; > + goto failed; > checksum += *buf; > if ( (rc = xc_tmem_control_oid(xch, pool_id, > TMEMC_RESTORE_PUT_PAGE, dom, > bufsize, index, oid, buf)) <= 0 ) > { > DPRINTF("xc_tmem_restore: putting page failed, rc=%d\n",rc); > - return -1; > + goto failed; > } > } > if ( n_pages ) > @@ -484,9 +489,13 @@ int xc_tmem_restore(xc_interface *xch, int dom, int io_fd) > n_pages-j,dom,pool_id,checksum); > } > if ( pool_id != -1 ) > - return -1; > + goto failed; > > + free(buf); > return 0; > +failed: > + free(buf); > + return -1; > } > > /* only called for live migration, must be called after suspend */