The virDomainGetCPUStats() API contract permits nparams == 0. print_cpu_usage() assumes nparams > 0 because the domtop example application isn't very useful if there are no statistics. Explicitly error out to avoid potentially using the local variable pos uninitialized. Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Sascha Silbe <silbe(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; --- examples/domtop/domtop.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index 2283994..f8e250b 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -203,6 +203,11 @@ print_cpu_usage(const char *dom_name, size_t nparams = now_nparams; bool delim = false; + if (nparams == 0) { + ERROR("parameter count is zero"); + return; + } + if (then_nparams != now_nparams) { /* this should not happen (TM) */ ERROR("parameters counts don't match");

On Sat, Jul 09, 2016 at 11:02 AM +0200, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: > On 06.07.2016 15:47, Marc Hartmayer wrote: >> The virDomainGetCPUStats() API contract permits nparams == >> 0. print_cpu_usage() assumes nparams > 0 because the domtop example >> application isn't very useful if there are no statistics. Explicitly >> error out to avoid potentially using the local variable pos >> uninitialized. >> >> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Sascha Silbe <silbe(a)linux.vnet.ibm.com&gt; >> Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; >> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; >> --- >> examples/domtop/domtop.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c >> index 2283994..f8e250b 100644 >> --- a/examples/domtop/domtop.c >> +++ b/examples/domtop/domtop.c >> @@ -203,6 +203,11 @@ print_cpu_usage(const char *dom_name, >> size_t nparams = now_nparams; >> bool delim = false; >> >> + if (nparams == 0) { >> + ERROR("parameter count is zero"); >> + return; >> + } >> + >> if (then_nparams != now_nparams) { >> /* this should not happen (TM) */ >> ERROR("parameters counts don't match"); >> > > Not that I dislike this patch, but I fail to see how can @pos be used > uninitialized if @nparams == 0. I mean, if @nparams == 0 and control > gets to line 221 where @j is set to 0. Since the condition is false, the > loop body is never executed leaving @pos unset. But, right after the > loop, there's check 'if (j == nparams)', which appears like true to me, > so and error is printed out and control jumps out from the function. > > Or am I missing something? No, you're right. I had the gcc warning 'may be used uninitialized in this function' warning for the variable 'pos' if I'm compiling libvirt with the optimization level for debugging (-Og).

Nevertheless, I think there could be an out-of-bound access on the now_params/then_params array if nparams == 0 and the array now/then_params has the size 0. I'm not quite sure if this is possible at the moment because the virDomainGetCPUStats() function isn't that easy to understand.