[libvirt] [PATCH 06/12] [v7] virNodeGetCPUStats: Implement linux support

[Eric Blake]

On 06/14/2011 03:40 AM, Daniel P. Berrange wrote:
> On Tue, Jun 14, 2011 at 10:15:36AM +0100, Daniel P. Berrange wrote:
>> On Tue, Jun 07, 2011 at 10:02:55AM +0900, Minoru Usui wrote:
>>> virNodeGetCPUStats: Implement linux support
>>>
>>> Signed-off-by: Minoru Usui <usui at mxm.nes.nec.co.jp>
>>> +#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
>>> +#define CPU_HEADER_LEN 8
>>> +
>>> +int linuxNodeGetCPUStats(FILE *procstat,
>>> +                         int cpuNum,
>>> +                         virCPUStatsPtr params,
>>> +                         int *nparams)
>>> +{
>>> +    int ret = -1;
>>> +    char line[1024];
>>> +    unsigned long long usr, ni, sys, idle, iowait;
>>> +    unsigned long long irq, softirq, steal, guest, guest_nice;
>>> +    char cpu_header[CPU_HEADER_LEN];

>>> +    } else {
>>> +        sprintf(cpu_header, "cpu%d", cpuNum);
>>> +    }
>>
>> cpu_header is declared to be 8 bytes in size, which only allows
>> for integers upto 4 digits long, before we get a buffer overflow
>> here.
>>
>> gnulib has some macro which can be used to declare a string buffer
>> large enough to hold an integer, but I can't remember what the
>> macro is called right now. Hopefully Eric will remind us shortly....
> 
> Jim Meyering reminded me that it is INT_BUFSIZE_BOUND. So eg you
> want todo
> 
>    char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum) + 1];
> 
> to allow enough space for 'cpu' + any cpuNum value formatted as
> a string + the trailing NULL.

Actually, INT_BUFSIZE_BOUND already includes space for the trailing NUL,
so the +1 is not needed.

> ACK if the buffer overflow problem is solved

Also, 'make syntax-check' complained about cppi indentation, and the
fact that we've blacklisted sprintf (use snprintf instead).  Plus you
had some weird indentation.

Here's what I squashed before pushing:

diff --git i/src/nodeinfo.c w/src/nodeinfo.c
index 235a68a..bdf8f8a 100644
--- i/src/nodeinfo.c
+++ w/src/nodeinfo.c
@@ -66,10 +66,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
                              virNodeInfoPtr nodeinfo,
                              bool need_hyperthreads);

-int linuxNodeGetCPUStats(FILE *procstat,
-                         int cpuNum,
-                         virCPUStatsPtr params,
-                         int *nparams);
+static int linuxNodeGetCPUStats(FILE *procstat,
+                                int cpuNum,
+                                virCPUStatsPtr params,
+                                int *nparams);

 /* Return the positive decimal contents of the given
  * CPU_SYS_PATH/cpu%u/FILE, or -1 on error.  If MISSING_OK and the
@@ -384,8 +384,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo,
     return 0;
 }

-#define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
-#define CPU_HEADER_LEN 8
+# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))

 int linuxNodeGetCPUStats(FILE *procstat,
                          int cpuNum,
@@ -396,7 +395,7 @@ int linuxNodeGetCPUStats(FILE *procstat,
     char line[1024];
     unsigned long long usr, ni, sys, idle, iowait;
     unsigned long long irq, softirq, steal, guest, guest_nice;
-    char cpu_header[CPU_HEADER_LEN];
+    char cpu_header[3 + INT_BUFSIZE_BOUND(cpuNum)];

     if ((*nparams) == 0) {
         /* Current number of cpu stats supported by linux */
@@ -414,7 +413,7 @@ int linuxNodeGetCPUStats(FILE *procstat,
     if (cpuNum == VIR_CPU_STATS_ALL_CPUS) {
         strcpy(cpu_header, "cpu");
     } else {
-        sprintf(cpu_header, "cpu%d", cpuNum);
+        snprintf(cpu_header, sizeof(cpu_header), "cpu%d", cpuNum);
     }

     while (fgets(line, sizeof(line), procstat) != NULL) {
@@ -436,7 +435,7 @@ int linuxNodeGetCPUStats(FILE *procstat,

                 switch (i) {
                 case 0: /* fill kernel cpu time here */
-                    if (virStrcpyStatic(param->field,
VIR_CPU_STATS_KERNEL)== NULL) {
+                    if (virStrcpyStatic(param->field,
VIR_CPU_STATS_KERNEL) == NULL) {
                         nodeReportError(VIR_ERR_INTERNAL_ERROR,
                                         "%s", _("Field kernel cpu time
too long for destination"));
                         goto cleanup;
@@ -528,22 +527,23 @@ int nodeGetCPUStats(virConnectPtr conn
ATTRIBUTE_UNUSED,
                     int cpuNum,
                     virCPUStatsPtr params,
                     int *nparams,
-                    unsigned int flags ATTRIBUTE_UNUSED)
+                    unsigned int flags)
 {
+    virCheckFlags(0, -1);

 #ifdef __linux__
     {
-    int ret;
-    FILE *procstat = fopen(PROCSTAT_PATH, "r");
-    if (!procstat) {
-        virReportSystemError(errno,
-                             _("cannot open %s"), PROCSTAT_PATH);
-        return -1;
-    }
-    ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams);
-    VIR_FORCE_FCLOSE(procstat);
+        int ret;
+        FILE *procstat = fopen(PROCSTAT_PATH, "r");
+        if (!procstat) {
+            virReportSystemError(errno,
+                                 _("cannot open %s"), PROCSTAT_PATH);
+            return -1;
+        }
+        ret = linuxNodeGetCPUStats(procstat, cpuNum, params, nparams);
+        VIR_FORCE_FCLOSE(procstat);

-    return ret;
+        return ret;
     }
 #else
     nodeReportError(VIR_ERR_NO_SUPPORT, "%s",

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110614/3e3dbac4/attachment-0001.sig>