[libvirt] [PATCH 2/5] openvzGetProcessInfo: address clang-detected low-probability flaw

2010-04-14 Thread Jim Meyering 
From: Jim Meyering 

* src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize
so that unexpected /proc/vz/vestat content cannot make us use
uninitialized variables.  Without this change, an input line with
a matching "readvps", but fewer than 4 numbers would result in our
using at least "systime" uninitialized.
---
 src/openvz/openvz_driver.c |   30 +++---
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
index 95c4236..47004d6 100644
--- a/src/openvz/openvz_driver.c
+++ b/src/openvz/openvz_driver.c
@@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long 
*cpuTime, int vpsid) {
 int fd;
 char line[1024] ;
 unsigned long long usertime, systime, nicetime;
-int readvps = 0, ret;
+int readvps = vpsid + 1;  /* ensure readvps is initially different */
+int ret;

 /* read statistic from /proc/vz/vestat.
 sample:
 Version: 2.2
-  VEID user  nice system uptime idle   
other..
-33   78 0   1330   59454597  142650441835148   
other..
-55  178 0   5340   59424597  542650441835148   
other..
+   VEID user  nice system uptime idle   other..
+ 33   78 0   1330   59454597  142650441835148   other..
+ 55  178 0   5340   59424597  542650441835148   other..
 */

 if ((fd = open("/proc/vz/vestat", O_RDONLY)) == -1)
@@ -1400,15 +1401,18 @@ Version: 2.2
 /*search line with VEID=vpsid*/
 while(1) {
 ret = openvz_readline(fd, line, sizeof(line));
-if(ret <= 0)
+if (ret <= 0)
 break;

-if (sscanf(line, "%d %llu %llu %llu",
-  &readvps, &usertime, &nicetime, &systime) != 4)
-continue;
-
-if (readvps == vpsid)
-break; /*found vpsid*/
+if (sscanf (line, "%d %llu %llu %llu",
+&readvps, &usertime, &nicetime, &systime) == 4
+&& readvps == vpsid) { /*found vpsid*/
+/* convert jiffies to nanoseconds */
+*cpuTime = (1000ull * 1000ull * 1000ull
+* (usertime + nicetime  + systime)
+/ (unsigned long long)sysconf(_SC_CLK_TCK));
+break;
+}
 }

 close(fd);
@@ -1418,10 +1422,6 @@ Version: 2.2
 if (readvps != vpsid) /*not found*/
 return -1;

-/* convert jiffies to nanoseconds */
-*cpuTime = 1000ull * 1000ull * 1000ull * (usertime + nicetime  + systime)
- / (unsigned long 
long)sysconf(_SC_CLK_TCK);
-
 return 0;
 }

-- 
1.7.1.rc1.248.gcefbb

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] openvzGetProcessInfo: address clang-detected low-probability flaw

2010-04-14 Thread Eric Blake 
On 04/14/2010 02:46 AM, Jim Meyering wrote:
> From: Jim Meyering 
> 
> * src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize
> so that unexpected /proc/vz/vestat content cannot make us use
> uninitialized variables.  Without this change, an input line with
> a matching "readvps", but fewer than 4 numbers would result in our
> using at least "systime" uninitialized.
> ---
>  src/openvz/openvz_driver.c |   30 +++---
>  1 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 95c4236..47004d6 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long 
> *cpuTime, int vpsid) {
>  int fd;
>  char line[1024] ;
>  unsigned long long usertime, systime, nicetime;
> -int readvps = 0, ret;
> +int readvps = vpsid + 1;  /* ensure readvps is initially different */
> +int ret;
> 
> -if (sscanf(line, "%d %llu %llu %llu",
> -  &readvps, &usertime, &nicetime, &systime) != 4)
> -continue;
> -
> -if (readvps == vpsid)
> -break; /*found vpsid*/
> +if (sscanf (line, "%d %llu %llu %llu",
> +&readvps, &usertime, &nicetime, &systime) == 4
> +&& readvps == vpsid) { /*found vpsid*/
> +/* convert jiffies to nanoseconds */
> +*cpuTime = (1000ull * 1000ull * 1000ull
> +* (usertime + nicetime  + systime)
> +/ (unsigned long long)sysconf(_SC_CLK_TCK));
> +break;
> +}

ACK that the rewrite fixes the problem.  However, there's still the
issue that we're using sscanf in the first place, instead of
virStrToLong_ull; do you want to prepare a followup patch, or shall I?

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] openvzGetProcessInfo: address clang-detected low-probability flaw

2010-04-14 Thread Jim Meyering 
Eric Blake wrote:
> On 04/14/2010 02:46 AM, Jim Meyering wrote:
>> From: Jim Meyering 
>>
>> * src/openvz/openvz_driver.c (openvzGetProcessInfo): Reorganize
>> so that unexpected /proc/vz/vestat content cannot make us use
>> uninitialized variables.  Without this change, an input line with
>> a matching "readvps", but fewer than 4 numbers would result in our
>> using at least "systime" uninitialized.
>> ---
>>  src/openvz/openvz_driver.c |   30 +++---
>>  1 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
>> index 95c4236..47004d6 100644
>> --- a/src/openvz/openvz_driver.c
>> +++ b/src/openvz/openvz_driver.c
>> @@ -1384,14 +1384,15 @@ static int openvzGetProcessInfo(unsigned long long 
>> *cpuTime, int vpsid) {
>>  int fd;
>>  char line[1024] ;
>>  unsigned long long usertime, systime, nicetime;
>> -int readvps = 0, ret;
>> +int readvps = vpsid + 1;  /* ensure readvps is initially different */
>> +int ret;
>>
>> -if (sscanf(line, "%d %llu %llu %llu",
>> -  &readvps, &usertime, &nicetime, &systime) != 4)
>> -continue;
>> -
>> -if (readvps == vpsid)
>> -break; /*found vpsid*/
>> +if (sscanf (line, "%d %llu %llu %llu",
>> +&readvps, &usertime, &nicetime, &systime) == 4
>> +&& readvps == vpsid) { /*found vpsid*/
>> +/* convert jiffies to nanoseconds */
>> +*cpuTime = (1000ull * 1000ull * 1000ull
>> +* (usertime + nicetime  + systime)
>> +/ (unsigned long long)sysconf(_SC_CLK_TCK));
>> +break;
>> +}
>
> ACK that the rewrite fixes the problem.  However, there's still the

Thanks for the review.

> issue that we're using sscanf in the first place, instead of
> virStrToLong_ull; do you want to prepare a followup patch, or shall I?

I am in no big hurry on that front, at least not for this
particular case, where the risk of bogus input is probably negligible.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list