[libvirt] [PATCH] domtop: Properly free cpu status

[Laine Stump]

On 04/20/2015 05:41 AM, Michal Privoznik wrote:
> So, in the example the cpu stats are collected within a function
> called do_top. At the beginning of the function we ask the daemon for
> how much vCPUs can we get stats, and how many stats for a vCPU can we
> get. This is because it's how our API works - users are required to
> preallocate a chunk of memory for the results. Now, at the end, we try
> to free the allocated array, but we are not doing it correctly.
> There's this virTypedParamsFree() function which gets a pointer to the
> array and the length of the array. However, if there was an error in
> getting vCPU stats we pass a negative number instead of the originally
> computed value. This flaw results in SIGSEGV:
>
> libvirt: QEMU Driver error : Requested operation is not valid: domain is not running
> ERROR do_top:333 : Unable to get cpu stats
> ==29201== Invalid read of size 4
> ==29201==    at 0x4F1DF8B: virTypedParamsClear (virtypedparam.c:1145)
> ==29201==    by 0x4F1DFEB: virTypedParamsFree (virtypedparam.c:1165)
> ==29201==    by 0x4023C3: do_top (domtop.c:349)
> ==29201==    by 0x40260B: main (domtop.c:386)
> ==29201==  Address 0x131cd7c0 is 16 bytes after a block of size 768 alloc'd
> ==29201==    at 0x4C2C070: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==29201==    by 0x401FF1: do_top (domtop.c:295)
> ==29201==    by 0x40260B: main (domtop.c:386)
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  examples/domtop/domtop.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c
> index e50988e..2283994 100644
> --- a/examples/domtop/domtop.c
> +++ b/examples/domtop/domtop.c
> @@ -346,8 +346,8 @@ do_top(virConnectPtr conn,
>  
>      ret = 0;
>   cleanup:
> -    virTypedParamsFree(now_params, now_nparams * max_id);
> -    virTypedParamsFree(then_params, then_nparams * max_id);
> +    virTypedParamsFree(now_params, nparams * max_id);
> +    virTypedParamsFree(then_params, nparams * max_id);

nparams can also be set to -1 when we reach cleanup (if the attempt to
initially set it results in an error). But of course in those cases
now_params and then_params will still be NULL, so virTypedParamsFree()
will be a NOP (and will ignore the count entirely), so that's okay.

ACK.