On 12.07.2016 13:49, Marc Hartmayer wrote:
> On Sat, Jul 09, 2016 at 11:02 AM +0200, Michal Privoznik <mprivozn@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Sascha Silbe <silbe@xxxxxxxxxxxxxxxxxx>
>>> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Marc Hartmayer <mhartmay@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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).
I've found it better to use '-O0 -ggdb'. Anyway, I'm unable to reproduce
the warning with -Og. At any rate, it's a false positive.
>
> 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.
>
I don't think there can be such access. I mean, firstly we get the total
number of guest vCPUs that we can get stats for. Then we check how many
stats can we get for a single vCPU (there's cpu_time, system_time,
user_time, ...).
Next, we allocate memory to hold all the stats combined (ncpus *
nstats). Actually, times two, one for old stats, one for new stats.
Then, virDomainGetCPUStats() is called repeatedly to fill those two
arrays. While calling it, we pass the maximum number of stats we expect
(fetched in the previous calls). So the only out of bound access there
could be is if virDomainGetCPUStats() would ignore that and returned
more stats all of sudden. But this would be impossible without
restarting the daemon and replacing it with a newer one (that probably
supports more stats). And if it was so, the connection would break and
the domtop example would terminate.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list