commit 5ebdf2fe3f1aca661ed1a13a6aa45e3c27d228c2
Author: Nathan Scott <nathans@redhat.com>
Date: Thu Feb 13 14:12:49 2014 +1100
Fix valgrind errors in pmdalinux /proc/stat refresh
This one has bugged me for a long time, and I finally needed to spend
some time hunting an obscure memory corruption in pmdalinux - so got
to revisit this properly.
There were a few small problems in refresh_proc_stat causing valgrind
to report "invalid read of zero size". Most problematic was the code
reading the stat buffer initially, which slightly confuses the offset
and size combo in passing a buffer pointer to read(2). The realloc
call here also happened to allocate 8x the (small) memory it wanted,
accidently multiplying the size by sizeof(char).
The nbufindex calculation also ended up with one more line than was
in the buffer, which was OK because it always created an empty line
at the end of it (which doesn't actually exist in /proc/stat).
Finally, added some small improvements to the handling of ENOMEM for
these global stat buffers, and removed a double-memset on the initial
call (which callocs a zeroed buffer).
diff --git a/src/pmdas/linux/proc_stat.c b/src/pmdas/linux/proc_stat.c
index f4e001c..234b713 100644
--- a/src/pmdas/linux/proc_stat.c
+++ b/src/pmdas/linux/proc_stat.c
@@ -1,7 +1,7 @@
/*
* Linux /proc/stat metrics cluster
*
- * Copyright (c) 2012 Red Hat.
+ * Copyright (c) 2012-2014 Red Hat.
* Copyright (c) 2008-2009 Aconex. All Rights Reserved.
* Copyright (c) 2000,2004-2008 Silicon Graphics, Inc. All Rights Reserved.
*
@@ -39,22 +39,28 @@ refresh_proc_stat(proc_cpuinfo_t *proc_cpuinfo, proc_stat_t *proc_stat)
static char **bufindex;
static int nbufindex;
static int maxbufindex;
+ int size;
int n;
int i;
int j;
- if (fd >= 0)
- lseek(fd, 0, SEEK_SET);
- else
+ if (fd >= 0) {
+ if (lseek(fd, 0, SEEK_SET) < 0)
+ return -oserror();
+ } else {
if ((fd = open("/proc/stat", O_RDONLY)) < 0)
return -oserror();
+ }
for (n=0;;) {
- if (n >= maxstatbuf) {
- maxstatbuf += 512;
- statbuf = (char *)realloc(statbuf, maxstatbuf * sizeof(char));
+ while (n >= maxstatbuf) {
+ size = maxstatbuf + 512;
+ if ((statbuf = (char *)realloc(statbuf, size)) == NULL)
+ return -ENOMEM;
+ maxstatbuf = size;
}
- if ((i = read(fd, statbuf+n, 512)) > 0)
+ size = (statbuf + maxstatbuf) - (statbuf + n);
+ if ((i = read(fd, statbuf + n, size)) > 0)
n += i;
else
break;
@@ -62,20 +68,24 @@ refresh_proc_stat(proc_cpuinfo_t *proc_cpuinfo, proc_stat_t *proc_stat)
statbuf[n] = '\0';
if (bufindex == NULL) {
+ size = 4 * sizeof(char *);
+ if ((bufindex = (char **)malloc(size)) == NULL)
+ return -ENOMEM;
maxbufindex = 4;
- bufindex = (char **)malloc(maxbufindex * sizeof(char *));
}
nbufindex = 0;
- bufindex[nbufindex++] = statbuf;
+ bufindex[nbufindex] = statbuf;
for (i=0; i < n; i++) {
- if (statbuf[i] == '\n') {
+ if (statbuf[i] == '\n' || statbuf[i] == '\0') {
statbuf[i] = '\0';
- if (nbufindex >= maxbufindex) {
+ if (nbufindex + 1 >= maxbufindex) {
+ size = (maxbufindex + 4) * sizeof(char *);
+ if ((bufindex = (char **)realloc(bufindex, size)) == NULL)
+ return -ENOMEM;
maxbufindex += 4;
- bufindex = (char **)realloc(bufindex, maxbufindex * sizeof(char *));
}
- bufindex[nbufindex++] = statbuf + i + 1;
+ bufindex[++nbufindex] = statbuf + i + 1;
}
}
@@ -130,19 +140,19 @@ refresh_proc_stat(proc_cpuinfo_t *proc_cpuinfo, proc_stat_t *proc_stat)
proc_stat->n_steal = calloc(1, n);
proc_stat->n_guest = calloc(1, n);
}
-
- /* reset per-node stats */
- n = proc_cpuinfo->node_indom->it_numinst * sizeof(unsigned long long);
- memset(proc_stat->n_user, 0, n);
- memset(proc_stat->n_nice, 0, n);
- memset(proc_stat->n_sys, 0, n);
- memset(proc_stat->n_idle, 0, n);
- memset(proc_stat->n_wait, 0, n);
- memset(proc_stat->n_irq, 0, n);
- memset(proc_stat->n_sirq, 0, n);
- memset(proc_stat->n_steal, 0, n);
- memset(proc_stat->n_guest, 0, n);
-
+ else {
+ /* reset per-node stats */
+ n = proc_cpuinfo->node_indom->it_numinst * sizeof(unsigned long long);
+ memset(proc_stat->n_user, 0, n);
+ memset(proc_stat->n_nice, 0, n);
+ memset(proc_stat->n_sys, 0, n);
+ memset(proc_stat->n_idle, 0, n);
+ memset(proc_stat->n_wait, 0, n);
+ memset(proc_stat->n_irq, 0, n);
+ memset(proc_stat->n_sirq, 0, n);
+ memset(proc_stat->n_steal, 0, n);
+ memset(proc_stat->n_guest, 0, n);
+ }
/*
* cpu 95379 4 20053 6502503
* 2.6 kernels have 3 additional fields
commit f5eaeea7900390ae007f5ed3375cadc6ca86497e
Author: Nathan Scott <nathans@redhat.com>
Date: Thu Feb 13 14:20:52 2014 +1100
Fix s390x platform issues in /proc/cpuinfo parser
The format of /proc/cpuinfo on s390x is nothing like any other
platform. We cannot attempt to push it through the same code
path, it simply has nothing in common. If we try (as we used to)
things fall apart when we use the (negative) cpu number local
variable to index into the cpuinfo array.
Resolved by adding in some defensive code to short-circuit the
decoding logic when an unexpected file format is seen.
diff --git a/src/pmdas/linux/proc_cpuinfo.c b/src/pmdas/linux/proc_cpuinfo.c
index 6380bc2..d0aa52a 100644
--- a/src/pmdas/linux/proc_cpuinfo.c
+++ b/src/pmdas/linux/proc_cpuinfo.c
@@ -1,7 +1,7 @@
/*
* Linux /proc/cpuinfo metrics cluster
*
- * Copyright (c) 2013 Red Hat.
+ * Copyright (c) 2013-2014 Red Hat.
* Copyright (c) 2000-2005 Silicon Graphics, Inc. All Rights Reserved.
* Portions Copyright (c) 2001 Gilly Ran (gilly@exanet.com) - for the
* portions supporting the Alpha platform. All rights reserved.
@@ -185,6 +185,9 @@ refresh_proc_cpuinfo(proc_cpuinfo_t *proc_cpuinfo)
}
#endif
+ if (cpunum < 0 || cpunum >= proc_cpuinfo->cpuindom->it_numinst)
+ continue;
+
info = &proc_cpuinfo->cpuinfo[cpunum];
/* note: order is important due to strNcmp comparisons */