[PATCH v1 0/4] Segv and RC checking fixes

[PATCH v1 0/4] Segv and RC checking fixes

[Ian Rogers]

Resolve issues found with address and leak sanitizer by running 'perf
top' and 'perf mem'.

Ian Rogers (4):
  perf ui browser: Avoid segv on title
  perf comm: Fix comm_str__put for reference count checking
  perf report: Avoid segv in report__setup_sample_type
  perf thread: Fixes to thread__new

 tools/perf/builtin-report.c |  2 +-
 tools/perf/ui/browser.c     |  2 +-
 tools/perf/util/comm.c      | 45 +++++++++++++++++++++++++------------
 tools/perf/util/thread.c    | 14 +++++-------
 4 files changed, 38 insertions(+), 25 deletions(-)

-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 1/4] perf ui browser: Avoid segv on title

[Ian Rogers]

If the title is NULL then it can lead to a segv.

Fixes: 769e6a1e15bd ("perf ui browser: Don't save pointer to stack memory")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 }
 
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

Re: [PATCH v1 1/4] perf ui browser: Avoid segv on title

[Namhyung Kim]

On Tue, May 7, 2024 at 8:53 PM Ian Rogers <irogers@google.com> wrote:
>
> If the title is NULL then it can lead to a segv.

Just out of curiosity, do you know where it sets to NULL?

Thanks,
Namhyung

>
> Fixes: 769e6a1e15bd ("perf ui browser: Don't save pointer to stack memory")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/ui/browser.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);
>  }
>
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>

Re: [PATCH v1 1/4] perf ui browser: Avoid segv on title

[Ian Rogers]

On Wed, May 8, 2024 at 10:26 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, May 7, 2024 at 8:53 PM Ian Rogers <irogers@google.com> wrote:
> >
> > If the title is NULL then it can lead to a segv.
>
> Just out of curiosity, do you know where it sets to NULL?

Yes, the fixes patch added strdup and zfree, the NULL is coming from the zfree.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Fixes: 769e6a1e15bd ("perf ui browser: Don't save pointer to stack memory")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/ui/browser.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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);
> >  }
> >
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >

[PATCH v1 2/4] perf comm: Fix comm_str__put for reference count checking

[Ian Rogers]

Searching for the entry in the array needs to avoid the intermediate
pointer with reference count checking. Refactor the array removal to
binary search for the entry. Change the array to hold an entry with a
reference count (so the intermediate pointer can work) and remove from
the array when the reference count on a comm_str falls to 1.

Fixes: 13ca628716c6 ("perf comm: Add reference count checking to 'struct comm_str'")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/comm.c | 45 +++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/comm.c b/tools/perf/util/comm.c
index 1aa9a08e5b03..233f2b6edf52 100644
--- a/tools/perf/util/comm.c
+++ b/tools/perf/util/comm.c
@@ -19,6 +19,8 @@ static struct comm_strs {
 	int capacity;
 } _comm_strs;
 
+static void comm_strs__remove_if_last(struct comm_str *cs);
+
 static void comm_strs__init(void)
 {
 	init_rwsem(&_comm_strs.lock);
@@ -58,22 +60,15 @@ static struct comm_str *comm_str__get(struct comm_str *cs)
 
 static void comm_str__put(struct comm_str *cs)
 {
-	if (cs && refcount_dec_and_test(comm_str__refcnt(cs))) {
-		struct comm_strs *comm_strs = comm_strs__get();
-		int i;
+	if (!cs)
+		return;
 
-		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);
+	if (refcount_dec_and_test(comm_str__refcnt(cs))) {
 		RC_CHK_FREE(cs);
 	} else {
+		if (refcount_read(comm_str__refcnt(cs)) == 1)
+			comm_strs__remove_if_last(cs);
+
 		RC_CHK_PUT(cs);
 	}
 }
@@ -107,6 +102,28 @@ static int comm_str__search(const void *_key, const void *_member)
 	return strcmp(key, comm_str__str(member));
 }
 
