[libvirt] [PATCH] examples: If parameters count is zero we have nothing to do

[Marc Hartmayer]

On Sat, Jul 09, 2016 at 11:02 AM +0200, Michal Privoznik <mprivozn at redhat.com> 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 at linux.vnet.ibm.com>
>> Reviewed-by: Sascha Silbe <silbe at linux.vnet.ibm.com>
>> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>> ---
>>  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.

If you want to I can send a v2 with an adapted commit message.

 Marc