diff mbox

libxc: Fix CID 1351228 resource leak

Message ID 1455095385-3120-1-git-send-email-write.harmandeep@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Harmandeep Kaur Feb. 10, 2016, 9:09 a.m. UTC 
Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 tools/libxc/xc_tbuf.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dario Faggioli Feb. 10, 2016, 9:32 a.m. UTC | #1 
On Wed, 2016-02-10 at 14:39 +0530, Harmandeep Kaur wrote:
>
What I just said about the other patch ("libxc: Fix CID 1351225
resource leak") applies here as well, of course. :-)

About the code...

> --- a/tools/libxc/xc_tbuf.c
> +++ b/tools/libxc/xc_tbuf.c
> @@ -70,9 +70,13 @@ int xc_tbuf_get_size(xc_interface *xch, unsigned
> long *size)
>                      sysctl.u.tbuf_op.buffer_mfn);
>  
>      if ( t_info == NULL || t_info->tbuf_size == 0 )
> +    {
> +        xenforeignmemory_unmap(xch->fmem, t_info, 0);
>          return -1;
> +    }
>  
>      *size = t_info->tbuf_size;
> +    xenforeignmemory_unmap(xch->fmem, t_info, *size);
>  
I think you can arrange for only calling the unmapping function once,
i.e., put the unmap call in a place where (after a slight
reorganization of the rest of the code as well) it can be common to
both the success and error path.

Regards,
Dario
Ian Jackson Feb. 10, 2016, 11:14 a.m. UTC | #2 
Dario Faggioli writes ("Re: [PATCH] libxc: Fix CID 1351228 resource leak"):
> On Wed, 2016-02-10 at 14:39 +0530, Harmandeep Kaur wrote:
> >
> What I just said about the other patch ("libxc: Fix CID 1351225
> resource leak") applies here as well, of course. :-)
> 
> About the code...
...
> >      if ( t_info == NULL || t_info->tbuf_size == 0 )
> > +    {
> > +        xenforeignmemory_unmap(xch->fmem, t_info, 0);
> >          return -1;
> > +    }
> >  
> >      *size = t_info->tbuf_size;
> > +    xenforeignmemory_unmap(xch->fmem, t_info, *size);
...
> I think you can arrange for only calling the unmapping function once,
> i.e., put the unmap call in a place where (after a slight
> reorganization of the rest of the code as well) it can be common to
> both the success and error path.

Indeed.  See tools/libxl/CODING_STYLE.

Ian.
Ian Campbell Feb. 10, 2016, 2:09 p.m. UTC | #3 
On Wed, 2016-02-10 at 11:14 +0000, Ian Jackson wrote:
> Dario Faggioli writes ("Re: [PATCH] libxc: Fix CID 1351228 resource
> leak"):
> > On Wed, 2016-02-10 at 14:39 +0530, Harmandeep Kaur wrote:
> > > 
> > What I just said about the other patch ("libxc: Fix CID 1351225
> > resource leak") applies here as well, of course. :-)
> > 
> > About the code...
> ...
> > >      if ( t_info == NULL || t_info->tbuf_size == 0 )
> > > +    {
> > > +        xenforeignmemory_unmap(xch->fmem, t_info, 0);
> > >          return -1;
> > > +    }
> > >  
> > >      *size = t_info->tbuf_size;
> > > +    xenforeignmemory_unmap(xch->fmem, t_info, *size);
> ...
> > I think you can arrange for only calling the unmapping function once,
> > i.e., put the unmap call in a place where (after a slight
> > reorganization of the rest of the code as well) it can be common to
> > both the success and error path.
> 
> Indeed.  See tools/libxl/CODING_STYLE.

The code being modified here is in libxc, so strictly speaking this doesn't
apply. However it is still a good principal to obey.

Ian.
diff mbox

Patch

diff --git a/tools/libxc/xc_tbuf.c b/tools/libxc/xc_tbuf.c
index 695939a..f06f566 100644
--- a/tools/libxc/xc_tbuf.c
+++ b/tools/libxc/xc_tbuf.c
@@ -70,9 +70,13 @@  int xc_tbuf_get_size(xc_interface *xch, unsigned long *size)
                     sysctl.u.tbuf_op.buffer_mfn);
 
     if ( t_info == NULL || t_info->tbuf_size == 0 )
+    {
+        xenforeignmemory_unmap(xch->fmem, t_info, 0);
         return -1;
+    }
 
     *size = t_info->tbuf_size;
+    xenforeignmemory_unmap(xch->fmem, t_info, *size);
 
     return 0;
 }