+static void comm_strs__remove_if_last(struct comm_str *cs)
+{
+	struct comm_strs *comm_strs = comm_strs__get();
+
+	down_write(&comm_strs->lock);
+	/*
+	 * Are there only references from the array, if so remove the array
+	 * reference under the write lock so that we don't race with findnew.
+	 */
+	if (refcount_read(comm_str__refcnt(cs)) == 1) {
+		struct comm_str **entry;
+
+		entry = bsearch(comm_str__str(cs), comm_strs->strs, comm_strs->num_strs,
+				sizeof(struct comm_str *), comm_str__search);
+		comm_str__put(*entry);
+		for (int i = entry - comm_strs->strs; 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);
+}
+
 static struct comm_str *__comm_strs__find(struct comm_strs *comm_strs, const char *str)
 {
 	struct comm_str **result;
@@ -158,7 +175,7 @@ static struct comm_str *comm_strs__findnew(const char *str)
 		}
 	}
 	up_write(&comm_strs->lock);
-	return result;
+	return comm_str__get(result);
 }
 
 struct comm *comm__new(const char *str, u64 timestamp, bool exec)
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 3/4] perf report: Avoid segv in report__setup_sample_type

[Ian Rogers]

In some cases evsel->name is lazily initialized in evsel__name. If not
initialized passing NULL to strstr leads to a segv.

Fixes: ccb17caecfbd ("perf report: Set PERF_SAMPLE_DATA_SRC bit for Arm SPE event")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 0892b6e3e5e7..69618fb0110b 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -429,7 +429,7 @@ static int report__setup_sample_type(struct report *rep)
 		 * compatibility, set the bit if it's an old perf data file.
 		 */
 		evlist__for_each_entry(session->evlist, evsel) {
-			if (strstr(evsel->name, "arm_spe") &&
+			if (strstr(evsel__name(evsel), "arm_spe") &&
 				!(sample_type & PERF_SAMPLE_DATA_SRC)) {
 				evsel->core.attr.sample_type |= PERF_SAMPLE_DATA_SRC;
 				sample_type |= PERF_SAMPLE_DATA_SRC;
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog

[PATCH v1 4/4] perf thread: Fixes to thread__new

[Ian Rogers]

Freeing the thread on failure won't work with reference count
checking, use thread__delete. Don't allocate the comm_str, use a stack
allocation instead.

Fixes: f6005cafebab ("perf thread: Add reference count checking")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/thread.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 0a473112f881..87c59aa9fe38 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -39,12 +39,13 @@ int thread__init_maps(struct thread *thread, struct machine *machine)
 
 struct thread *thread__new(pid_t pid, pid_t tid)
 {
-	char *comm_str;
-	struct comm *comm;
 	RC_STRUCT(thread) *_thread = zalloc(sizeof(*_thread));
 	struct thread *thread;
 
 	if (ADD_RC_CHK(thread, _thread) != NULL) {
+		struct comm *comm;
+		char comm_str[32];
+
 		thread__set_pid(thread, pid);
 		thread__set_tid(thread, tid);
 		thread__set_ppid(thread, -1);
@@ -56,13 +57,8 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 		init_rwsem(thread__namespaces_lock(thread));
 		init_rwsem(thread__comm_lock(thread));
 
-		comm_str = malloc(32);
-		if (!comm_str)
-			goto err_thread;
-
-		snprintf(comm_str, 32, ":%d", tid);
+		snprintf(comm_str, sizeof(comm_str), ":%d", tid);
 		comm = comm__new(comm_str, 0, false);
-		free(comm_str);
 		if (!comm)
 			goto err_thread;
 
@@ -76,7 +72,7 @@ struct thread *thread__new(pid_t pid, pid_t tid)
 	return thread;
 
 err_thread:
-	free(thread);
+	thread__delete(thread);
 	return NULL;
 }
 
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog