[PATCH v1 0/5] perf trace: Tracepoint format error handling and leak fixes

[PATCH v1 0/5] perf trace: Tracepoint format error handling and leak fixes

[Ian Rogers]

Running 'perf trace' under address sanitizer turned up a handful of
problems around the global tracepoint format cache.

trace_event__tp_format() encoded failures with ERR_PTR(), but
trace_event__tp_format_id() returned a plain NULL when the lookup missed
and tp_format() discarded the return value of tep_parse_format(), so two
of the three failure modes were indistinguishable from success to a
caller using IS_ERR(). syscall__read_info() then dereferenced the NULL.
Patch 1 drops the error pointers and reports failures as NULL with errno
set, which is what the callers were already testing for.

Patch 2 stops re-reading and re-parsing a format file that has already
been parsed. Each parse adds another tep_event to the handle and
libtraceevent can only free the whole handle, so a repeated lookup left
a duplicate behind for the rest of the session.

Patch 3 finally does what the TODO above the global has asked for since
the code was added, and frees the handle once the command is done.

That in turn exposed two leaks that had been hidden because the handle
kept them reachable: patch 4 frees the machine created by
machine__new_host(), which was released with machine__exit() rather than
machine__delete() and so leaked the allocation itself, and patch 5 frees
the buffer procfs__read_str() hands to thread__set_comm_from_proc(),
which is only freed when the read comes back empty.

With these, and with an unrelated libtraceevent fix I will send
separately to linux-trace-devel, 'perf trace' exits with no leaks
reported.

Tested on x86_64. Every patch builds individually, and the series also
builds with NO_LIBTRACEEVENT=1.

Ian Rogers (5):
  perf trace-event: Report tracepoint format errors with NULL and errno
  perf trace-event: Reuse an already parsed tracepoint format
  perf trace-event: Free the global trace_event when a command ends
  perf trace: Free the host machine allocation
  perf thread: Free the comm read from procfs

 tools/perf/builtin-kmem.c     |  3 +-
 tools/perf/builtin-sched.c    |  5 +-
 tools/perf/builtin-trace.c    | 12 ++---
 tools/perf/perf.c             |  6 +++
 tools/perf/util/evsel.c       |  5 +-
 tools/perf/util/thread.c      | 10 ++--
 tools/perf/util/trace-event.c | 90 +++++++++++++++++++++++++++--------
 tools/perf/util/trace-event.h |  1 +
 8 files changed, 91 insertions(+), 41 deletions(-)

-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v1 1/5] perf trace-event: Report tracepoint format errors with NULL and errno

[Ian Rogers]

trace_event__tp_format() encoded failure with ERR_PTR() while
trace_event__tp_format_id() returned a plain NULL when tep_find_event()
found nothing, and tp_format() itself returned NULL when
tep_parse_format() failed, as its return value was discarded. Callers
test with IS_ERR(), which NULL does not satisfy, so those two failures
were taken for success. In syscall__read_info() that leads straight to:

	if (IS_ERR(sc->tp_format)) {
		...
		return err;
	}
	if (syscall__alloc_arg_fmts(sc, sc->tp_format->format.nr_fields - 1))

which dereferences NULL when a format file fails to parse.

Mixing encoded error pointers with pointers that are compared against
NULL is what allows that to happen, so drop ERR_PTR() here and report
failures the way the rest of these paths already expect, by returning
NULL with errno set. evsel__tp_format() no longer has to translate the
error back into errno before printing it with %m, and the remaining
callers become NULL tests.

syscall__scnprintf_args() gains back its fallback of printing raw
arguments: it asked for IS_ERR(sc->tp_format), but syscall__read_info()
had already replaced the error pointer with NULL, so the branch could
never be taken.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-kmem.c     |  3 +-
 tools/perf/builtin-sched.c    |  5 ++--
 tools/perf/builtin-trace.c    | 10 +++----
 tools/perf/util/evsel.c       |  5 +---
 tools/perf/util/trace-event.c | 52 ++++++++++++++++++++++++++---------
 5 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index e1b2f5bc1ba8..8693c6b135ca 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -1870,8 +1870,7 @@ static bool slab_legacy_tp_is_exposed(void)
 	 * means the tool is running on an old kernel, we need to
 	 * rollback to support these legacy tracepoints.
 	 */
-	return IS_ERR(trace_event__tp_format("kmem", "kmalloc_node")) ?
-		false : true;
+	return trace_event__tp_format("kmem", "kmalloc_node");
 }
 
 static int __cmd_record(int argc, const char **argv)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index dd39a4fb6c7a..5b4092ae33ee 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -5182,8 +5182,7 @@ static bool schedstat_events_exposed(void)
 	 * Select "sched:sched_stat_wait" event to check
 	 * whether schedstat tracepoints are exposed.
 	 */
