commit 775bd386c2b9a6e553c61a5c0f86d29d9c2320a8
Author: Mark Goodwin <mgoodwin@redhat.com>
Date: Thu Jul 25 13:30:05 2019 +1000
pmdaproc: fix memory leak in pidlist refresh
RHBZ #1721107
The pidlist refresh function was leaking instance name buffers for
processes that had exited between refreshes. Fix this by storing the
instance name (truncated psargs) in a new field of the hash table
entry, and harvest them correctly if they've exited after each refresh.
The indom table instance names now share a pointer to the new field
in the hash table entry so they dont need to be separately free'd.
This similifies the code and memory management.
Also put a 4K cap on the maximum length of a psargs string for the
unlikely scenario of a psargs string that is not NULL terminated,
i.e. use strndup() rather than strdup(). This may be possible if
a rogue process modified it's argv[0] at run-time.
For testing, the recipe in RHBZ#1721107 was repro'd using valgind
and a workload with lots of process churn (i.e. a couple of large
builds running). Before the fix, valgrind reported numerous similar
leaks as reported in the bug. With the fix applied, those leaks
are gone. The refresh is more efficient now too because we only
have one copy of the psargs strings (in the hash table entry).
Also thoroughly exercised all tests in the QA pmda.proc and
pmda.hotproc groups. A new test might be possible, but the nature
of the required workload would make it difficult (and expensive)
to run.
diff --git a/src/pmdas/linux_proc/proc_pid.c b/src/pmdas/linux_proc/proc_pid.c
index 2e8f705b9..dcbc28850 100644
--- a/src/pmdas/linux_proc/proc_pid.c
+++ b/src/pmdas/linux_proc/proc_pid.c
@@ -820,7 +820,7 @@ disable_hotproc(void)
static void
refresh_proc_pidlist(proc_pid_t *proc_pid, proc_pid_list_t *pids)
{
- int i;
+ int i, numinst, idx;
int fd;
char *p;
char buf[MAXPATHLEN];
@@ -828,14 +828,6 @@ refresh_proc_pidlist(proc_pid_t *proc_pid, proc_pid_list_t *pids)
proc_pid_entry_t *ep;
pmdaIndom *indomp = proc_pid->indom;
- if (indomp->it_numinst < pids->count) {
- indomp->it_set = (pmdaInstid *)realloc(indomp->it_set,
- pids->count * sizeof(pmdaInstid));
- memset(&indomp->it_set[indomp->it_numinst], 0,
- (pids->count - indomp->it_numinst) * sizeof(pmdaInstid));
- }
- indomp->it_numinst = pids->count;
-
/*
* invalidate all entries so we can harvest pids that have exited
*/
@@ -852,7 +844,9 @@ refresh_proc_pidlist(proc_pid_t *proc_pid, proc_pid_list_t *pids)
*/
for (i=0; i < pids->count; i++) {
node = __pmHashSearch(pids->pids[i], &proc_pid->pidhash);
- if (node == NULL) {
+ if (node)
+ ep = (proc_pid_entry_t *)node->data;
+ else {
int k = 0;
ep = (proc_pid_entry_t *)malloc(sizeof(proc_pid_entry_t));
@@ -940,57 +934,52 @@ refresh_proc_pidlist(proc_pid_t *proc_pid, proc_pid_list_t *pids)
__pmHashAdd(pids->pids[i], (void *)ep, &proc_pid->pidhash);
//fprintf(stderr, "key %d : ADDED \"%s\" to hash table\n", pids->pids[i], buf);
}
- else
- ep = (proc_pid_entry_t *)node->data;
-
- /* mark pid as still existing */
- ep->flags |= PROC_PID_FLAG_VALID;
- /* refresh the indom pointer */
- indomp->it_set[i].i_inst = ep->id;
- if (indomp->it_set[i].i_name) {
- int len = strlen(indomp->it_set[i].i_name);
- if (strncmp(indomp->it_set[i].i_name, ep->name, len) != 0) {
- free(indomp->it_set[i].i_name);
- indomp->it_set[i].i_name = NULL;
- }
- else if (pmDebugOptions.libpmda) {
- fprintf(stderr, "refresh_proc_pidlist: Instance id=%d \"%s\" no change\n",
- ep->id, indomp->it_set[i].i_name);
- }
- }
- if (indomp->it_set[i].i_name == NULL) {
- /*
- * The external instance name is the pid followed by
- * a copy of the psargs truncated at the first space.
- * e.g. "012345 /path/to/command". Command line args,
- * if any, are truncated. The full command line is
- * available in the proc.psinfo.psargs metric.
- */
- if ((p = strchr(ep->name, ' ')) != NULL) {
- if ((p = strchr(p+1, ' ')) != NULL) {
- int len = p - ep->name;
- indomp->it_set[i].i_name = (char *)malloc(len+1);
- strncpy(indomp->it_set[i].i_name, ep->name, len);
- indomp->it_set[i].i_name[len] = '\0';
- }
- }
- if (indomp->it_set[i].i_name == NULL)
- indomp->it_set[i].i_name = strdup(ep->name);
+ if (ep->instname == NULL) {
+ /*
+ * The external instance name is the pid followed by
+ * a copy of the psargs truncated at the first space.
+ * e.g. "012345 /path/to/command". Command line args,
+ * if any, are truncated. The full command line is
+ * available in the proc.psinfo.psargs metric.
+ */
+ if ((p = strchr(ep->name, ' ')) != NULL) {
+ if ((p = strchr(p+1, ' ')) != NULL) {
+ int len = p - ep->name;
+ if (len > PROC_PID_STAT_CMD_MAXLEN)
+ len = PROC_PID_STAT_CMD_MAXLEN;
+ ep->instname = (char *)malloc(len+1);
+ strncpy(ep->instname, ep->name, len);
+ ep->instname[len] = '\0';
+ }
+ }
+ if (ep->instname == NULL) /* no spaces found, so use the full name */
+ ep->instname = strndup(ep->name, PROC_PID_STAT_CMD_MAXLEN);
}
+
+ /* mark pid as valid (new or still running) */
+ ep->flags |= PROC_PID_FLAG_VALID;
}
/*
- * harvest exited pids from the pid hash table
+ * harvest pids that have exit'ed
*/
+ numinst = 0;
for (i=0; i < proc_pid->pidhash.hsize; i++) {
for (prev=NULL, node=proc_pid->pidhash.hash[i]; node != NULL;) {
next = node->next;
ep = (proc_pid_entry_t *)node->data;
// fprintf(stderr, "CHECKING key=%d node=" PRINTF_P_PFX "%p prev=" PRINTF_P_PFX "%p next=" PRINTF_P_PFX "%p ep=" PRINTF_P_PFX "%p valid=%d\n",
// ep->id, node, prev, node->next, ep, ep->valid);
- if (!(ep->flags & PROC_PID_FLAG_VALID)) {
+ if (ep->flags & PROC_PID_FLAG_VALID) {
+ numinst++;
+ prev = node;
+ }
+ else {
+ // This process has exited.
//fprintf(stderr, "DELETED key=%d name=\"%s\"\n", ep->id, ep->name);
+ if (ep->instname != NULL)
+ free(ep->instname);
if (ep->name != NULL)
free(ep->name);
if (ep->stat_buf != NULL)
@@ -1017,13 +1006,25 @@ refresh_proc_pidlist(proc_pid_t *proc_pid, proc_pid_list_t *pids)
free(ep);
free(node);
}
- else {
- prev = node;
- }
if ((node = next) == NULL)
break;
}
}
+
+ /*
+ * At this point, the hash table contains only valid pids. Refresh the indom table,
+ * based on the updated process hash table. Indom table instance names are shared
+ * with their hash table entry, do not free!
+ */
+ indomp->it_numinst = numinst;
+ indomp->it_set = (pmdaInstid *)realloc(indomp->it_set, numinst * sizeof(pmdaInstid));
+ for (idx=0, i=0; i < proc_pid->pidhash.hsize; i++) {
+ for (node=proc_pid->pidhash.hash[i]; node != NULL; node=node->next, idx++) {
+ ep = (proc_pid_entry_t *)node->data;
+ indomp->it_set[idx].i_inst = ep->id; /* internal instid is pid */
+ indomp->it_set[idx].i_name = ep->instname; /* ptr ref, do not free */
+ }
+ }
}
int
diff --git a/src/pmdas/linux_proc/proc_pid.h b/src/pmdas/linux_proc/proc_pid.h
index 344f4efc0..582965b3a 100644
--- a/src/pmdas/linux_proc/proc_pid.h
+++ b/src/pmdas/linux_proc/proc_pid.h
@@ -20,6 +20,10 @@
#include "proc_runq.h"
#include "hotproc.h"
+/*
+ * Maximim length of psargs and proc instance name.
+ */
+#define PROC_PID_STAT_CMD_MAXLEN 4096
/*
* /proc/<pid>/stat metrics
@@ -252,7 +256,8 @@ enum {
typedef struct {
int id; /* pid, hash key and internal instance id */
int flags; /* combinations of PROC_PID_FLAG_* values */
- char *name; /* external instance name (<pid> cmdline) */
+ char *name; /* full command line and args */
+ char *instname; /* external instance name (truncated <pid> cmdline) */
/* /proc/<pid>/stat cluster */
int stat_buflen;