[libvirt] [PATCH] examples: If parameters count is zero we have nothing to do
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@linux.vnet.ibm.com> Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@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"); -- 2.5.5
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@linux.vnet.ibm.com> Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@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? Michal
On Sat, Jul 09, 2016 at 11:02 AM +0200, Michal Privoznik <mprivozn@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@linux.vnet.ibm.com> Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@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
On 12.07.2016 13:49, Marc Hartmayer wrote:
On Sat, Jul 09, 2016 at 11:02 AM +0200, Michal Privoznik <mprivozn@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@linux.vnet.ibm.com> Reviewed-by: Sascha Silbe <silbe@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Signed-off-by: Marc Hartmayer <mhartmay@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).
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
participants (2)
-
Marc Hartmayer -
Michal Privoznik