[PATCH v1 0/8] Address/leak sanitizer clean ups

[PATCH v1 0/8] Address/leak sanitizer clean ups

[Ian Rogers]

Remove unnecessary reference counts for structs with no gets.  Add
reference count checking to comm_str and mem_info.  Fix memory leaks
and errors detected on "perf mem report" by address sanitizer and leak
sanitizer.

Ian Rogers (8):
  perf ui browser: Don't save pointer to stack memory
  perf annotate: Fix memory leak in annotated_source
  perf block-info: Remove unused refcount
  perf cpumap: Remove refcnt from cpu_aggr_map
  perf comm: Add reference count checking to comm_str
  perf mem-info: Move mem-info out of mem-events and symbol
  perf mem-info: Add reference count checking
  perf hist: Avoid hist_entry_iter mem_info memory leak

 tools/perf/builtin-c2c.c                      |  13 +-
 tools/perf/builtin-report.c                   |   3 +-
 tools/perf/builtin-script.c                   |  12 +-
 tools/perf/builtin-stat.c                     |  16 +-
 tools/perf/tests/mem.c                        |  11 +-
 tools/perf/ui/browser.c                       |   4 +-
 tools/perf/ui/browser.h                       |   2 +-
 tools/perf/util/Build                         |   1 +
 tools/perf/util/annotate.c                    |   6 +
 tools/perf/util/block-info.c                  |  22 +-
 tools/perf/util/block-info.h                  |  15 +-
 tools/perf/util/comm.c                        | 196 +++++++++++-------
 tools/perf/util/cpumap.c                      |   2 -
 tools/perf/util/cpumap.h                      |   2 -
 tools/perf/util/hist.c                        |  62 +++---
 tools/perf/util/hist.h                        |   8 +-
 tools/perf/util/machine.c                     |   7 +-
 tools/perf/util/mem-events.c                  |  36 ++--
 tools/perf/util/mem-events.h                  |  29 +--
 tools/perf/util/mem-info.c                    |  35 ++++
 tools/perf/util/mem-info.h                    |  54 +++++
 .../scripting-engines/trace-event-python.c    |  12 +-
 tools/perf/util/sort.c                        |  69 +++---
 tools/perf/util/symbol.c                      |  26 +--
 tools/perf/util/symbol.h                      |  12 --
 25 files changed, 370 insertions(+), 285 deletions(-)
 create mode 100644 tools/perf/util/mem-info.c
 create mode 100644 tools/perf/util/mem-info.h

-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Ian Rogers]

ui_browser__show is capturing the input title that is stack allocated
memory in hist_browser__run. Avoid a use after return by strdup-ing
the string.

Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browser.c | 4 +++-
 tools/perf/ui/browser.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index 603d11283cbd..c4cdf2ea69b7 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
 	mutex_lock(&ui__lock);
 	__ui_browser__show_title(browser, title);
 
-	browser->title = title;
+	free(browser->title);
+	browser->title = strdup(title);
 	zfree(&browser->helpline);
 
 	va_start(ap, helpline);
@@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
 	mutex_lock(&ui__lock);
 	ui_helpline__pop();
 	zfree(&browser->helpline);
+	zfree(&browser->title);
 	mutex_unlock(&ui__lock);
 }
 
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index 510ce4554050..6e98d5f8f71c 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -21,7 +21,7 @@ struct ui_browser {
 	u8	      extra_title_lines;
 	int	      current_color;
 	void	      *priv;
-	const char    *title;
+	char	      *title;
 	char	      *helpline;
 	const char    *no_samples_msg;
 	void 	      (*refresh_dimensions)(struct ui_browser *browser);
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 2/8] perf annotate: Fix memory leak in annotated_source

[Ian Rogers]

Freeing hash map doesn't free the entries added to the hashmap, add
missing free.

Fixes: d3e7cad6f36d ("perf annotate: Add a hashmap for symbol histogram")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/annotate.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index d7d55263fc91..a83722f32d6b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -107,9 +107,15 @@ static struct annotated_source *annotated_source__new(void)
 
 static __maybe_unused void annotated_source__delete(struct annotated_source *src)
 {
+	struct hashmap_entry *cur;
+	size_t bkt;
+
 	if (src == NULL)
 		return;
 
+	hashmap__for_each_entry(src->samples, cur, bkt)
+		zfree(&cur->pvalue);
+
 	hashmap__free(src->samples);
 	zfree(&src->histograms);
 	free(src);
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 3/8] perf block-info: Remove unused refcount

[Ian Rogers]

block_info__get has no callers so the refcount is only ever one. As
such remove the reference counting logic and turn puts to deletes.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/block-info.c | 22 +++++-----------------
 tools/perf/util/block-info.h | 15 +--------------
 tools/perf/util/hist.c       |  4 ++--
 3 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 895ee8adf3b3..04068d48683f 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -43,26 +43,14 @@ static struct block_header_column {
 	}
 };
 
-struct block_info *block_info__get(struct block_info *bi)
-{
-	if (bi)
-		refcount_inc(&bi->refcnt);
-	return bi;
-}
-
-void block_info__put(struct block_info *bi)
+struct block_info *block_info__new(void)
 {
-	if (bi && refcount_dec_and_test(&bi->refcnt))
-		free(bi);
+	return zalloc(sizeof(struct block_info));
 }
 
-struct block_info *block_info__new(void)
+void block_info__delete(struct block_info *bi)
 {
-	struct block_info *bi = zalloc(sizeof(*bi));
-
-	if (bi)
-		refcount_set(&bi->refcnt, 1);
-	return bi;
+	free(bi);
 }
 
 int64_t __block_info__cmp(struct hist_entry *left, struct hist_entry *right)
@@ -148,7 +136,7 @@ int block_info__process_sym(struct hist_entry *he, struct block_hist *bh,
 			he_block = hists__add_entry_block(&bh->block_hists,
 							  &al, bi);
 			if (!he_block) {
-				block_info__put(bi);
+				block_info__delete(bi);
 				return -1;
 			}
 		}
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index 96f53e89795e..0b9e1aad4c55 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -3,7 +3,6 @@
 #define __PERF_BLOCK_H
 
 #include <linux/types.h>
-#include <linux/refcount.h>
 #include "hist.h"
 #include "symbol.h"
 #include "sort.h"
@@ -19,7 +18,6 @@ struct block_info {
 	u64			total_cycles;
 	int			num;
 	int			num_aggr;
-	refcount_t		refcnt;
 };
 
 struct block_fmt {
@@ -48,19 +46,8 @@ struct block_report {
 	int			nr_fmts;
 };
 
-struct block_hist;
-
 struct block_info *block_info__new(void);
-struct block_info *block_info__get(struct block_info *bi);
-void   block_info__put(struct block_info *bi);
-
-static inline void __block_info__zput(struct block_info **bi)
-{
-	block_info__put(*bi);
-	*bi = NULL;
-}
-
-#define block_info__zput(bi) __block_info__zput(&bi)
+void block_info__delete(struct block_info *bi);
 
 int64_t __block_info__cmp(struct hist_entry *left, struct hist_entry *right);
 
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 55ea6afcc437..b8a508cd0b14 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -631,7 +631,7 @@ static struct hist_entry *hists__findnew_entry(struct hists *hists,
 			 */
 			mem_info__zput(entry->mem_info);
 
-			block_info__zput(entry->block_info);
+			block_info__delete(entry->block_info);
 
 			kvm_info__zput(entry->kvm_info);
 
@@ -1341,7 +1341,7 @@ void hist_entry__delete(struct hist_entry *he)
 	}
 
 	if (he->block_info)
-		block_info__zput(he->block_info);
+		block_info__delete(he->block_info);
 
 	if (he->kvm_info)
 		kvm_info__zput(he->kvm_info);
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 4/8] perf cpumap: Remove refcnt from cpu_aggr_map

[Ian Rogers]

It is assigned a value of 1 and never incremented. Remove and replace
puts with delete.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-stat.c | 16 +++-------------
 tools/perf/util/cpumap.c  |  2 --
 tools/perf/util/cpumap.h  |  2 --
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 65a3dd7ffac3..35f79b48e8dc 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1631,23 +1631,13 @@ static int perf_stat_init_aggr_mode(void)
 
 static void cpu_aggr_map__delete(struct cpu_aggr_map *map)
 {
-	if (map) {
-		WARN_ONCE(refcount_read(&map->refcnt) != 0,
-			  "cpu_aggr_map refcnt unbalanced\n");
-		free(map);
-	}
-}
-
-static void cpu_aggr_map__put(struct cpu_aggr_map *map)
-{
-	if (map && refcount_dec_and_test(&map->refcnt))
-		cpu_aggr_map__delete(map);
+	free(map);
 }
 
 static void perf_stat__exit_aggr_mode(void)
 {
-	cpu_aggr_map__put(stat_config.aggr_map);
-	cpu_aggr_map__put(stat_config.cpus_aggr_map);
+	cpu_aggr_map__delete(stat_config.aggr_map);
+	cpu_aggr_map__delete(stat_config.cpus_aggr_map);
 	stat_config.aggr_map = NULL;
 	stat_config.cpus_aggr_map = NULL;
 }
diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 6a270d640acb..27094211edd8 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -180,8 +180,6 @@ struct cpu_aggr_map *cpu_aggr_map__empty_new(int nr)
 		cpus->nr = nr;
 		for (i = 0; i < nr; i++)
 			cpus->map[i] = aggr_cpu_id__empty();
-
-		refcount_set(&cpus->refcnt, 1);
 	}
 
 	return cpus;
