Idlestat: Plug memory leaks from inter() and topology

Function inter() may return newly allocated structures or one of the
parameters (if the other parameter is NULL). This function is called
in a loop from cluster_data(), core_cluster_data() and
physical_cluster_data() with one of the parameters often being an
earlier return value from inter(). This causes a great number of
structure allocations without matching memory releases.

Code in topology.c takes the result from *_cluster_data(), but it
only releases the top-level structure, without releasing the
data pointed to by top-level structure members.

This patch plugs both types of leaks. Since the memory pointed to
by inter() may or may not be allocated by that call, a new member
has been added to struct cpuidle_cstate to indicate the origin
of the structure.

This patch also adds detection for data aliasing related to inter(),
but does not remove the bug.

Signed-off-by: Tuukka Tikkanen <tuukka.tikkanen@linaro.org>

3 files changed, 82 insertions, 12 deletions

diff --git a/idlestat.c b/idlestat.c
index e4e6317..0457c9a 100644
--- a/idlestat.c
+++ b/idlestat.c
@@ -223,6 +223,11 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1,
 	struct cpuidle_data *data = NULL;
 	size_t index;
 
+	/*
+	 * Watch out! These may create aliases that end up stored
+	 * permanently! (e.g. if nrcpus == 1)
+	 * TODO: This is obviously a bug.
+	 */
 	if (!c1)
 		return c2;
 	if (!c2)
@@ -232,6 +237,8 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1,
 	if (!result)
 		return ptrerror(__func__);
 
