diff mbox

[2/3] target/ppc: fix memory leak in kvmppc_is_mem_backend_page_size_ok()

Message ID 149676257351.31274.10816877292939465521.stgit@bahia.lab.toulouse-stg.fr.ibm.com
State New
Headers show

Commit Message

Greg Kurz June 6, 2017, 3:22 p.m. UTC 
The string returned by object_property_get_str() is dynamically allocated.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/kvm.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Peter Maydell June 6, 2017, 3:28 p.m. UTC | #1 
On 6 June 2017 at 16:22, Greg Kurz <groug@kaod.org> wrote:
> The string returned by object_property_get_str() is dynamically allocated.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  target/ppc/kvm.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 88817620766c..f2f7c531bc7b 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -486,6 +486,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
>
>      if (mempath) {
>          pagesize = qemu_mempath_getpagesize(mempath);
> +        g_free(mempath);
>      } else {
>          pagesize = getpagesize();
>      }

Huh, I wasn't expecting this function to free its argument.
If we take that API then don't we also need to change the
two other implementations of it in the tree? Having the
single caller do the g_free() seems simpler...

thanks
-- PMM
Greg Kurz June 6, 2017, 3:41 p.m. UTC | #2 
On Tue, 6 Jun 2017 16:28:26 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 6 June 2017 at 16:22, Greg Kurz <groug@kaod.org> wrote:
> > The string returned by object_property_get_str() is dynamically allocated.
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  target/ppc/kvm.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 88817620766c..f2f7c531bc7b 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -486,6 +486,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
> >
> >      if (mempath) {
> >          pagesize = qemu_mempath_getpagesize(mempath);
> > +        g_free(mempath);
> >      } else {
> >          pagesize = getpagesize();
> >      }  
> 
> Huh, I wasn't expecting this function to free its argument.

Hmm... mempath isn't an argument, it is computed locally:

bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
{
    Object *mem_obj = object_resolve_path(obj_path, NULL);
    char *mempath = object_property_get_str(mem_obj, "mem-path", NULL);
    long pagesize;

    if (mempath) {
        pagesize = qemu_mempath_getpagesize(mempath);
        g_free(mempath);
    } else {
        pagesize = getpagesize();
    }

    return pagesize >= max_cpu_page_size;
}

> If we take that API then don't we also need to change the
> two other implementations of it in the tree? Having the
> single caller do the g_free() seems simpler...
> 
> thanks
> -- PMM
Thomas Huth June 6, 2017, 3:41 p.m. UTC | #3 
On 06.06.2017 17:22, Greg Kurz wrote:
> The string returned by object_property_get_str() is dynamically allocated.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  target/ppc/kvm.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 88817620766c..f2f7c531bc7b 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -486,6 +486,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
>  
>      if (mempath) {
>          pagesize = qemu_mempath_getpagesize(mempath);
> +        g_free(mempath);
>      } else {
>          pagesize = getpagesize();
>      }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>
Peter Maydell June 6, 2017, 3:43 p.m. UTC | #4 
On 6 June 2017 at 16:41, Greg Kurz <groug@kaod.org> wrote:
> On Tue, 6 Jun 2017 16:28:26 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 6 June 2017 at 16:22, Greg Kurz <groug@kaod.org> wrote:
>> > The string returned by object_property_get_str() is dynamically allocated.
>> >
>> > Signed-off-by: Greg Kurz <groug@kaod.org>
>> > ---
>> >  target/ppc/kvm.c |    1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> > index 88817620766c..f2f7c531bc7b 100644
>> > --- a/target/ppc/kvm.c
>> > +++ b/target/ppc/kvm.c
>> > @@ -486,6 +486,7 @@ bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
>> >
>> >      if (mempath) {
>> >          pagesize = qemu_mempath_getpagesize(mempath);
>> > +        g_free(mempath);
>> >      } else {
>> >          pagesize = getpagesize();
>> >      }
>>
>> Huh, I wasn't expecting this function to free its argument.
>
> Hmm... mempath isn't an argument, it is computed locally:

Oops; sorry, I misread the patch.

thanks
-- PMM
diff mbox

Patch

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 88817620766c..f2f7c531bc7b 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -486,6 +486,7 @@  bool kvmppc_is_mem_backend_page_size_ok(const char *obj_path)
 
     if (mempath) {
         pagesize = qemu_mempath_getpagesize(mempath);
+        g_free(mempath);
     } else {
         pagesize = getpagesize();
     }