diff --git a/tools/perf/util/cpumap.h b/tools/perf/util/cpumap.h
index 26cf76c693f5..ee0f6139b04a 100644
--- a/tools/perf/util/cpumap.h
+++ b/tools/perf/util/cpumap.h
@@ -5,7 +5,6 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <perf/cpumap.h>
-#include <linux/refcount.h>
 
 /** Identify where counts are aggregated, -1 implies not to aggregate. */
 struct aggr_cpu_id {
@@ -37,7 +36,6 @@ struct aggr_cpu_id {
 
 /** A collection of aggr_cpu_id values, the "built" version is sorted and uniqued. */
 struct cpu_aggr_map {
-	refcount_t refcnt;
 	/** Number of valid entries. */
 	int nr;
 	/** The entries. */
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 5/8] perf comm: Add reference count checking to comm_str

[Ian Rogers]

Reference count checking of an rbtree is troublesome as each pointer
should have a reference, switch to using a sorted array. Remove an
indirection by embedding the reference count with the string. Use
pthread_once to safely initialize the comm_strs and reader writer
mutex.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/comm.c | 196 ++++++++++++++++++++++++++---------------
 1 file changed, 126 insertions(+), 70 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index afb8d4fd2644..1aa9a08e5b03 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -1,108 +1,164 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "comm.h"
 #include <errno.h>
-#include <stdlib.h>
-#include <stdio.h>
 #include <string.h>
+#include <internal/rc_check.h>
 #include <linux/refcount.h>
-#include <linux/rbtree.h>
 #include <linux/zalloc.h>
 #include "rwsem.h"
 
-struct comm_str {
-	char *str;
-	struct rb_node rb_node;
+DECLARE_RC_STRUCT(comm_str) {
 	refcount_t refcnt;
+	char str[];
 };
 
-/* Should perhaps be moved to struct machine */
-static struct rb_root comm_str_root;
-static struct rw_semaphore comm_str_lock = {.lock = PTHREAD_RWLOCK_INITIALIZER,};
+static struct comm_strs {
+	struct rw_semaphore lock;
+	struct comm_str **strs;
+	int num_strs;
+	int capacity;
+} _comm_strs;
+
+static void comm_strs__init(void)
+{
+	init_rwsem(&_comm_strs.lock);
+	_comm_strs.capacity = 16;
+	_comm_strs.num_strs = 0;
+	_comm_strs.strs = calloc(16, sizeof(*_comm_strs.strs));
+}
+
+static struct comm_strs *comm_strs__get(void)
+{
+	static pthread_once_t comm_strs_type_once = PTHREAD_ONCE_INIT;
+
+	pthread_once(&comm_strs_type_once, comm_strs__init);
+
+	return &_comm_strs;
+}
+
+static refcount_t *comm_str__refcnt(struct comm_str *cs)
+{
+	return &RC_CHK_ACCESS(cs)->refcnt;
+}
+
+static const char *comm_str__str(const struct comm_str *cs)
+{
+	return &RC_CHK_ACCESS(cs)->str[0];
+}
 
 static struct comm_str *comm_str__get(struct comm_str *cs)
 {
-	if (cs && refcount_inc_not_zero(&cs->refcnt))
-		return cs;
+	struct comm_str *result;
+
+	if (RC_CHK_GET(result, cs))
+		refcount_inc_not_zero(comm_str__refcnt(cs));
 
-	return NULL;
+	return result;
 }
 
 static void comm_str__put(struct comm_str *cs)
 {
-	if (cs && refcount_dec_and_test(&cs->refcnt)) {
-		down_write(&comm_str_lock);
-		rb_erase(&cs->rb_node, &comm_str_root);
-		up_write(&comm_str_lock);
-		zfree(&cs->str);
-		free(cs);
+	if (cs && refcount_dec_and_test(comm_str__refcnt(cs))) {
+		struct comm_strs *comm_strs = comm_strs__get();
+		int i;
+
+		down_write(&comm_strs->lock);
+		for (i = 0; i < comm_strs->num_strs; i++) {
+			if (comm_strs->strs[i] == cs)
+				break;
+		}
+		for (; i < comm_strs->num_strs - 1; i++)
+			comm_strs->strs[i] = comm_strs->strs[i + 1];
+
+		comm_strs->num_strs--;
+		up_write(&comm_strs->lock);
+		RC_CHK_FREE(cs);
+	} else {
+		RC_CHK_PUT(cs);
 	}
 }
 
-static struct comm_str *comm_str__alloc(const char *str)
+static struct comm_str *comm_str__new(const char *str)
 {
-	struct comm_str *cs;
+	struct comm_str *result = NULL;
+	RC_STRUCT(comm_str) *cs;
 
-	cs = zalloc(sizeof(*cs));
-	if (!cs)
-		return NULL;
-
-	cs->str = strdup(str);
-	if (!cs->str) {
-		free(cs);
-		return NULL;
+	cs = malloc(sizeof(*cs) + strlen(str) + 1);
+	if (ADD_RC_CHK(result, cs)) {
+		refcount_set(comm_str__refcnt(result), 1);
+		strcpy(&cs->str[0], str);
 	}
+	return result;
+}
 
-	refcount_set(&cs->refcnt, 1);
+static int comm_str__cmp(const void *_lhs, const void *_rhs)
+{
+	const struct comm_str *lhs = *(const struct comm_str * const *)_lhs;
+	const struct comm_str *rhs = *(const struct comm_str * const *)_rhs;
 
-	return cs;
+	return strcmp(comm_str__str(lhs), comm_str__str(rhs));
 }
 
-static
-struct comm_str *__comm_str__findnew(const char *str, struct rb_root *root)
+static int comm_str__search(const void *_key, const void *_member)
 {
-	struct rb_node **p = &root->rb_node;
-	struct rb_node *parent = NULL;
-	struct comm_str *iter, *new;
-	int cmp;
-
-	while (*p != NULL) {
-		parent = *p;
-		iter = rb_entry(parent, struct comm_str, rb_node);
-
-		/*
-		 * If we race with comm_str__put, iter->refcnt is 0
-		 * and it will be removed within comm_str__put call
-		 * shortly, ignore it in this search.
-		 */
-		cmp = strcmp(str, iter->str);
-		if (!cmp && comm_str__get(iter))
-			return iter;
-
-		if (cmp < 0)
-			p = &(*p)->rb_left;
-		else
-			p = &(*p)->rb_right;
-	}
+	const char *key = _key;
+	const struct comm_str *member = *(const struct comm_str * const *)_member;
 
-	new = comm_str__alloc(str);
-	if (!new)
-		return NULL;
+	return strcmp(key, comm_str__str(member));
+}
+
+static struct comm_str *__comm_strs__find(struct comm_strs *comm_strs, const char *str)
+{
+	struct comm_str **result;
 
-	rb_link_node(&new->rb_node, parent, p);
-	rb_insert_color(&new->rb_node, root);
+	result = bsearch(str, comm_strs->strs, comm_strs->num_strs, sizeof(struct comm_str *),
+			 comm_str__search);
 
-	return new;
+	if (!result)
+		return NULL;
+
+	return comm_str__get(*result);
 }
 
-static struct comm_str *comm_str__findnew(const char *str, struct rb_root *root)
+static struct comm_str *comm_strs__findnew(const char *str)
 {
-	struct comm_str *cs;
+	struct comm_strs *comm_strs = comm_strs__get();
+	struct comm_str *result;
 
-	down_write(&comm_str_lock);
-	cs = __comm_str__findnew(str, root);
-	up_write(&comm_str_lock);
+	if (!comm_strs)
+		return NULL;
 
-	return cs;
+	down_read(&comm_strs->lock);
+	result = __comm_strs__find(comm_strs, str);
+	up_read(&comm_strs->lock);
+	if (result)
+		return result;
+
+	down_write(&comm_strs->lock);
+	result = __comm_strs__find(comm_strs, str);
+	if (!result) {
+		if (comm_strs->num_strs == comm_strs->capacity) {
+			struct comm_str **tmp;
+
+			tmp = reallocarray(comm_strs->strs,
+					   comm_strs->capacity + 16,
+					   sizeof(*comm_strs->strs));
+			if (!tmp) {
+				up_write(&comm_strs->lock);
+				return NULL;
+			}
+			comm_strs->strs = tmp;
+			comm_strs->capacity += 16;
+		}
+		result = comm_str__new(str);
+		if (result) {
+			comm_strs->strs[comm_strs->num_strs++] = result;
+			qsort(comm_strs->strs, comm_strs->num_strs, sizeof(struct comm_str *),
+			      comm_str__cmp);
+		}
+	}
+	up_write(&comm_strs->lock);
+	return result;
 }
 
 struct comm *comm__new(const char *str, u64 timestamp, bool exec)
@@ -115,7 +171,7 @@ struct comm *comm__new(const char *str, u64 timestamp, bool exec)
 	comm->start = timestamp;
 	comm->exec = exec;
 
-	comm->comm_str = comm_str__findnew(str, &comm_str_root);
+	comm->comm_str = comm_strs__findnew(str);
 	if (!comm->comm_str) {
 		free(comm);
 		return NULL;
@@ -128,7 +184,7 @@ int comm__override(struct comm *comm, const char *str, u64 timestamp, bool exec)
 {
 	struct comm_str *new, *old = comm->comm_str;
 
-	new = comm_str__findnew(str, &comm_str_root);
+	new = comm_strs__findnew(str);
 	if (!new)
 		return -ENOMEM;
 
@@ -149,5 +205,5 @@ void comm__free(struct comm *comm)
 
 const char *comm__str(const struct comm *comm)
 {
-	return comm->comm_str->str;
+	return comm_str__str(comm->comm_str);
 }
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 6/8] perf mem-info: Move mem-info out of mem-events and symbol

[Ian Rogers]

Move mem-info to its own header rather than having it split between
mem-events and symbol.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-c2c.c                      |  1 +
 tools/perf/builtin-report.c                   |  1 +
 tools/perf/builtin-script.c                   |  1 +
 tools/perf/tests/mem.c                        |  1 +
 tools/perf/util/Build                         |  1 +
 tools/perf/util/hist.c                        |  1 +
 tools/perf/util/machine.c                     |  1 +
 tools/perf/util/mem-events.c                  | 16 +++++-----
 tools/perf/util/mem-events.h                  | 29 +++++++------------
 tools/perf/util/mem-info.c                    | 28 ++++++++++++++++++
 tools/perf/util/mem-info.h                    | 28 ++++++++++++++++++
 .../scripting-engines/trace-event-python.c    |  1 +
 tools/perf/util/sort.c                        |  1 +
 tools/perf/util/symbol.c                      | 26 +----------------
 tools/perf/util/symbol.h                      | 12 --------
 15 files changed, 85 insertions(+), 63 deletions(-)
 create mode 100644 tools/perf/util/mem-info.c
 create mode 100644 tools/perf/util/mem-info.h

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 1f1d17df9b9a..c624414b2285 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -38,6 +38,7 @@
 #include "ui/browsers/hists.h"
 #include "thread.h"
 #include "mem2node.h"
+#include "mem-info.h"
 #include "symbol.h"
 #include "ui/ui.h"
 #include "ui/progress.h"
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b5525f4f7090..6eb1d90b589f 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -31,6 +31,7 @@
 #include "util/evsel.h"
 #include "util/evswitch.h"
 #include "util/header.h"
+#include "util/mem-info.h"
 #include "util/session.h"
 #include "util/srcline.h"
 #include "util/tool.h"
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index f7c3c3868c3c..58af4f3aa592 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -32,6 +32,7 @@
 #include "util/time-utils.h"
 #include "util/path.h"
 #include "util/event.h"
+#include "util/mem-info.h"
 #include "ui/ui.h"
 #include "print_binary.h"
 #include "print_insn.h"
diff --git a/tools/perf/tests/mem.c b/tools/perf/tests/mem.c
index 56014ec7d49d..f694a60d8a73 100644
--- a/tools/perf/tests/mem.c
+++ b/tools/perf/tests/mem.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "util/map_symbol.h"
 #include "util/mem-events.h"
+#include "util/mem-info.h"
 #include "util/symbol.h"
 #include "linux/perf_event.h"
 #include "util/debug.h"
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 292170a99ab6..da64efd8718f 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -141,6 +141,7 @@ perf-y += term.o
 perf-y += help-unknown-cmd.o
 perf-y += dlfilter.o
 perf-y += mem-events.o
+perf-y += mem-info.o
 perf-y += vsprintf.o
 perf-y += units.o
 perf-y += time-utils.o
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b8a508cd0b14..56453a02cdf4 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -9,6 +9,7 @@
 #include "map_symbol.h"
 #include "branch.h"
 #include "mem-events.h"
+#include "mem-info.h"
 #include "session.h"
 #include "namespaces.h"
 #include "cgroup.h"
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index a3ff2ab154bd..d5a01ef92668 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -16,6 +16,7 @@
 #include "map_symbol.h"
 #include "branch.h"
 #include "mem-events.h"
+#include "mem-info.h"
 #include "path.h"
 #include "srcline.h"
 #include "symbol.h"
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index 637cbd4a7bfb..f618a5ccc7af 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -10,7 +10,9 @@
 #include <linux/kernel.h>
 #include "map_symbol.h"
 #include "mem-events.h"
+#include "mem-info.h"
 #include "debug.h"
+#include "evsel.h"
 #include "symbol.h"
 #include "pmu.h"
 #include "pmus.h"
@@ -281,7 +283,7 @@ static const char * const tlb_access[] = {
 	"Fault",
 };
 
-int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	size_t l = 0, i;
 	u64 m = PERF_MEM_TLB_NA;
@@ -359,7 +361,7 @@ static const char * const mem_hops[] = {
 	"board",
 };
 
-static int perf_mem__op_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+static int perf_mem__op_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	u64 op = PERF_MEM_LOCK_NA;
 	int l;
@@ -383,7 +385,7 @@ static int perf_mem__op_scnprintf(char *out, size_t sz, struct mem_info *mem_inf
 	return l;
 }
 
-int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	union perf_mem_data_src data_src;
 	int printed = 0;
@@ -465,7 +467,7 @@ static const char * const snoopx_access[] = {
 	"Peer",
 };
 
-int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	size_t i, l = 0;
 	u64 m = PERF_MEM_SNOOP_NA;
@@ -507,7 +509,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
 	return l;
 }
 
-int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	u64 mask = PERF_MEM_LOCK_NA;
 	int l;
@@ -525,7 +527,7 @@ int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
 	return l;
 }
 
-int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	size_t l = 0;
 	u64 mask = PERF_MEM_BLK_NA;
@@ -548,7 +550,7 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
 	return l;
 }
 
-int perf_script__meminfo_scnprintf(char *out, size_t sz, struct mem_info *mem_info)
+int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *mem_info)
 {
 	int i = 0;
 
diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
index 15d5f0320d27..ca31014d7934 100644
--- a/tools/perf/util/mem-events.h
+++ b/tools/perf/util/mem-events.h
@@ -3,13 +3,7 @@
 #define __PERF_MEM_EVENTS_H
 
 #include <stdbool.h>
-#include <stdint.h>
-#include <stdio.h>
 #include <linux/types.h>
-#include <linux/refcount.h>
-#include <linux/perf_event.h>
-#include "stat.h"
-#include "evsel.h"
 
 struct perf_mem_event {
 	bool		record;
@@ -21,13 +15,6 @@ struct perf_mem_event {
 	const char	*event_name;
 };
 
-struct mem_info {
-	struct addr_map_symbol	iaddr;
-	struct addr_map_symbol	daddr;
-	union perf_mem_data_src	data_src;
-	refcount_t		refcnt;
-};
-
 enum {
 	PERF_MEM_EVENTS__LOAD,
 	PERF_MEM_EVENTS__STORE,
@@ -35,6 +22,10 @@ enum {
 	PERF_MEM_EVENTS__MAX,
 };
 
+struct evsel;
+struct mem_info;
+struct perf_pmu;
+
 extern unsigned int perf_mem_events__loads_ldlat;
 extern struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX];
 
@@ -49,13 +40,13 @@ bool is_mem_loads_aux_event(struct evsel *leader);
 void perf_pmu__mem_events_list(struct perf_pmu *pmu);
 int perf_mem_events__record_args(const char **rec_argv, int *argv_nr);
 
-int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__snp_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__lck_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-int perf_mem__blk_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
+int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
+int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_info);
 
-int perf_script__meminfo_scnprintf(char *bf, size_t size, struct mem_info *mem_info);
+int perf_script__meminfo_scnprintf(char *bf, size_t size, const struct mem_info *mem_info);
 
 struct c2c_stats {
 	u32	nr_entries;
diff --git a/tools/perf/util/mem-info.c b/tools/perf/util/mem-info.c
new file mode 100644
index 000000000000..ff0dfdb5369a
--- /dev/null
+++ b/tools/perf/util/mem-info.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/zalloc.h>
+#include "mem-info.h"
+
+struct mem_info *mem_info__get(struct mem_info *mi)
+{
+	if (mi)
+		refcount_inc(&mi->refcnt);
+	return mi;
+}
+
+void mem_info__put(struct mem_info *mi)
+{
+	if (mi && refcount_dec_and_test(&mi->refcnt)) {
+		addr_map_symbol__exit(&mi->iaddr);
+		addr_map_symbol__exit(&mi->daddr);
+		free(mi);
+	}
+}
+
+struct mem_info *mem_info__new(void)
+{
+	struct mem_info *mi = zalloc(sizeof(*mi));
+
+	if (mi)
+		refcount_set(&mi->refcnt, 1);
+	return mi;
+}
diff --git a/tools/perf/util/mem-info.h b/tools/perf/util/mem-info.h
new file mode 100644
index 000000000000..bb7d8375d73c
--- /dev/null
+++ b/tools/perf/util/mem-info.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __PERF_MEM_INFO_H
+#define __PERF_MEM_INFO_H
+
+#include <linux/refcount.h>
+#include <linux/perf_event.h>
+#include "map_symbol.h"
+
+struct mem_info {
+	struct addr_map_symbol	iaddr;
+	struct addr_map_symbol	daddr;
+	union perf_mem_data_src	data_src;
+	refcount_t		refcnt;
+};
+
+struct mem_info *mem_info__new(void);
+struct mem_info *mem_info__get(struct mem_info *mi);
+void   mem_info__put(struct mem_info *mi);
+
+static inline void __mem_info__zput(struct mem_info **mi)
+{
+	mem_info__put(*mi);
+	*mi = NULL;
+}
+
+#define mem_info__zput(mi) __mem_info__zput(&mi)
+
+#endif /* __PERF_MEM_INFO_H */
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c2caa5720299..c77fe08a0545 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -45,6 +45,7 @@
 #include "../thread.h"
 #include "../comm.h"
 #include "../machine.h"
+#include "../mem-info.h"
 #include "../db-export.h"
 #include "../thread-stack.h"
 #include "../trace-event.h"
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 704664e5b4ea..711ef69306f3 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -23,6 +23,7 @@
 #include "strlist.h"
 #include "strbuf.h"
 #include "mem-events.h"
+#include "mem-info.h"
 #include "annotate.h"
 #include "annotate-data.h"
 #include "event.h"
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 7772a4d3e66c..eb3319baa1b5 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -27,6 +27,7 @@
 #include "symbol.h"
 #include "map_symbol.h"
 #include "mem-events.h"
+#include "mem-info.h"
 #include "symsrc.h"
 #include "strlist.h"
 #include "intlist.h"
@@ -2570,31 +2571,6 @@ int symbol__config_symfs(const struct option *opt __maybe_unused,
 	return 0;
 }
 
-struct mem_info *mem_info__get(struct mem_info *mi)
-{
-	if (mi)
-		refcount_inc(&mi->refcnt);
-	return mi;
-}
-
-void mem_info__put(struct mem_info *mi)
-{
-	if (mi && refcount_dec_and_test(&mi->refcnt)) {
-		addr_map_symbol__exit(&mi->iaddr);
-		addr_map_symbol__exit(&mi->daddr);
-		free(mi);
-	}
-}
-
-struct mem_info *mem_info__new(void)
-{
-	struct mem_info *mi = zalloc(sizeof(*mi));
-
-	if (mi)
-		refcount_set(&mi->refcnt, 1);
-	return mi;
-}
-
 /*
  * Checks that user supplied symbol kernel files are accessible because
  * the default mechanism for accessing elf files fails silently. i.e. if
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 071837ddce2a..3fb5d146d9b1 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -268,18 +268,6 @@ enum {
 	SDT_NOTE_IDX_REFCTR,
 };
 
-struct mem_info *mem_info__new(void);
-struct mem_info *mem_info__get(struct mem_info *mi);
-void   mem_info__put(struct mem_info *mi);
-
-static inline void __mem_info__zput(struct mem_info **mi)
-{
-	mem_info__put(*mi);
-	*mi = NULL;
-}
-
-#define mem_info__zput(mi) __mem_info__zput(&mi)
-
 int symbol__validate_sym_arguments(void);
 
 #endif /* __PERF_SYMBOL */
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 7/8] perf mem-info: Add reference count checking

[Ian Rogers]

Add reference count checking and switch struct mem_info usage to use
accessor functions.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-c2c.c                      | 12 ++--
 tools/perf/builtin-report.c                   |  2 +-
 tools/perf/builtin-script.c                   | 11 ++-
 tools/perf/tests/mem.c                        | 10 +--
 tools/perf/util/hist.c                        | 26 +++----
 tools/perf/util/machine.c                     |  6 +-
 tools/perf/util/mem-events.c                  | 20 +++---
 tools/perf/util/mem-info.c                    | 29 +++++---
 tools/perf/util/mem-info.h                    | 28 +++++++-
 .../scripting-engines/trace-event-python.c    | 11 ++-
 tools/perf/util/sort.c                        | 68 +++++++++----------
 11 files changed, 135 insertions(+), 88 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c624414b2285..c157bd31f2e5 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -530,7 +530,7 @@ static int dcacheline_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 	char buf[20];
 
 	if (he->mem_info)
-		addr = cl_address(he->mem_info->daddr.addr, chk_double_cl);
+		addr = cl_address(mem_info__daddr(he->mem_info)->addr, chk_double_cl);
 
 	return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
 }
@@ -568,7 +568,7 @@ static int offset_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 	char buf[20];
 
 	if (he->mem_info)
-		addr = cl_offset(he->mem_info->daddr.al_addr, chk_double_cl);
+		addr = cl_offset(mem_info__daddr(he->mem_info)->al_addr, chk_double_cl);
 
 	return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
 }
@@ -580,10 +580,10 @@ offset_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
 	uint64_t l = 0, r = 0;
 
 	if (left->mem_info)
-		l = cl_offset(left->mem_info->daddr.addr, chk_double_cl);
+		l = cl_offset(mem_info__daddr(left->mem_info)->addr, chk_double_cl);
 
 	if (right->mem_info)
-		r = cl_offset(right->mem_info->daddr.addr, chk_double_cl);
+		r = cl_offset(mem_info__daddr(right->mem_info)->addr, chk_double_cl);
 
 	return (int64_t)(r - l);
 }