-	return IS_ERR(trace_event__tp_format("sched", "sched_stat_wait")) ?
-		false : true;
+	return trace_event__tp_format("sched", "sched_stat_wait");
 }
 
 static int __cmd_record(int argc, const char **argv)
@@ -5240,7 +5239,7 @@ static int __cmd_record(int argc, const char **argv)
 
 	rec_argv[i++] = strdup("-e");
 	waking_event = trace_event__tp_format("sched", "sched_waking");
-	if (!IS_ERR(waking_event))
+	if (waking_event)
 		rec_argv[i++] = strdup("sched:sched_waking");
 	else
 		rec_argv[i++] = strdup("sched:sched_wakeup");
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 20fffc24507b..5bd62b61287e 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2385,7 +2385,7 @@ static int syscall__read_info(struct syscall *sc, struct trace *trace)
 	snprintf(tp_name, sizeof(tp_name), "sys_enter_%s", sc->name);
 	sc->tp_format = trace_event__tp_format("syscalls", tp_name);
 
-	if (IS_ERR(sc->tp_format) && sc->fmt && sc->fmt->alias) {
+	if (!sc->tp_format && sc->fmt && sc->fmt->alias) {
 		snprintf(tp_name, sizeof(tp_name), "sys_enter_%s", sc->fmt->alias);
 		sc->tp_format = trace_event__tp_format("syscalls", tp_name);
 	}
@@ -2394,11 +2394,9 @@ static int syscall__read_info(struct syscall *sc, struct trace *trace)
 	 * Fails to read trace point format via sysfs node, so the trace point
 	 * doesn't exist.  Set the 'nonexistent' flag as true.
 	 */
-	if (IS_ERR(sc->tp_format)) {
+	if (!sc->tp_format) {
 		sc->nonexistent = true;
-		err = PTR_ERR(sc->tp_format);
-		sc->tp_format = NULL;
-		return err;
+		return -errno;
 	}
 
 	/*
@@ -2681,7 +2679,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
 			printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
 								  bf + printed, size - printed, &arg, val);
 		}
-	} else if (IS_ERR(sc->tp_format)) {
+	} else if (!sc->tp_format) {
 		/*
 		 * If we managed to read the tracepoint /format file, then we
 		 * may end up not having any args, like with gettid(), so only
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index c663aafa88b2..2570ea8d5d7b 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -720,10 +720,7 @@ struct tep_event *evsel__tp_format(struct evsel *evsel)
 	else
 		tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
 
-	if (IS_ERR(tp_format)) {
-		int err = -PTR_ERR(tp_format);
-
-		errno = err;
+	if (!tp_format) {
 		pr_err("Error getting tracepoint format '%s': %m\n",
 			evsel__name(evsel));
 		return NULL;
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 000c1e1d68c1..10a7652f0305 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -7,7 +7,6 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <linux/kernel.h>
-#include <linux/err.h>
 #include <event-parse.h>
 #include <api/fs/tracing_path.h>
 #include <api/fs/fs.h>
@@ -77,7 +76,7 @@ void trace_event__cleanup(struct trace_event *t)
 }
 
 /*
- * Returns pointer with encoded error via <linux/err.h> interface.
+ * Returns NULL and sets errno on failure.
  */
 static struct tep_event*
 tp_format(const char *sys, const char *name)
@@ -90,38 +89,65 @@ tp_format(const char *sys, const char *name)
 	char *data;
 	int err;
 
-	if (!tp_dir)
-		return ERR_PTR(-errno);
+	if (!tp_dir) {
+		errno = ENOMEM;
+		return NULL;
+	}
 
 	scnprintf(path, PATH_MAX, "%s/%s/format", tp_dir, name);
 	put_events_file(tp_dir);
 
 	err = filename__read_str(path, &data, &size);
-	if (err)
-		return ERR_PTR(err);
+	if (err) {
+		errno = -err;
+		return NULL;
+	}
 
-	tep_parse_format(pevent, &event, data, size, sys);
+	err = tep_parse_format(pevent, &event, data, size, sys);
 
 	free(data);
+
+	/*
+	 * A parse failure leaves no event behind, report it rather than
+	 * letting a NULL be mistaken for a successfully parsed format.
+	 */
+	if (err != TEP_ERRNO__SUCCESS || !event) {
+		errno = EINVAL;
+		return NULL;
+	}
+
 	return event;
 }
 
 /*
- * Returns pointer with encoded error via <linux/err.h> interface.
+ * Returns NULL and sets errno on failure.
  */
 struct tep_event*
 trace_event__tp_format(const char *sys, const char *name)
 {
-	if (!tevent_initialized && trace_event__init2())
-		return ERR_PTR(-ENOMEM);
+	if (!tevent_initialized && trace_event__init2()) {
+		errno = ENOMEM;
+		return NULL;
+	}
 
 	return tp_format(sys, name);
 }
 
+/*
+ * Returns NULL and sets errno on failure.
+ */
 struct tep_event *trace_event__tp_format_id(int id)
 {
-	if (!tevent_initialized && trace_event__init2())
-		return ERR_PTR(-ENOMEM);
+	struct tep_event *event;
 
-	return tep_find_event(tevent.pevent, id);
+	if (!tevent_initialized && trace_event__init2()) {
+		errno = ENOMEM;
+		return NULL;
+	}
+
+	event = tep_find_event(tevent.pevent, id);
+	if (!event)
+		errno = ENOENT;
+
+	return event;
 }
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v1 2/5] perf trace-event: Reuse an already parsed tracepoint format