+	result->inter_result = 1;
+
 	for (i = 0, index = 0; i < c1->nrdata; i++) {
 
 		for (j = index; j < c2->nrdata; j++) {
@@ -319,7 +326,7 @@ static char *cpuidle_cstate_name(int cpu, int state)
  * @cstates: per-cpu array of C-state statistics structs
  * @nrcpus: number of CPUs
  */
-static void release_cstate_info(struct cpuidle_cstates *cstates, int nrcpus)
+void release_cstate_info(struct cpuidle_cstates *cstates, int nrcpus)
 {
 	int cpu, i;
 
@@ -1103,7 +1110,7 @@ struct cpuidle_datas *idlestat_load(const char *filename)
 
 struct cpuidle_datas *cluster_data(struct cpuidle_datas *datas)
 {
-	struct cpuidle_cstate *c1, *cstates;
+	struct cpuidle_cstate *c1, *cstates, *tmp;
 	struct cpuidle_datas *result;
 	int i, j;
 	int cstate_max = -1;
@@ -1120,7 +1127,7 @@ struct cpuidle_datas *cluster_data(struct cpuidle_datas *datas)
 		return NULL;
 	}
 
-	/* hack but negligeable overhead */
+	/* hack but negligible overhead */
 	for (i = 0; i < datas->nrcpus; i++)
 		cstate_max = MAX(cstate_max, datas->cstates[i].cstate_max);
 	result->cstates[0].cstate_max = cstate_max;
@@ -1131,7 +1138,14 @@ struct cpuidle_datas *cluster_data(struct cpuidle_datas *datas)
 
 			c1 = &datas->cstates[j].cstate[i];
 
-			cstates = inter(cstates, c1);
+			tmp = inter(cstates, c1);
+			if (cstates && cstates != tmp &&
+				cstates->inter_result) {
+				free(cstates->data);
+				free(cstates);
+			}
+			cstates = tmp;
+
 			if (!cstates)
 				continue;
 			if (is_err(cstates)) {
@@ -1145,6 +1159,19 @@ struct cpuidle_datas *cluster_data(struct cpuidle_datas *datas)
 		cstates->name = strdup(datas->cstates[0].cstate[i].name);
 
 		result->cstates[0].cstate[i] = *cstates;
+		result->cstates[0].cstate[i].inter_result = 0;
+
+		/*
+		 * Free cstates if it has been allocated by inter()
+		 * Do not free things pointed to by members of cstates
+		 * even if you free cstates itself.
+		 */
+		if (cstates && cstates->inter_result) {
+			free(cstates);
+		} else {
+			fprintf(stderr, "Warning: %s aliased cstates at %p\n",
+				__func__, cstates);
+		}
 	}
 
 	return result;
@@ -1152,7 +1179,7 @@ struct cpuidle_datas *cluster_data(struct cpuidle_datas *datas)
 
 struct cpuidle_cstates *core_cluster_data(struct cpu_core *s_core)
 {
-	struct cpuidle_cstate *c1, *cstates;
+	struct cpuidle_cstate *c1, *cstates, *tmp;
 	struct cpuidle_cstates *result;
 	struct cpu_cpu      *s_cpu;
 	int i;
@@ -1166,7 +1193,7 @@ struct cpuidle_cstates *core_cluster_data(struct cpu_core *s_core)
 	if (!result)
 		return NULL;
 
-	/* hack but negligeable overhead */
+	/* hack but negligible overhead */
 	list_for_each_entry(s_cpu, &s_core->cpu_head, list_cpu)
 		cstate_max = MAX(cstate_max, s_cpu->cstates->cstate_max);
 	result->cstate_max = cstate_max;
@@ -1176,7 +1203,14 @@ struct cpuidle_cstates *core_cluster_data(struct cpu_core *s_core)
 		list_for_each_entry(s_cpu, &s_core->cpu_head, list_cpu) {
 			c1 = &s_cpu->cstates->cstate[i];
 
-			cstates = inter(cstates, c1);
+			tmp = inter(cstates, c1);
+			if (cstates && cstates != tmp &&
+				cstates->inter_result) {
+				free(cstates->data);
+				free(cstates);
+			}
+			cstates = tmp;
+
 			if (!cstates)
 				continue;
 			if (is_err(cstates)) {
@@ -1190,6 +1224,19 @@ struct cpuidle_cstates *core_cluster_data(struct cpu_core *s_core)
 		cstates->name = strdup(s_cpu->cstates->cstate[i].name);
 
 		result->cstate[i] = *cstates;
+		result->cstate[i].inter_result = 0;
+
+		/*
+		 * Free cstates if it has been allocated by inter()
+		 * Do not free things pointed to by members of cstates
+		 * even if you free cstates itself.
+		 */
+		if (cstates && cstates->inter_result) {
+			free(cstates);
+		} else {
+			fprintf(stderr, "Warning: %s aliased cstates at %p\n",
+				__func__, cstates);
+		}
 	}
 
 	return result;
@@ -1197,7 +1244,7 @@ struct cpuidle_cstates *core_cluster_data(struct cpu_core *s_core)
 
 struct cpuidle_cstates *physical_cluster_data(struct cpu_physical *s_phy)
 {
-	struct cpuidle_cstate *c1, *cstates;
+	struct cpuidle_cstate *c1, *cstates, *tmp;
 	struct cpuidle_cstates *result;
 	struct cpu_core      *s_core;
 	int i;
@@ -1207,7 +1254,7 @@ struct cpuidle_cstates *physical_cluster_data(struct cpu_physical *s_phy)
 	if (!result)
 		return NULL;
 
-	/* hack but negligeable overhead */
+	/* hack but negligible overhead */
 	list_for_each_entry(s_core, &s_phy->core_head, list_core)
 		cstate_max = MAX(cstate_max, s_core->cstates->cstate_max);
 	result->cstate_max = cstate_max;
@@ -1217,7 +1264,14 @@ struct cpuidle_cstates *physical_cluster_data(struct cpu_physical *s_phy)
 		list_for_each_entry(s_core, &s_phy->core_head, list_core) {
 			c1 = &s_core->cstates->cstate[i];
 
-			cstates = inter(cstates, c1);
+			tmp = inter(cstates, c1);
+			if (cstates && cstates != tmp &&
+				cstates->inter_result) {
+				free(cstates->data);
+				free(cstates);
+			}
+			cstates = tmp;
+
 			if (!cstates)
 				continue;
 			if (is_err(cstates)) {
@@ -1232,6 +1286,19 @@ struct cpuidle_cstates *physical_cluster_data(struct cpu_physical *s_phy)
 			strdup(s_core->cstates->cstate[i].name) : NULL;
 
 		result->cstate[i] = *cstates;
+		result->cstate[i].inter_result = 0;
+
+		/*
+		 * Free cstates if it has been allocated by inter()
+		 * Do not free things pointed to by members of cstates
+		 * even if you free cstates itself.
+		 */
+		if (cstates && cstates->inter_result) {
+			free(cstates);
+		} else {
+			fprintf(stderr, "Warning: %s aliased cstates at %p\n",
+				__func__, cstates);
+		}
 	}
 
 	return result;
diff --git a/idlestat.h b/idlestat.h
index a32aab9..253d2b9 100644
--- a/idlestat.h
+++ b/idlestat.h
@@ -63,6 +63,7 @@ struct cpuidle_cstate {
 	double min_time;
 	double duration;
 	int target_residency; /* -1 if not available */
+	int inter_result; /* 1 if created by a call to inter() */
 };
 
 struct wakeup_irq {
@@ -87,6 +88,8 @@ struct cpuidle_cstates {
 	enum {as_expected, too_long, too_short} actual_residency;
 };
 
+extern void release_cstate_info(struct cpuidle_cstates *cstates, int nrcpus);
+
 struct cpufreq_pstate {
 	int id;
 	unsigned int freq;
diff --git a/topology.c b/topology.c
index 316aec8..1cd4757 100644
--- a/topology.c
+++ b/topology.c
@@ -500,11 +500,11 @@ int release_cpu_topo_cstates(void)
 
 	list_for_each_entry(s_phy, &g_cpu_topo_list.physical_head,
 			    list_physical) {
-		free(s_phy->cstates);
+		release_cstate_info(s_phy->cstates, 1);
 		s_phy->cstates = NULL;
 		list_for_each_entry(s_core, &s_phy->core_head, list_core)
 			if (s_core->is_ht) {
-				free(s_core->cstates);
+				release_cstate_info(s_core->cstates, 1);
 				s_core->cstates = NULL;
 			}
 	}