@@ -597,7 +597,7 @@ iaddr_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
 	char buf[20];
 
 	if (he->mem_info)
-		addr = he->mem_info->iaddr.addr;
+		addr = mem_info__iaddr(he->mem_info)->addr;
 
 	return scnprintf(hpp->buf, hpp->size, "%*s", width, HEX_STR(buf, addr));
 }
@@ -2593,7 +2593,7 @@ perf_c2c_cacheline_browser__title(struct hist_browser *browser,
 	he = cl_browser->he;
 
 	if (he->mem_info)
-		addr = cl_address(he->mem_info->daddr.addr, chk_double_cl);
+		addr = cl_address(mem_info__daddr(he->mem_info)->addr, chk_double_cl);
 
 	scnprintf(bf, size, "Cacheline 0x%lx", addr);
 	return 0;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6eb1d90b589f..0892b6e3e5e7 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -186,7 +186,7 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
 
 	} else if (rep->mem_mode) {
 		mi = he->mem_info;
-		err = addr_map_symbol__inc_samples(&mi->daddr, sample, evsel);
+		err = addr_map_symbol__inc_samples(mem_info__daddr(mi), sample, evsel);
 		if (err)
 			goto out;
 
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 58af4f3aa592..c16224b1fef3 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2051,13 +2051,18 @@ static int evlist__max_name_len(struct evlist *evlist)
 
 static int data_src__fprintf(u64 data_src, FILE *fp)
 {
-	struct mem_info mi = { .data_src.val = data_src };
+	struct mem_info *mi = mem_info__new();
 	char decode[100];
 	char out[100];
 	static int maxlen;
 	int len;
 
-	perf_script__meminfo_scnprintf(decode, 100, &mi);
+	if (!mi)
+		return -ENOMEM;
+
+	mem_info__data_src(mi)->val = data_src;
+	perf_script__meminfo_scnprintf(decode, 100, mi);
+	mem_info__put(mi);
 
 	len = scnprintf(out, 100, "%16" PRIx64 " %s", data_src, decode);
 	if (maxlen < len)
@@ -2498,7 +2503,7 @@ static int process_attr(struct perf_tool *tool, union perf_event *event,
 	evsel = evlist__last(*pevlist);
 
 	if (!evsel->priv) {
-		if (scr->per_event_dump) { 
+		if (scr->per_event_dump) {
 			evsel->priv = evsel_script__new(evsel, scr->session->data);
 			if (!evsel->priv)
 				return -ENOMEM;
diff --git a/tools/perf/tests/mem.c b/tools/perf/tests/mem.c
index f694a60d8a73..cb3d749e157b 100644
--- a/tools/perf/tests/mem.c
+++ b/tools/perf/tests/mem.c
@@ -13,12 +13,14 @@ static int check(union perf_mem_data_src data_src,
 {
 	char out[100];
 	char failure[100];
-	struct mem_info mi = { .data_src = data_src };
-
+	struct mem_info *mi = mem_info__new();
 	int n;
 
-	n = perf_mem__snp_scnprintf(out, sizeof out, &mi);
-	n += perf_mem__lvl_scnprintf(out + n, sizeof out - n, &mi);
+	TEST_ASSERT_VAL("Memory allocation failed", mi);
+	*mem_info__data_src(mi) = data_src;
+	n = perf_mem__snp_scnprintf(out, sizeof out, mi);
+	n += perf_mem__lvl_scnprintf(out + n, sizeof out - n, mi);
+	mem_info__put(mi);
 	scnprintf(failure, sizeof failure, "unexpected %s", out);
 	TEST_ASSERT_VAL(failure, !strcmp(string, out));
 	return 0;
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 56453a02cdf4..00814d42d5f1 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -154,8 +154,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 	}
 
 	if (h->mem_info) {
-		if (h->mem_info->daddr.ms.sym) {
-			symlen = (int)h->mem_info->daddr.ms.sym->namelen + 4
+		if (mem_info__daddr(h->mem_info)->ms.sym) {
+			symlen = (int)mem_info__daddr(h->mem_info)->ms.sym->namelen + 4
 			       + unresolved_col_width + 2;
 			hists__new_col_len(hists, HISTC_MEM_DADDR_SYMBOL,
 					   symlen);
@@ -169,8 +169,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 					   symlen);
 		}
 
-		if (h->mem_info->iaddr.ms.sym) {
-			symlen = (int)h->mem_info->iaddr.ms.sym->namelen + 4
+		if (mem_info__iaddr(h->mem_info)->ms.sym) {
+			symlen = (int)mem_info__iaddr(h->mem_info)->ms.sym->namelen + 4
 			       + unresolved_col_width + 2;
 			hists__new_col_len(hists, HISTC_MEM_IADDR_SYMBOL,
 					   symlen);
@@ -180,8 +180,8 @@ void hists__calc_col_len(struct hists *hists, struct hist_entry *h)
 					   symlen);
 		}
 
-		if (h->mem_info->daddr.ms.map) {
-			symlen = dso__name_len(map__dso(h->mem_info->daddr.ms.map));
+		if (mem_info__daddr(h->mem_info)->ms.map) {
+			symlen = dso__name_len(map__dso(mem_info__daddr(h->mem_info)->ms.map));
 			hists__new_col_len(hists, HISTC_MEM_DADDR_DSO,
 					   symlen);
 		} else {
@@ -477,8 +477,10 @@ static int hist_entry__init(struct hist_entry *he,
 	}
 
 	if (he->mem_info) {
-		he->mem_info->iaddr.ms.map = map__get(he->mem_info->iaddr.ms.map);
-		he->mem_info->daddr.ms.map = map__get(he->mem_info->daddr.ms.map);
+		mem_info__iaddr(he->mem_info)->ms.map =
+			map__get(mem_info__iaddr(he->mem_info)->ms.map);
+		mem_info__daddr(he->mem_info)->ms.map =
+			map__get(mem_info__daddr(he->mem_info)->ms.map);
 	}
 
 	if (hist_entry__has_callchains(he) && symbol_conf.use_callchain)
@@ -526,8 +528,8 @@ static int hist_entry__init(struct hist_entry *he,
 		zfree(&he->branch_info);
 	}
 	if (he->mem_info) {
-		map_symbol__exit(&he->mem_info->iaddr.ms);
-		map_symbol__exit(&he->mem_info->daddr.ms);
+		map_symbol__exit(&mem_info__iaddr(he->mem_info)->ms);
+		map_symbol__exit(&mem_info__daddr(he->mem_info)->ms);
 	}
 err:
 	map_symbol__exit(&he->ms);
@@ -1336,8 +1338,8 @@ void hist_entry__delete(struct hist_entry *he)
 	}
 
 	if (he->mem_info) {
-		map_symbol__exit(&he->mem_info->iaddr.ms);
-		map_symbol__exit(&he->mem_info->daddr.ms);
+		map_symbol__exit(&mem_info__iaddr(he->mem_info)->ms);
+		map_symbol__exit(&mem_info__daddr(he->mem_info)->ms);
 		mem_info__zput(he->mem_info);
 	}
 
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d5a01ef92668..8477edefc299 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -2013,11 +2013,11 @@ struct mem_info *sample__resolve_mem(struct perf_sample *sample,
 	if (!mi)
 		return NULL;
 
-	ip__resolve_ams(al->thread, &mi->iaddr, sample->ip);
-	ip__resolve_data(al->thread, al->cpumode, &mi->daddr,
+	ip__resolve_ams(al->thread, mem_info__iaddr(mi), sample->ip);
+	ip__resolve_data(al->thread, al->cpumode, mem_info__daddr(mi),
 			 sample->addr, sample->phys_addr,
 			 sample->data_page_size);
-	mi->data_src.val = sample->data_src;
+	mem_info__data_src(mi)->val = sample->data_src;
 
 	return mi;
 }
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index f618a5ccc7af..6dda47bb774f 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -293,7 +293,7 @@ int perf_mem__tlb_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 	out[0] = '\0';
 
 	if (mem_info)
-		m = mem_info->data_src.mem_dtlb;
+		m = mem_info__const_data_src(mem_info)->mem_dtlb;
 
 	hit = m & PERF_MEM_TLB_HIT;
 	miss = m & PERF_MEM_TLB_MISS;
@@ -367,7 +367,7 @@ static int perf_mem__op_scnprintf(char *out, size_t sz, const struct mem_info *m
 	int l;
 
 	if (mem_info)
-		op = mem_info->data_src.mem_op;
+		op = mem_info__const_data_src(mem_info)->mem_op;
 
 	if (op & PERF_MEM_OP_NA)
 		l = scnprintf(out, sz, "N/A");
@@ -400,7 +400,7 @@ int perf_mem__lvl_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 	if (!mem_info)
 		goto na;
 
-	data_src = mem_info->data_src;
+	data_src = *mem_info__const_data_src(mem_info);
 
 	if (data_src.mem_lvl & PERF_MEM_LVL_HIT)
 		memcpy(hit_miss, "hit", 3);
@@ -476,7 +476,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 	out[0] = '\0';
 
 	if (mem_info)
-		m = mem_info->data_src.mem_snoop;
+		m = mem_info__const_data_src(mem_info)->mem_snoop;
 
 	for (i = 0; m && i < ARRAY_SIZE(snoop_access); i++, m >>= 1) {
 		if (!(m & 0x1))
@@ -490,7 +490,7 @@ int perf_mem__snp_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 
 	m = 0;
 	if (mem_info)
-		m = mem_info->data_src.mem_snoopx;
+		m = mem_info__const_data_src(mem_info)->mem_snoopx;
 
 	for (i = 0; m && i < ARRAY_SIZE(snoopx_access); i++, m >>= 1) {
 		if (!(m & 0x1))
@@ -515,7 +515,7 @@ int perf_mem__lck_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 	int l;
 
 	if (mem_info)
-		mask = mem_info->data_src.mem_lock;
+		mask = mem_info__const_data_src(mem_info)->mem_lock;
 
 	if (mask & PERF_MEM_LOCK_NA)
 		l = scnprintf(out, sz, "N/A");
@@ -536,7 +536,7 @@ int perf_mem__blk_scnprintf(char *out, size_t sz, const struct mem_info *mem_inf
 	out[0] = '\0';
 
 	if (mem_info)
-		mask = mem_info->data_src.mem_blk;
+		mask = mem_info__const_data_src(mem_info)->mem_blk;
 
 	if (!mask || (mask & PERF_MEM_BLK_NA)) {
 		l += scnprintf(out + l, sz - l, " N/A");
@@ -572,8 +572,8 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
 
 int c2c_decode_stats(struct c2c_stats *stats, struct mem_info *mi)
 {
-	union perf_mem_data_src *data_src = &mi->data_src;
-	u64 daddr  = mi->daddr.addr;
+	union perf_mem_data_src *data_src = mem_info__data_src(mi);
+	u64 daddr  = mem_info__daddr(mi)->addr;
 	u64 op     = data_src->mem_op;
 	u64 lvl    = data_src->mem_lvl;
 	u64 snoop  = data_src->mem_snoop;
@@ -700,7 +700,7 @@ do {				\
 		return -1;
 	}
 
-	if (!mi->daddr.ms.map || !mi->iaddr.ms.map) {
+	if (!mem_info__daddr(mi)->ms.map || !mem_info__iaddr(mi)->ms.map) {
 		stats->nomap++;
 		return -1;
 	}
diff --git a/tools/perf/util/mem-info.c b/tools/perf/util/mem-info.c
index ff0dfdb5369a..27d67721a695 100644
--- a/tools/perf/util/mem-info.c
+++ b/tools/perf/util/mem-info.c
@@ -4,25 +4,32 @@
 
 struct mem_info *mem_info__get(struct mem_info *mi)
 {
-	if (mi)
-		refcount_inc(&mi->refcnt);
-	return mi;
+	struct mem_info *result;
+
+	if (RC_CHK_GET(result, mi))
+		refcount_inc(mem_info__refcnt(mi));
+
+	return result;
 }
 
 void mem_info__put(struct mem_info *mi)
 {
-	if (mi && refcount_dec_and_test(&mi->refcnt)) {
-		addr_map_symbol__exit(&mi->iaddr);
-		addr_map_symbol__exit(&mi->daddr);
-		free(mi);
+	if (mi && refcount_dec_and_test(mem_info__refcnt(mi))) {
+		addr_map_symbol__exit(mem_info__iaddr(mi));
+		addr_map_symbol__exit(mem_info__daddr(mi));
+		RC_CHK_FREE(mi);
+	} else {
+		RC_CHK_PUT(mi);
 	}
 }
 
 struct mem_info *mem_info__new(void)
 {
-	struct mem_info *mi = zalloc(sizeof(*mi));
+	struct mem_info *result = NULL;
+	RC_STRUCT(mem_info) *mi = zalloc(sizeof(*mi));
+
+	if (ADD_RC_CHK(result, mi))
+		refcount_set(mem_info__refcnt(result), 1);
 
-	if (mi)
-		refcount_set(&mi->refcnt, 1);
-	return mi;
+	return result;
 }
diff --git a/tools/perf/util/mem-info.h b/tools/perf/util/mem-info.h
index bb7d8375d73c..0f68e29f311b 100644
--- a/tools/perf/util/mem-info.h
+++ b/tools/perf/util/mem-info.h
@@ -4,9 +4,10 @@
 
 #include <linux/refcount.h>
 #include <linux/perf_event.h>
+#include <internal/rc_check.h>
 #include "map_symbol.h"
 
-struct mem_info {
+DECLARE_RC_STRUCT(mem_info) {
 	struct addr_map_symbol	iaddr;
 	struct addr_map_symbol	daddr;
 	union perf_mem_data_src	data_src;
@@ -25,4 +26,29 @@ static inline void __mem_info__zput(struct mem_info **mi)
 
 #define mem_info__zput(mi) __mem_info__zput(&mi)
 
+static inline struct addr_map_symbol *mem_info__iaddr(struct mem_info *mi)
+{
+	return &RC_CHK_ACCESS(mi)->iaddr;
+}
+
+static inline struct addr_map_symbol *mem_info__daddr(struct mem_info *mi)
+{
+	return &RC_CHK_ACCESS(mi)->daddr;
+}
+
+static inline union perf_mem_data_src *mem_info__data_src(struct mem_info *mi)
+{
+	return &RC_CHK_ACCESS(mi)->data_src;
+}
+
+static inline const union perf_mem_data_src *mem_info__const_data_src(const struct mem_info *mi)
+{
+	return &RC_CHK_ACCESS(mi)->data_src;
+}
+
+static inline refcount_t *mem_info__refcnt(struct mem_info *mi)
+{
+	return &RC_CHK_ACCESS(mi)->refcnt;
+}
+
 #endif /* __PERF_MEM_INFO_H */
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c77fe08a0545..fb00f3ad6815 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -721,15 +721,20 @@ static void set_sample_read_in_dict(PyObject *dict_sample,
 }
 
 static void set_sample_datasrc_in_dict(PyObject *dict,
-				       struct perf_sample *sample)
+				      struct perf_sample *sample)
 {
-	struct mem_info mi = { .data_src.val = sample->data_src };
+	struct mem_info *mi = mem_info__new();
 	char decode[100];
 
+	if (!mi)
+		Py_FatalError("couldn't create mem-info");
+
 	pydict_set_item_string_decref(dict, "datasrc",
 			PyLong_FromUnsignedLongLong(sample->data_src));
 
-	perf_script__meminfo_scnprintf(decode, 100, &mi);
+	mem_info__data_src(mi)->val = sample->data_src;
+	perf_script__meminfo_scnprintf(decode, 100, mi);
+	mem_info__put(mi);
 
 	pydict_set_item_string_decref(dict, "datasrc_decode",
 			_PyUnicode_FromString(decode));
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 711ef69306f3..cd39ea972193 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1365,9 +1365,9 @@ sort__daddr_cmp(struct hist_entry *left, struct hist_entry *right)
 	uint64_t l = 0, r = 0;
 
 	if (left->mem_info)
-		l = left->mem_info->daddr.addr;
+		l = mem_info__daddr(left->mem_info)->addr;
 	if (right->mem_info)
-		r = right->mem_info->daddr.addr;
+		r = mem_info__daddr(right->mem_info)->addr;
 
 	return (int64_t)(r - l);
 }
@@ -1379,8 +1379,8 @@ static int hist_entry__daddr_snprintf(struct hist_entry *he, char *bf,
 	struct map_symbol *ms = NULL;
 
 	if (he->mem_info) {
-		addr = he->mem_info->daddr.addr;
-		ms = &he->mem_info->daddr.ms;
+		addr = mem_info__daddr(he->mem_info)->addr;
+		ms = &mem_info__daddr(he->mem_info)->ms;
 	}
 	return _hist_entry__sym_snprintf(ms, addr, he->level, bf, size, width);
 }
@@ -1391,9 +1391,9 @@ sort__iaddr_cmp(struct hist_entry *left, struct hist_entry *right)
 	uint64_t l = 0, r = 0;
 
 	if (left->mem_info)
-		l = left->mem_info->iaddr.addr;
+		l = mem_info__iaddr(left->mem_info)->addr;
 	if (right->mem_info)
-		r = right->mem_info->iaddr.addr;
+		r = mem_info__iaddr(right->mem_info)->addr;
 
 	return (int64_t)(r - l);
 }
@@ -1405,8 +1405,8 @@ static int hist_entry__iaddr_snprintf(struct hist_entry *he, char *bf,
 	struct map_symbol *ms = NULL;
 
 	if (he->mem_info) {
-		addr = he->mem_info->iaddr.addr;
-		ms   = &he->mem_info->iaddr.ms;
+		addr = mem_info__iaddr(he->mem_info)->addr;
+		ms   = &mem_info__iaddr(he->mem_info)->ms;
 	}
 	return _hist_entry__sym_snprintf(ms, addr, he->level, bf, size, width);
 }
@@ -1418,9 +1418,9 @@ sort__dso_daddr_cmp(struct hist_entry *left, struct hist_entry *right)
 	struct map *map_r = NULL;
 
 	if (left->mem_info)
-		map_l = left->mem_info->daddr.ms.map;
+		map_l = mem_info__daddr(left->mem_info)->ms.map;
 	if (right->mem_info)
-		map_r = right->mem_info->daddr.ms.map;
+		map_r = mem_info__daddr(right->mem_info)->ms.map;
 
 	return _sort__dso_cmp(map_l, map_r);
 }
@@ -1431,7 +1431,7 @@ static int hist_entry__dso_daddr_snprintf(struct hist_entry *he, char *bf,
 	struct map *map = NULL;
 
 	if (he->mem_info)
-		map = he->mem_info->daddr.ms.map;
+		map = mem_info__daddr(he->mem_info)->ms.map;
 
 	return _hist_entry__dso_snprintf(map, bf, size, width);
 }
@@ -1443,12 +1443,12 @@ sort__locked_cmp(struct hist_entry *left, struct hist_entry *right)
 	union perf_mem_data_src data_src_r;
 
 	if (left->mem_info)
-		data_src_l = left->mem_info->data_src;
+		data_src_l = *mem_info__data_src(left->mem_info);
 	else
 		data_src_l.mem_lock = PERF_MEM_LOCK_NA;
 
 	if (right->mem_info)
-		data_src_r = right->mem_info->data_src;
+		data_src_r = *mem_info__data_src(right->mem_info);
 	else
 		data_src_r.mem_lock = PERF_MEM_LOCK_NA;
 
@@ -1471,12 +1471,12 @@ sort__tlb_cmp(struct hist_entry *left, struct hist_entry *right)
 	union perf_mem_data_src data_src_r;
 
 	if (left->mem_info)
-		data_src_l = left->mem_info->data_src;
+		data_src_l = *mem_info__data_src(left->mem_info);
 	else
 		data_src_l.mem_dtlb = PERF_MEM_TLB_NA;
 
 	if (right->mem_info)
-		data_src_r = right->mem_info->data_src;
+		data_src_r = *mem_info__data_src(right->mem_info);
 	else
 		data_src_r.mem_dtlb = PERF_MEM_TLB_NA;
 
@@ -1499,12 +1499,12 @@ sort__lvl_cmp(struct hist_entry *left, struct hist_entry *right)
 	union perf_mem_data_src data_src_r;
 
 	if (left->mem_info)
-		data_src_l = left->mem_info->data_src;
+		data_src_l = *mem_info__data_src(left->mem_info);
 	else
 		data_src_l.mem_lvl = PERF_MEM_LVL_NA;
 
 	if (right->mem_info)
-		data_src_r = right->mem_info->data_src;
+		data_src_r = *mem_info__data_src(right->mem_info);
 	else
 		data_src_r.mem_lvl = PERF_MEM_LVL_NA;
 
@@ -1527,12 +1527,12 @@ sort__snoop_cmp(struct hist_entry *left, struct hist_entry *right)
 	union perf_mem_data_src data_src_r;
 
 	if (left->mem_info)
-		data_src_l = left->mem_info->data_src;
+		data_src_l = *mem_info__data_src(left->mem_info);
 	else
 		data_src_l.mem_snoop = PERF_MEM_SNOOP_NA;
 
 	if (right->mem_info)
-		data_src_r = right->mem_info->data_src;
+		data_src_r = *mem_info__data_src(right->mem_info);
 	else
 		data_src_r.mem_snoop = PERF_MEM_SNOOP_NA;
 
@@ -1563,8 +1563,8 @@ sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
 	if (left->cpumode > right->cpumode) return -1;
 	if (left->cpumode < right->cpumode) return 1;
 
-	l_map = left->mem_info->daddr.ms.map;
-	r_map = right->mem_info->daddr.ms.map;
+	l_map = mem_info__daddr(left->mem_info)->ms.map;
+	r_map = mem_info__daddr(right->mem_info)->ms.map;
 
 	/* if both are NULL, jump to sort on al_addr instead */
 	if (!l_map && !r_map)
@@ -1599,8 +1599,8 @@ sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
 
 addr:
 	/* al_addr does all the right addr - start + offset calculations */
-	l = cl_address(left->mem_info->daddr.al_addr, chk_double_cl);
-	r = cl_address(right->mem_info->daddr.al_addr, chk_double_cl);
+	l = cl_address(mem_info__daddr(left->mem_info)->al_addr, chk_double_cl);
+	r = cl_address(mem_info__daddr(right->mem_info)->al_addr, chk_double_cl);
 
 	if (l > r) return -1;
 	if (l < r) return 1;
@@ -1617,11 +1617,11 @@ static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
 	char level = he->level;
 
 	if (he->mem_info) {
-		struct map *map = he->mem_info->daddr.ms.map;
+		struct map *map = mem_info__daddr(he->mem_info)->ms.map;
 		struct dso *dso = map ? map__dso(map) : NULL;
 
-		addr = cl_address(he->mem_info->daddr.al_addr, chk_double_cl);
-		ms = &he->mem_info->daddr.ms;
+		addr = cl_address(mem_info__daddr(he->mem_info)->al_addr, chk_double_cl);
+		ms = &mem_info__daddr(he->mem_info)->ms;
 
 		/* print [s] for shared data mmaps */
 		if ((he->cpumode != PERF_RECORD_MISC_KERNEL) &&
@@ -1806,12 +1806,12 @@ sort__blocked_cmp(struct hist_entry *left, struct hist_entry *right)
 	union perf_mem_data_src data_src_r;
 
 	if (left->mem_info)
-		data_src_l = left->mem_info->data_src;
+		data_src_l = *mem_info__data_src(left->mem_info);
 	else
 		data_src_l.mem_blk = PERF_MEM_BLK_NA;
 
 	if (right->mem_info)
-		data_src_r = right->mem_info->data_src;
+		data_src_r = *mem_info__data_src(right->mem_info);
 	else
 		data_src_r.mem_blk = PERF_MEM_BLK_NA;
 
@@ -1840,9 +1840,9 @@ sort__phys_daddr_cmp(struct hist_entry *left, struct hist_entry *right)
 	uint64_t l = 0, r = 0;
 
 	if (left->mem_info)
-		l = left->mem_info->daddr.phys_addr;
+		l = mem_info__daddr(left->mem_info)->phys_addr;
 	if (right->mem_info)
-		r = right->mem_info->daddr.phys_addr;
+		r = mem_info__daddr(right->mem_info)->phys_addr;
 
 	return (int64_t)(r - l);
 }
@@ -1854,7 +1854,7 @@ static int hist_entry__phys_daddr_snprintf(struct hist_entry *he, char *bf,
 	size_t ret = 0;
 	size_t len = BITS_PER_LONG / 4;
 
-	addr = he->mem_info->daddr.phys_addr;
+	addr = mem_info__daddr(he->mem_info)->phys_addr;
 
 	ret += repsep_snprintf(bf + ret, size - ret, "[%c] ", he->level);
 
@@ -1881,9 +1881,9 @@ sort__data_page_size_cmp(struct hist_entry *left, struct hist_entry *right)
 	uint64_t l = 0, r = 0;
 
 	if (left->mem_info)
-		l = left->mem_info->daddr.data_page_size;
+		l = mem_info__daddr(left->mem_info)->data_page_size;
 	if (right->mem_info)
-		r = right->mem_info->daddr.data_page_size;
+		r = mem_info__daddr(right->mem_info)->data_page_size;
 
 	return (int64_t)(r - l);
 }
@@ -1894,7 +1894,7 @@ static int hist_entry__data_page_size_snprintf(struct hist_entry *he, char *bf,
 	char str[PAGE_SIZE_NAME_LEN];
 
 	return repsep_snprintf(bf, size, "%-*s", width,
-			       get_page_size_name(he->mem_info->daddr.data_page_size, str));
+			get_page_size_name(mem_info__daddr(he->mem_info)->data_page_size, str));
 }
 
 struct sort_entry sort_mem_data_page_size = {
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 8/8] perf hist: Avoid hist_entry_iter mem_info memory leak

[Ian Rogers]

struct mem_info is reference counted while branch_info and he_cache
are not. Break apart the priv field in hist_entry_iter so that we can
know which values are owned by the iter and do the appropriate free or
put. Move hide_unresolved to marginally shrink the size of the now
grown struct.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/hist.c | 39 ++++++++++++++-------------------------
 tools/perf/util/hist.h |  8 +++++---
 2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 00814d42d5f1..2e9e193179dd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -476,13 +476,6 @@ static int hist_entry__init(struct hist_entry *he,
 		he->branch_info->to.ms.map = map__get(he->branch_info->to.ms.map);
 	}
 
-	if (he->mem_info) {
-		mem_info__iaddr(he->mem_info)->ms.map =
-			map__get(mem_info__iaddr(he->mem_info)->ms.map);
-		mem_info__daddr(he->mem_info)->ms.map =
-			map__get(mem_info__daddr(he->mem_info)->ms.map);
-	}
-
 	if (hist_entry__has_callchains(he) && symbol_conf.use_callchain)
 		callchain_init(he->callchain);
 
@@ -574,7 +567,6 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
 			he = NULL;
 		}
 	}
-
 	return he;
 }
 
@@ -747,7 +739,7 @@ __hists__add_entry(struct hists *hists,
 		.filtered = symbol__parent_filter(sym_parent) | al->filtered,
 		.hists	= hists,
 		.branch_info = bi,
-		.mem_info = mi,
+		.mem_info = mem_info__get(mi),
 		.kvm_info = ki,
 		.block_info = block_info,
 		.transaction = sample->transaction,
@@ -836,7 +828,7 @@ iter_prepare_mem_entry(struct hist_entry_iter *iter, struct addr_location *al)
 	if (mi == NULL)
 		return -ENOMEM;
 
-	iter->priv = mi;
+	iter->mi = mi;
 	return 0;
 }
 
@@ -844,7 +836,7 @@ static int
 iter_add_single_mem_entry(struct hist_entry_iter *iter, struct addr_location *al)
 {
 	u64 cost;
-	struct mem_info *mi = iter->priv;
+	struct mem_info *mi = iter->mi;
 	struct hists *hists = evsel__hists(iter->evsel);
 	struct perf_sample *sample = iter->sample;
 	struct hist_entry *he;
@@ -891,12 +883,7 @@ iter_finish_mem_entry(struct hist_entry_iter *iter,
 	err = hist_entry__append_callchain(he, iter->sample);
 
 out:
-	/*
-	 * We don't need to free iter->priv (mem_info) here since the mem info
-	 * was either already freed in hists__findnew_entry() or passed to a
-	 * new hist entry by hist_entry__new().
-	 */
-	iter->priv = NULL;
+	mem_info__zput(iter->mi);
 
 	iter->he = NULL;
 	return err;
@@ -915,7 +902,7 @@ iter_prepare_branch_entry(struct hist_entry_iter *iter, struct addr_location *al
 	iter->curr = 0;
 	iter->total = sample->branch_stack->nr;
 
-	iter->priv = bi;
+	iter->bi = bi;
 	return 0;
 }
 
@@ -929,7 +916,7 @@ iter_add_single_branch_entry(struct hist_entry_iter *iter __maybe_unused,
 static int
 iter_next_branch_entry(struct hist_entry_iter *iter, struct addr_location *al)
 {
-	struct branch_info *bi = iter->priv;
+	struct branch_info *bi = iter->bi;
 	int i = iter->curr;
 
 	if (bi == NULL)
@@ -958,7 +945,7 @@ iter_add_next_branch_entry(struct hist_entry_iter *iter, struct addr_location *a
 	int i = iter->curr;
 	int err = 0;
 
-	bi = iter->priv;
+	bi = iter->bi;
 
 	if (iter->hide_unresolved && !(bi[i].from.ms.sym && bi[i].to.ms.sym))
 		goto out;
@@ -987,7 +974,7 @@ static int
 iter_finish_branch_entry(struct hist_entry_iter *iter,
 			 struct addr_location *al __maybe_unused)
 {
-	zfree(&iter->priv);
+	zfree(&iter->bi);
 	iter->he = NULL;
 
 	return iter->curr >= iter->total ? 0 : -1;
@@ -1055,7 +1042,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	if (he_cache == NULL)
 		return -ENOMEM;
 
-	iter->priv = he_cache;
+	iter->he_cache = he_cache;
 	iter->curr = 0;
 
 	return 0;
@@ -1068,7 +1055,7 @@ iter_add_single_cumulative_entry(struct hist_entry_iter *iter,
 	struct evsel *evsel = iter->evsel;
 	struct hists *hists = evsel__hists(evsel);
 	struct perf_sample *sample = iter->sample;
-	struct hist_entry **he_cache = iter->priv;
+	struct hist_entry **he_cache = iter->he_cache;
 	struct hist_entry *he;
 	int err = 0;
 
@@ -1126,7 +1113,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 {
 	struct evsel *evsel = iter->evsel;
 	struct perf_sample *sample = iter->sample;
-	struct hist_entry **he_cache = iter->priv;
+	struct hist_entry **he_cache = iter->he_cache;
 	struct hist_entry *he;
 	struct hist_entry he_tmp = {
 		.hists = evsel__hists(evsel),
@@ -1192,7 +1179,9 @@ static int
 iter_finish_cumulative_entry(struct hist_entry_iter *iter,
 			     struct addr_location *al __maybe_unused)
 {
-	zfree(&iter->priv);
+	mem_info__zput(iter->mi);
+	zfree(&iter->bi);
+	zfree(&iter->he_cache);
 	iter->he = NULL;
 
 	return 0;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 5260822b9773..8fb3bdd29188 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -132,18 +132,20 @@ struct hist_entry_iter {
 	int total;
 	int curr;
 
-	bool hide_unresolved;
-
 	struct evsel *evsel;
 	struct perf_sample *sample;
 	struct hist_entry *he;
 	struct symbol *parent;
-	void *priv;
+
+	struct mem_info *mi;
+	struct branch_info *bi;
+	struct hist_entry **he_cache;
 
 	const struct hist_iter_ops *ops;
 	/* user-defined callback function (optional) */
 	int (*add_entry_cb)(struct hist_entry_iter *iter,
 			    struct addr_location *al, bool single, void *arg);
+	bool hide_unresolved;
 };
 
 extern const struct hist_iter_ops hist_iter_normal;
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> ui_browser__show is capturing the input title that is stack allocated
> memory in hist_browser__run. Avoid a use after return by strdup-ing
> the string.

But everything happens in that context, i.e. hist_brower__run() will
call ui_browser__ methods and then exit.

We end up having browser->title pointing to returned stack memory
(invalid) but there will be no references to it, no?

If we return to hist_browser__run() we then call ui_browser__show
passing a new title, for "live" stack memory, rinse repeat. Or have you
noticed an actual use-after-"free"?

- Arnaldo
 
> Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/ui/browser.c | 4 +++-
>  tools/perf/ui/browser.h | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index 603d11283cbd..c4cdf2ea69b7 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
>  	mutex_lock(&ui__lock);
>  	__ui_browser__show_title(browser, title);
>  
> -	browser->title = title;
> +	free(browser->title);
> +	browser->title = strdup(title);
>  	zfree(&browser->helpline);
>  
>  	va_start(ap, helpline);
> @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
>  	mutex_lock(&ui__lock);
>  	ui_helpline__pop();
>  	zfree(&browser->helpline);
> +	zfree(&browser->title);
>  	mutex_unlock(&ui__lock);
>  }
>  
> diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> index 510ce4554050..6e98d5f8f71c 100644
> --- a/tools/perf/ui/browser.h
> +++ b/tools/perf/ui/browser.h
> @@ -21,7 +21,7 @@ struct ui_browser {
>  	u8	      extra_title_lines;
>  	int	      current_color;
>  	void	      *priv;
> -	const char    *title;
> +	char	      *title;
>  	char	      *helpline;
>  	const char    *no_samples_msg;
>  	void 	      (*refresh_dimensions)(struct ui_browser *browser);
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > ui_browser__show is capturing the input title that is stack allocated
> > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > the string.
> 
> But everything happens in that context, i.e. hist_brower__run() will
> call ui_browser__ methods and then exit.
> 
> We end up having browser->title pointing to returned stack memory
> (invalid) but there will be no references to it, no?
> 
> If we return to hist_browser__run() we then call ui_browser__show
> passing a new title, for "live" stack memory, rinse repeat. Or have you
> noticed an actual use-after-"free"?

And I'll take the patch, I'm just trying to figure it out if it fixed a
real bug or if it just makes the code more future proof, i.e. to avoid
us adding code that actually uses invalid stack memory.

- Arnaldo
 
> - Arnaldo
>  
> > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/ui/browser.c | 4 +++-
> >  tools/perf/ui/browser.h | 2 +-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > index 603d11283cbd..c4cdf2ea69b7 100644
> > --- a/tools/perf/ui/browser.c
> > +++ b/tools/perf/ui/browser.c
> > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> >  	mutex_lock(&ui__lock);
> >  	__ui_browser__show_title(browser, title);
> >  
> > -	browser->title = title;
> > +	free(browser->title);
> > +	browser->title = strdup(title);
> >  	zfree(&browser->helpline);
> >  
> >  	va_start(ap, helpline);
> > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> >  	mutex_lock(&ui__lock);
> >  	ui_helpline__pop();
> >  	zfree(&browser->helpline);
> > +	zfree(&browser->title);
> >  	mutex_unlock(&ui__lock);
> >  }
> >  
> > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > index 510ce4554050..6e98d5f8f71c 100644
> > --- a/tools/perf/ui/browser.h
> > +++ b/tools/perf/ui/browser.h
> > @@ -21,7 +21,7 @@ struct ui_browser {
> >  	u8	      extra_title_lines;
> >  	int	      current_color;
> >  	void	      *priv;
> > -	const char    *title;
> > +	char	      *title;
> >  	char	      *helpline;
> >  	const char    *no_samples_msg;
> >  	void 	      (*refresh_dimensions)(struct ui_browser *browser);
> > -- 
> > 2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Ian Rogers]

On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > ui_browser__show is capturing the input title that is stack allocated
> > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > the string.
> >
> > But everything happens in that context, i.e. hist_brower__run() will
> > call ui_browser__ methods and then exit.
> >
> > We end up having browser->title pointing to returned stack memory
> > (invalid) but there will be no references to it, no?
> >
> > If we return to hist_browser__run() we then call ui_browser__show
> > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > noticed an actual use-after-"free"?
>
> And I'll take the patch, I'm just trying to figure it out if it fixed a
> real bug or if it just makes the code more future proof, i.e. to avoid
> us adding code that actually uses invalid stack memory.

My command line using tui is:
$ sudo bash -c 'rm /tmp/asan.log*; export
ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
sleep 1; /tmp/perf/perf mem report'
I then go to the perf annotate view and quit. This triggers the asan
error (from the log file):
```
==1254591==ERROR: AddressSanitizer: stack-use-after-return on address
0x7f2813331920 at pc 0x7f28180
65991 bp 0x7fff0a21c750 sp 0x7fff0a21bf10
READ of size 80 at 0x7f2813331920 thread T0
    #0 0x7f2818065990 in __interceptor_strlen
../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:461
    #1 0x7f2817698251 in SLsmg_write_wrapped_string
(/lib/x86_64-linux-gnu/libslang.so.2+0x98251)
    #2 0x7f28176984b9 in SLsmg_write_nstring
(/lib/x86_64-linux-gnu/libslang.so.2+0x984b9)
    #3 0x55c94045b365 in ui_browser__write_nstring ui/browser.c:60
    #4 0x55c94045c558 in __ui_browser__show_title ui/browser.c:266
    #5 0x55c94045c776 in ui_browser__show ui/browser.c:288
    #6 0x55c94045c06d in ui_browser__handle_resize ui/browser.c:206
    #7 0x55c94047979b in do_annotate ui/browsers/hists.c:2458
    #8 0x55c94047fb17 in evsel__hists_browse ui/browsers/hists.c:3412
    #9 0x55c940480a0c in perf_evsel_menu__run ui/browsers/hists.c:3527
    #10 0x55c940481108 in __evlist__tui_browse_hists ui/browsers/hists.c:3613
    #11 0x55c9404813f7 in evlist__tui_browse_hists ui/browsers/hists.c:3661
    #12 0x55c93ffa253f in report__browse_hists tools/perf/builtin-report.c:671
    #13 0x55c93ffa58ca in __cmd_report tools/perf/builtin-report.c:1141
    #14 0x55c93ffaf159 in cmd_report tools/perf/builtin-report.c:1805
    #15 0x55c94000c05c in report_events tools/perf/builtin-mem.c:374
    #16 0x55c94000d96d in cmd_mem tools/perf/builtin-mem.c:516
    #17 0x55c9400e44ee in run_builtin tools/perf/perf.c:350
    #18 0x55c9400e4a5a in handle_internal_command tools/perf/perf.c:403
    #19 0x55c9400e4e22 in run_argv tools/perf/perf.c:447
    #20 0x55c9400e53ad in main tools/perf/perf.c:561
    #21 0x7f28170456c9 in __libc_start_call_main
../sysdeps/nptl/libc_start_call_main.h:58
    #22 0x7f2817045784 in __libc_start_main_impl ../csu/libc-start.c:360
    #23 0x55c93ff544c0 in _start (/tmp/perf/perf+0x19a4c0) (BuildId:
84899b0e8c7d3a3eaa67b2eb35e3d8b2f8cd4c93)

Address 0x7f2813331920 is located in stack of thread T0 at offset 32 in frame
    #0 0x55c94046e85e in hist_browser__run ui/browsers/hists.c:746

  This frame has 1 object(s):
    [32, 192) 'title' (line 747) <== Memory access at offset 32 is
inside this variable
HINT: this may be a false positive if your program uses some custom
stack unwind mechanism, swapcontext or vfork
```
hist_browser__run isn't on the stack so the asan error looks legit.
There's no clean init/exit on struct ui_browser so I may be trading a
use-after-return for a memory leak, but that seems look a good trade
anyway.

Thanks,
Ian

> - Arnaldo
>
> > - Arnaldo
> >
> > > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/ui/browser.c | 4 +++-
> > >  tools/perf/ui/browser.h | 2 +-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > > index 603d11283cbd..c4cdf2ea69b7 100644
> > > --- a/tools/perf/ui/browser.c
> > > +++ b/tools/perf/ui/browser.c
> > > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > >     mutex_lock(&ui__lock);
> > >     __ui_browser__show_title(browser, title);
> > >
> > > -   browser->title = title;
> > > +   free(browser->title);
> > > +   browser->title = strdup(title);
> > >     zfree(&browser->helpline);
> > >
> > >     va_start(ap, helpline);
> > > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> > >     mutex_lock(&ui__lock);
> > >     ui_helpline__pop();
> > >     zfree(&browser->helpline);
> > > +   zfree(&browser->title);
> > >     mutex_unlock(&ui__lock);
> > >  }
> > >
> > > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > > index 510ce4554050..6e98d5f8f71c 100644
> > > --- a/tools/perf/ui/browser.h
> > > +++ b/tools/perf/ui/browser.h
> > > @@ -21,7 +21,7 @@ struct ui_browser {
> > >     u8            extra_title_lines;
> > >     int           current_color;
> > >     void          *priv;
> > > -   const char    *title;
> > > +   char          *title;
> > >     char          *helpline;
> > >     const char    *no_samples_msg;
> > >     void          (*refresh_dimensions)(struct ui_browser *browser);
> > > --
> > > 2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 0/8] Address/leak sanitizer clean ups

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 11:35:37AM -0700, Ian Rogers wrote:
> Remove unnecessary reference counts for structs with no gets.  Add
> reference count checking to comm_str and mem_info.  Fix memory leaks
> and errors detected on "perf mem report" by address sanitizer and leak
> sanitizer.

Applied locally, doing build tests. Will soon go to tmp.perf-tools-next.

- Arnaldo
 
> Ian Rogers (8):
>   perf ui browser: Don't save pointer to stack memory
>   perf annotate: Fix memory leak in annotated_source
>   perf block-info: Remove unused refcount
>   perf cpumap: Remove refcnt from cpu_aggr_map
>   perf comm: Add reference count checking to comm_str
>   perf mem-info: Move mem-info out of mem-events and symbol
>   perf mem-info: Add reference count checking
>   perf hist: Avoid hist_entry_iter mem_info memory leak
> 
>  tools/perf/builtin-c2c.c                      |  13 +-
>  tools/perf/builtin-report.c                   |   3 +-
>  tools/perf/builtin-script.c                   |  12 +-
>  tools/perf/builtin-stat.c                     |  16 +-
>  tools/perf/tests/mem.c                        |  11 +-
>  tools/perf/ui/browser.c                       |   4 +-
>  tools/perf/ui/browser.h                       |   2 +-
>  tools/perf/util/Build                         |   1 +
>  tools/perf/util/annotate.c                    |   6 +
>  tools/perf/util/block-info.c                  |  22 +-
>  tools/perf/util/block-info.h                  |  15 +-
>  tools/perf/util/comm.c                        | 196 +++++++++++-------
>  tools/perf/util/cpumap.c                      |   2 -
>  tools/perf/util/cpumap.h                      |   2 -
>  tools/perf/util/hist.c                        |  62 +++---
>  tools/perf/util/hist.h                        |   8 +-
>  tools/perf/util/machine.c                     |   7 +-
>  tools/perf/util/mem-events.c                  |  36 ++--
>  tools/perf/util/mem-events.h                  |  29 +--
>  tools/perf/util/mem-info.c                    |  35 ++++
>  tools/perf/util/mem-info.h                    |  54 +++++
>  .../scripting-engines/trace-event-python.c    |  12 +-
>  tools/perf/util/sort.c                        |  69 +++---
>  tools/perf/util/symbol.c                      |  26 +--
>  tools/perf/util/symbol.h                      |  12 --
>  25 files changed, 370 insertions(+), 285 deletions(-)
>  create mode 100644 tools/perf/util/mem-info.c
>  create mode 100644 tools/perf/util/mem-info.h
> 
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> >
> > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > ui_browser__show is capturing the input title that is stack allocated
> > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > the string.
> > >
> > > But everything happens in that context, i.e. hist_brower__run() will
> > > call ui_browser__ methods and then exit.
> > >
> > > We end up having browser->title pointing to returned stack memory
> > > (invalid) but there will be no references to it, no?
> > >
> > > If we return to hist_browser__run() we then call ui_browser__show
> > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > noticed an actual use-after-"free"?
> >
> > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > real bug or if it just makes the code more future proof, i.e. to avoid
> > us adding code that actually uses invalid stack memory.
> 
> My command line using tui is:
> $ sudo bash -c 'rm /tmp/asan.log*; export
> ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> sleep 1; /tmp/perf/perf mem report'
> I then go to the perf annotate view and quit. This triggers the asan
> error (from the log file):
> ```

Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
in the full details can hopefully find this message going from the Link:
tag.

- Arnaldo

> ==1254591==ERROR: AddressSanitizer: stack-use-after-return on address
> 0x7f2813331920 at pc 0x7f28180
> 65991 bp 0x7fff0a21c750 sp 0x7fff0a21bf10
> READ of size 80 at 0x7f2813331920 thread T0
>     #0 0x7f2818065990 in __interceptor_strlen
> ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:461
>     #1 0x7f2817698251 in SLsmg_write_wrapped_string
> (/lib/x86_64-linux-gnu/libslang.so.2+0x98251)
>     #2 0x7f28176984b9 in SLsmg_write_nstring
> (/lib/x86_64-linux-gnu/libslang.so.2+0x984b9)
>     #3 0x55c94045b365 in ui_browser__write_nstring ui/browser.c:60
>     #4 0x55c94045c558 in __ui_browser__show_title ui/browser.c:266
>     #5 0x55c94045c776 in ui_browser__show ui/browser.c:288
>     #6 0x55c94045c06d in ui_browser__handle_resize ui/browser.c:206
>     #7 0x55c94047979b in do_annotate ui/browsers/hists.c:2458
>     #8 0x55c94047fb17 in evsel__hists_browse ui/browsers/hists.c:3412
>     #9 0x55c940480a0c in perf_evsel_menu__run ui/browsers/hists.c:3527
>     #10 0x55c940481108 in __evlist__tui_browse_hists ui/browsers/hists.c:3613
>     #11 0x55c9404813f7 in evlist__tui_browse_hists ui/browsers/hists.c:3661
>     #12 0x55c93ffa253f in report__browse_hists tools/perf/builtin-report.c:671
>     #13 0x55c93ffa58ca in __cmd_report tools/perf/builtin-report.c:1141
>     #14 0x55c93ffaf159 in cmd_report tools/perf/builtin-report.c:1805
>     #15 0x55c94000c05c in report_events tools/perf/builtin-mem.c:374
>     #16 0x55c94000d96d in cmd_mem tools/perf/builtin-mem.c:516
>     #17 0x55c9400e44ee in run_builtin tools/perf/perf.c:350
>     #18 0x55c9400e4a5a in handle_internal_command tools/perf/perf.c:403
>     #19 0x55c9400e4e22 in run_argv tools/perf/perf.c:447
>     #20 0x55c9400e53ad in main tools/perf/perf.c:561
>     #21 0x7f28170456c9 in __libc_start_call_main
> ../sysdeps/nptl/libc_start_call_main.h:58
>     #22 0x7f2817045784 in __libc_start_main_impl ../csu/libc-start.c:360
>     #23 0x55c93ff544c0 in _start (/tmp/perf/perf+0x19a4c0) (BuildId:
> 84899b0e8c7d3a3eaa67b2eb35e3d8b2f8cd4c93)
> 
> Address 0x7f2813331920 is located in stack of thread T0 at offset 32 in frame
>     #0 0x55c94046e85e in hist_browser__run ui/browsers/hists.c:746
> 
>   This frame has 1 object(s):
>     [32, 192) 'title' (line 747) <== Memory access at offset 32 is
> inside this variable
> HINT: this may be a false positive if your program uses some custom
> stack unwind mechanism, swapcontext or vfork
> ```
> hist_browser__run isn't on the stack so the asan error looks legit.
> There's no clean init/exit on struct ui_browser so I may be trading a
> use-after-return for a memory leak, but that seems look a good trade
> anyway.
> 
> Thanks,
> Ian
> 
> > - Arnaldo
> >
> > > - Arnaldo
> > >
> > > > Fixes: 05e8b0804ec4 ("perf ui browser: Stop using 'self'")
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/ui/browser.c | 4 +++-
> > > >  tools/perf/ui/browser.h | 2 +-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> > > > index 603d11283cbd..c4cdf2ea69b7 100644
> > > > --- a/tools/perf/ui/browser.c
> > > > +++ b/tools/perf/ui/browser.c
> > > > @@ -287,7 +287,8 @@ int ui_browser__show(struct ui_browser *browser, const char *title,
> > > >     mutex_lock(&ui__lock);
> > > >     __ui_browser__show_title(browser, title);
> > > >
> > > > -   browser->title = title;
> > > > +   free(browser->title);
> > > > +   browser->title = strdup(title);
> > > >     zfree(&browser->helpline);
> > > >
> > > >     va_start(ap, helpline);
> > > > @@ -304,6 +305,7 @@ void ui_browser__hide(struct ui_browser *browser)
> > > >     mutex_lock(&ui__lock);
> > > >     ui_helpline__pop();
> > > >     zfree(&browser->helpline);
> > > > +   zfree(&browser->title);
> > > >     mutex_unlock(&ui__lock);
> > > >  }
> > > >
> > > > diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> > > > index 510ce4554050..6e98d5f8f71c 100644
> > > > --- a/tools/perf/ui/browser.h
> > > > +++ b/tools/perf/ui/browser.h
> > > > @@ -21,7 +21,7 @@ struct ui_browser {
> > > >     u8            extra_title_lines;
> > > >     int           current_color;
> > > >     void          *priv;
> > > > -   const char    *title;
> > > > +   char          *title;
> > > >     char          *helpline;
> > > >     const char    *no_samples_msg;
> > > >     void          (*refresh_dimensions)(struct ui_browser *browser);
> > > > --
> > > > 2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Arnaldo Carvalho de Melo]

On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > >
> > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > > ui_browser__show is capturing the input title that is stack allocated
> > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > > the string.
> > > >
> > > > But everything happens in that context, i.e. hist_brower__run() will
> > > > call ui_browser__ methods and then exit.
> > > >
> > > > We end up having browser->title pointing to returned stack memory
> > > > (invalid) but there will be no references to it, no?
> > > >
> > > > If we return to hist_browser__run() we then call ui_browser__show
> > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > > noticed an actual use-after-"free"?
> > >
> > > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > > real bug or if it just makes the code more future proof, i.e. to avoid
> > > us adding code that actually uses invalid stack memory.
> > 
> > My command line using tui is:
> > $ sudo bash -c 'rm /tmp/asan.log*; export
> > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> > sleep 1; /tmp/perf/perf mem report'
> > I then go to the perf annotate view and quit. This triggers the asan
> > error (from the log file):
> > ```
> 
> Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
> in the full details can hopefully find this message going from the Link:
> tag.

Nah, I added your explanation to the cset log message.

Thanks,

- Arnaldo

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Ian Rogers]

On Tue, May 7, 2024 at 2:07 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
> > > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > >
> > > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
> > > > > > ui_browser__show is capturing the input title that is stack allocated
> > > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
> > > > > > the string.
> > > > >
> > > > > But everything happens in that context, i.e. hist_brower__run() will
> > > > > call ui_browser__ methods and then exit.
> > > > >
> > > > > We end up having browser->title pointing to returned stack memory
> > > > > (invalid) but there will be no references to it, no?
> > > > >
> > > > > If we return to hist_browser__run() we then call ui_browser__show
> > > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
> > > > > noticed an actual use-after-"free"?
> > > >
> > > > And I'll take the patch, I'm just trying to figure it out if it fixed a
> > > > real bug or if it just makes the code more future proof, i.e. to avoid
> > > > us adding code that actually uses invalid stack memory.
> > >
> > > My command line using tui is:
> > > $ sudo bash -c 'rm /tmp/asan.log*; export
> > > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
> > > sleep 1; /tmp/perf/perf mem report'
> > > I then go to the perf annotate view and quit. This triggers the asan
> > > error (from the log file):
> > > ```
> >
> > Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
> > in the full details can hopefully find this message going from the Link:
> > tag.
>
> Nah, I added your explanation to the cset log message.


Okay, I found I needed this to avoid a segv introduced by this patch:
```
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index c4cdf2ea69b7..19503e838738 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -203,7 +203,7 @@ void ui_browser__refresh_dimensions(struct
ui_browser *browser)
void ui_browser__handle_resize(struct ui_browser *browser)
{
       ui__refresh_dimensions(false);
-       ui_browser__show(browser, browser->title, ui_helpline__current);
+       ui_browser__show(browser, browser->title ?: "", ui_helpline__current);
       ui_browser__refresh(browser);
}
```
I also found a use-after-free issue with patch 5. I'll send a v2.

Thanks,
Ian

> Thanks,
>
> - Arnaldo

Re: [PATCH v1 1/8] perf ui browser: Don't save pointer to stack memory

[Ian Rogers]

On Tue, May 7, 2024 at 6:07 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On Tue, May 7, 2024, 9:51 PM Ian Rogers <irogers@google.com> wrote:
>>
>> On Tue, May 7, 2024 at 2:07 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> >
>> > On Tue, May 07, 2024 at 06:04:43PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > On Tue, May 07, 2024 at 01:48:28PM -0700, Ian Rogers wrote:
>> > > > On Tue, May 7, 2024 at 1:22 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>> > > > >
>> > > > > On Tue, May 07, 2024 at 05:20:59PM -0300, Arnaldo Carvalho de Melo wrote:
>> > > > > > On Tue, May 07, 2024 at 11:35:38AM -0700, Ian Rogers wrote:
>> > > > > > > ui_browser__show is capturing the input title that is stack allocated
>> > > > > > > memory in hist_browser__run. Avoid a use after return by strdup-ing
>> > > > > > > the string.
>> > > > > >
>> > > > > > But everything happens in that context, i.e. hist_brower__run() will
>> > > > > > call ui_browser__ methods and then exit.
>> > > > > >
>> > > > > > We end up having browser->title pointing to returned stack memory
>> > > > > > (invalid) but there will be no references to it, no?
>> > > > > >
>> > > > > > If we return to hist_browser__run() we then call ui_browser__show
>> > > > > > passing a new title, for "live" stack memory, rinse repeat. Or have you
>> > > > > > noticed an actual use-after-"free"?
>> > > > >
>> > > > > And I'll take the patch, I'm just trying to figure it out if it fixed a
>> > > > > real bug or if it just makes the code more future proof, i.e. to avoid
>> > > > > us adding code that actually uses invalid stack memory.
>> > > >
>> > > > My command line using tui is:
>> > > > $ sudo bash -c 'rm /tmp/asan.log*; export
>> > > > ASAN_OPTIONS="log_path=/tmp/asan.log"; /tmp/perf/perf mem record -a
>> > > > sleep 1; /tmp/perf/perf mem report'
>> > > > I then go to the perf annotate view and quit. This triggers the asan
>> > > > error (from the log file):
>> > > > ```
>> > >
>> > > Thanks, it is indeed a bug, I'll keep that Fixes tag, people interested
>> > > in the full details can hopefully find this message going from the Link:
>> > > tag.
>> >
>> > Nah, I added your explanation to the cset log message.
>>
>>
>> Okay, I found I needed this to avoid a segv introduced by this patch:
>> ```
>> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
>> index c4cdf2ea69b7..19503e838738 100644
>> --- a/tools/perf/ui/browser.c
>> +++ b/tools/perf/ui/browser.c
>> @@ -203,7 +203,7 @@ void ui_browser__refresh_dimensions(struct
>> ui_browser *browser)
>> void ui_browser__handle_resize(struct ui_browser *browser)
>> {
>>        ui__refresh_dimensions(false);
>> -       ui_browser__show(browser, browser->title, ui_helpline__current);
>> +       ui_browser__show(browser, browser->title ?: "", ui_helpline__current);
>>        ui_browser__refresh(browser);
>> }
>> ```
>> I also found a use-after-free issue with patch 5. I'll send a v2.
>>
>> Please send a fix, it's already in perf-tools-next.
>

Okay. It looks like you accidentally pushed tmp.perf-tools.next, that
is a .next rather than a -next (dot not dash). I'll work from
perf-tools-next.

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
>> Thanks,
>> Ian
>>
>> > Thanks,
>> >
>> > - Arnaldo