[Ian Rogers]

tp_format() read and parsed the format file on every call. Each parse
registers another tep_event with the global tep handle, and
libtraceevent has no way to free an individual event, only the whole
handle, so a repeated lookup of the same tracepoint both redoes the
work of reading and parsing the file and leaves a duplicate behind for
the rest of the session.

Ask the handle for the event first with tep_find_event_by_name() and
only fall back to reading the format file when it has not been parsed
yet. The handle holds formats for the running kernel alone, so a name
that is found is the same format that would have been read.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/trace-event.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 10a7652f0305..826f75464171 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -81,14 +81,25 @@ void trace_event__cleanup(struct trace_event *t)
 static struct tep_event*
 tp_format(const char *sys, const char *name)
 {
-	char *tp_dir = get_events_file(sys);
 	struct tep_handle *pevent = tevent.pevent;
-	struct tep_event *event = NULL;
+	struct tep_event *event;
+	char *tp_dir;
 	char path[PATH_MAX];
 	size_t size;
 	char *data;
 	int err;
 
+	/*
+	 * Each parse adds an event to the tep handle that can only be freed
+	 * by freeing the whole handle, so re-reading a format file both
+	 * repeats the work and grows the handle with a duplicate. Reuse the
+	 * event if it was already parsed.
+	 */
+	event = tep_find_event_by_name(pevent, sys, name);
+	if (event)
+		return event;
+
+	tp_dir = get_events_file(sys);
 	if (!tp_dir) {
 		errno = ENOMEM;
 		return NULL;
@@ -103,6 +114,7 @@ tp_format(const char *sys, const char *name)
 		return NULL;
 	}
 
+	event = NULL;
 	err = tep_parse_format(pevent, &event, data, size, sys);
 
 	free(data);
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v1 3/5] perf trace-event: Free the global trace_event when a command ends

[Ian Rogers]

The tep handle behind trace_event__tp_format() is a process wide cache
of the running kernel's tracepoint formats and nothing ever released
it, as the comment above it had noted since the code was added.

Add trace_event__exit() and call it from run_builtin() next to the
existing perf_config__exit(). By then the builtin has returned, so the
tep_event pointers it handed out, such as the ones cached in
evsel->tp_format, are no longer reachable.

Note that this does not silence every libtraceevent report under
leak sanitizer. Parsing a print fmt drops a token inside the library
itself, and as those allocations are not linked into the handle,
freeing the handle cannot reclaim them.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/perf.c             |  6 ++++++
 tools/perf/util/trace-event.c | 22 +++++++++++++++++-----
 tools/perf/util/trace-event.h |  1 +
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 6c5baa285b13..d60bc929c257 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -32,6 +32,9 @@
 #include "util/build-id.h"
 #include "util/config.h"
 #include "util/debug.h"
+#ifdef HAVE_LIBTRACEEVENT
+#include "util/trace-event.h"
+#endif
 
 const char perf_usage_string[] =
 	"perf [--version] [--help] [OPTIONS] COMMAND [ARGS]";
@@ -379,6 +382,9 @@ static int run_builtin(const struct cmd_struct *p, int argc, const char **argv)
 
 	status = p->fn(argc, argv);
 	perf_config__exit();
+#ifdef HAVE_LIBTRACEEVENT
+	trace_event__exit();
+#endif
 	exit_browser(status);
 
 	if (status)
diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index 826f75464171..819542a73f55 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -14,11 +14,9 @@
 #include "machine.h"
 
 /*
- * global trace_event object used by trace_event__tp_format
- *
- * TODO There's no cleanup call for this. Add some sort of
- * __exit function support and call trace_event__cleanup
- * there.
+ * Global trace_event object used by trace_event__tp_format. It caches the
+ * tracepoint formats of the running kernel for the lifetime of the command
+ * and is released by trace_event__exit.
  */
 static struct trace_event tevent;
 static bool tevent_initialized;
@@ -75,6 +73,20 @@ void trace_event__cleanup(struct trace_event *t)
 	t->plugin_list = NULL;
 }
 
+/*
+ * Release the global trace_event. Called once the command is done, when the
+ * tep_event pointers handed out by trace_event__tp_format are no longer in
+ * use.
+ */
+void trace_event__exit(void)
+{
+	if (!tevent_initialized)
+		return;
+
+	trace_event__cleanup(&tevent);
+	tevent_initialized = false;
+}
+
 /*
  * Returns NULL and sets errno on failure.
  */
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index 720121c74f1d..1c342fce36bb 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -32,6 +32,7 @@ bool have_tracepoints(struct list_head *evlist);
 
 int trace_event__init(struct trace_event *t);
 void trace_event__cleanup(struct trace_event *t);
+void trace_event__exit(void);
 int trace_event__register_resolver(struct machine *machine,
 				   tep_func_resolver_t *func);
 struct tep_event*
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v1 4/5] perf trace: Free the host machine allocation

[Ian Rogers]

trace__symbols_init() creates the host machine with machine__new_host(),
which allocates a struct machine, but trace__symbols__exit() released it
with machine__exit(). That tears down the contents of the machine and
leaves the allocation itself behind, so use machine__delete(), which
does both. The replay path points trace->host at the machine embedded in
the session and never reaches here, so nothing else is affected.

The leak was hidden from leak sanitizer because trace__symbols_init()
passes the machine to trace_event__register_resolver(), which stores it
as the private pointer of the global tep handle. The handle kept the
machine reachable for as long as the process lived, so it was only once
the handle started being freed that this was reported:

  Direct leak of 1256 byte(s) in 1 object(s) allocated from:
      #1 __machine__new_host util/machine.c:135
      #2 machine__new_host util/machine.c:155
      #3 trace__symbols_init builtin-trace.c:2105
      #4 trace__run builtin-trace.c:4759
      #5 cmd_trace builtin-trace.c:6091

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 5bd62b61287e..e1a4abf44673 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2132,7 +2132,7 @@ static int trace__symbols_init(struct trace *trace, int argc, const char **argv,
 
 static void trace__symbols__exit(struct trace *trace)
 {
-	machine__exit(trace->host);
+	machine__delete(trace->host);
 	trace->host = NULL;
 
 	perf_env__exit(&trace->host_env);
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v1 5/5] perf thread: Free the comm read from procfs

[Ian Rogers]

procfs__read_str() allocates the buffer it hands back and
thread__set_comm() only stores an interned copy of the string, so the
caller owns the buffer. thread__set_comm_from_proc() freed it when the
read came back empty but not once it had been used, leaking it on
every successful call.

Free it on both paths. The empty read still returns -1, err being
untouched in that case.

Found with leak sanitizer while running 'perf trace':

  Direct leak of 7 byte(s) in 1 object(s) allocated from:
      #1 io__getdelim fs/../io.h:179
      #2 filename__read_str fs/fs.c:365
      #3 procfs__read_str fs/fs.c:402
      #4 thread__set_comm_from_proc util/thread.c:297
      #5 syscall_arg__scnprintf_pid trace/beauty/pid.c:15

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/thread.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index f0d3773d87db..fcf7c78ab767 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -296,12 +296,12 @@ int thread__set_comm_from_proc(struct thread *thread)
 		       thread__pid(thread), thread__tid(thread)) >= (int)sizeof(path)) &&
 	    procfs__read_str(path, &comm, &sz) == 0) {
 		/* sz==0: read got nothing, e.g. race during exit teardown */
-		if (sz == 0) {
-			free(comm);
-			return -1;
+		if (sz > 0) {
+			comm[sz - 1] = '\0';
+			err = thread__set_comm(thread, comm, 0);
 		}
-		comm[sz - 1] = '\0';
-		err = thread__set_comm(thread, comm, 0);
+		/* thread__set_comm() copies the string, so release the buffer. */
+		free(comm);
 	}
 
 	return err;
-- 
2.55.0.1082.g2b9226bbc0-goog