perf trace: Fix some more memory leaks

[PATCH v1] perf trace: Fix some more memory leaks

Posted by Ian Rogers 10 months, 1 week ago

The files.max is the maximum valid fd in the files array and so
freeing the values needs to be inclusive of the max value.

Add missing thread__put of the found parent thread in
thread__e_machine.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-trace.c | 3 ++-
 tools/perf/util/thread.c   | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 58a2ce3ff2db..c02ea4e8b270 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1657,7 +1657,7 @@ static const size_t trace__entry_str_size = 2048;
 
 static void thread_trace__free_files(struct thread_trace *ttrace)
 {
-	for (int i = 0; i < ttrace->files.max; ++i) {
+	for (int i = 0; i <= ttrace->files.max; ++i) {
 		struct file *file = ttrace->files.table + i;
 		zfree(&file->pathname);
 	}
@@ -1703,6 +1703,7 @@ static int trace__set_fd_pathname(struct thread *thread, int fd, const char *pat
 
 	if (file != NULL) {
 		struct stat st;
+
 		if (stat(pathname, &st) == 0)
 			file->dev_maj = major(st.st_rdev);
 		file->pathname = strdup(pathname);
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 89585f53c1d5..415c0e5d1e75 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -471,6 +471,7 @@ uint16_t thread__e_machine(struct thread *thread, struct machine *machine)
 
 		if (parent) {
 			e_machine = thread__e_machine(parent, machine);
+			thread__put(parent);
 			thread__set_e_machine(thread, e_machine);
 			return e_machine;
 		}
-- 
2.49.0.504.g3bcea36a83-goog

Re: [PATCH v1] perf trace: Fix some more memory leaks

Posted by Arnaldo Carvalho de Melo 9 months ago

On Tue, Apr 01, 2025 at 01:27:15PM -0700, Ian Rogers wrote:
> The files.max is the maximum valid fd in the files array and so
> freeing the values needs to be inclusive of the max value.
> 
> Add missing thread__put of the found parent thread in
> thread__e_machine.

Split it into:

⬢ [acme@toolbx perf-tools-next]$ git log --oneline -2
7900938850645ed4 (HEAD -> perf-tools-next) perf trace: Add missing thread__put() in thread__e_machine()
8830091383b03498 perf trace: Free the files.max entry in files->table
⬢ [acme@toolbx perf-tools-next]$ 

So that git --oneline is more descriptive, etc.

Thanks, applied to perf-tools-next,

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-trace.c | 3 ++-
>  tools/perf/util/thread.c   | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 58a2ce3ff2db..c02ea4e8b270 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1657,7 +1657,7 @@ static const size_t trace__entry_str_size = 2048;
>  
>  static void thread_trace__free_files(struct thread_trace *ttrace)
>  {
> -	for (int i = 0; i < ttrace->files.max; ++i) {
> +	for (int i = 0; i <= ttrace->files.max; ++i) {
>  		struct file *file = ttrace->files.table + i;
>  		zfree(&file->pathname);
>  	}
> @@ -1703,6 +1703,7 @@ static int trace__set_fd_pathname(struct thread *thread, int fd, const char *pat
>  
>  	if (file != NULL) {
>  		struct stat st;
> +
>  		if (stat(pathname, &st) == 0)
>  			file->dev_maj = major(st.st_rdev);
>  		file->pathname = strdup(pathname);
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 89585f53c1d5..415c0e5d1e75 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -471,6 +471,7 @@ uint16_t thread__e_machine(struct thread *thread, struct machine *machine)
>  
>  		if (parent) {
>  			e_machine = thread__e_machine(parent, machine);
> +			thread__put(parent);
>  			thread__set_e_machine(thread, e_machine);
>  			return e_machine;
>  		}
> -- 
> 2.49.0.504.g3bcea36a83-goog
>

Re: [PATCH v1] perf trace: Fix some more memory leaks

Posted by Namhyung Kim 9 months ago

On Thu, May 08, 2025 at 11:52:08AM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Apr 01, 2025 at 01:27:15PM -0700, Ian Rogers wrote:
> > The files.max is the maximum valid fd in the files array and so
> > freeing the values needs to be inclusive of the max value.
> > 
> > Add missing thread__put of the found parent thread in
> > thread__e_machine.
> 
> Split it into:
> 
> ⬢ [acme@toolbx perf-tools-next]$ git log --oneline -2
> 7900938850645ed4 (HEAD -> perf-tools-next) perf trace: Add missing thread__put() in thread__e_machine()
> 8830091383b03498 perf trace: Free the files.max entry in files->table
> ⬢ [acme@toolbx perf-tools-next]$ 
> 
> So that git --oneline is more descriptive, etc.
> 
> Thanks, applied to perf-tools-next,

PTAL this one as well.

https://lore.kernel.org/r/20250403054213.7021-1-namhyung@kernel.org

Thanks,
Namhyung

Re: [PATCH v1] perf trace: Fix some more memory leaks

Posted by Arnaldo Carvalho de Melo 9 months ago

On Thu, May 08, 2025 at 10:36:19AM -0700, Namhyung Kim wrote:
> On Thu, May 08, 2025 at 11:52:08AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Tue, Apr 01, 2025 at 01:27:15PM -0700, Ian Rogers wrote:
> > > The files.max is the maximum valid fd in the files array and so
> > > freeing the values needs to be inclusive of the max value.
> > > 
> > > Add missing thread__put of the found parent thread in
> > > thread__e_machine.
> > 
> > Split it into:
> > 
> > ⬢ [acme@toolbx perf-tools-next]$ git log --oneline -2
> > 7900938850645ed4 (HEAD -> perf-tools-next) perf trace: Add missing thread__put() in thread__e_machine()
> > 8830091383b03498 perf trace: Free the files.max entry in files->table
> > ⬢ [acme@toolbx perf-tools-next]$ 
> > 
> > So that git --oneline is more descriptive, etc.
> > 
> > Thanks, applied to perf-tools-next,
> 
> PTAL this one as well.
> 
> https://lore.kernel.org/r/20250403054213.7021-1-namhyung@kernel.org

Split into two patches, as one fixes one long standing problem (from
2017:

    perf trace: Fix leaks of 'struct thread' in set_filter_loop_pids()
    
    I've found some leaks from 'perf trace -a'.
    
    It seems there are more leaks but this is what I can find for now.
    
    Fixes: 082ab9a18e532864 ("perf trace: Filter out 'sshd' in the tracer ancestry in syswide tracing")

But the other fixes a more recent bug:

    perf trace: Fix leaks of 'struct thread' in fprintf_sys_enter()
    
    I've found some leaks from 'perf trace -a'.
    
    It seems there are more leaks but this is what I can find for now.
    
    Fixes: 70351029b55677eb ("perf thread: Add support for reading the e_machine type for a thread")

- Arnaldo

Re: [PATCH v1] perf trace: Fix some more memory leaks

Posted by Namhyung Kim 9 months ago

On Thu, May 08, 2025 at 05:11:45PM -0300, Arnaldo Carvalho de Melo wrote:
> On Thu, May 08, 2025 at 10:36:19AM -0700, Namhyung Kim wrote:
> > On Thu, May 08, 2025 at 11:52:08AM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Tue, Apr 01, 2025 at 01:27:15PM -0700, Ian Rogers wrote:
> > > > The files.max is the maximum valid fd in the files array and so
> > > > freeing the values needs to be inclusive of the max value.
> > > > 
> > > > Add missing thread__put of the found parent thread in
> > > > thread__e_machine.
> > > 
> > > Split it into:
> > > 
> > > ⬢ [acme@toolbx perf-tools-next]$ git log --oneline -2
> > > 7900938850645ed4 (HEAD -> perf-tools-next) perf trace: Add missing thread__put() in thread__e_machine()
> > > 8830091383b03498 perf trace: Free the files.max entry in files->table
> > > ⬢ [acme@toolbx perf-tools-next]$ 
> > > 
> > > So that git --oneline is more descriptive, etc.
> > > 
> > > Thanks, applied to perf-tools-next,
> > 
> > PTAL this one as well.
> > 
> > https://lore.kernel.org/r/20250403054213.7021-1-namhyung@kernel.org
> 
> Split into two patches, as one fixes one long standing problem (from
> 2017:
> 
>     perf trace: Fix leaks of 'struct thread' in set_filter_loop_pids()
>     
>     I've found some leaks from 'perf trace -a'.
>     
>     It seems there are more leaks but this is what I can find for now.
>     
>     Fixes: 082ab9a18e532864 ("perf trace: Filter out 'sshd' in the tracer ancestry in syswide tracing")
> 
> But the other fixes a more recent bug:
> 
>     perf trace: Fix leaks of 'struct thread' in fprintf_sys_enter()
>     
>     I've found some leaks from 'perf trace -a'.
>     
>     It seems there are more leaks but this is what I can find for now.
>     
>     Fixes: 70351029b55677eb ("perf thread: Add support for reading the e_machine type for a thread")
> 
> - Arnaldo

Thanks for doing this!
Namhyung

Re: [PATCH v1] perf trace: Fix some more memory leaks

Posted by Howard Chu 10 months, 1 week ago

Hello Ian,

On Tue, Apr 1, 2025 at 1:27 PM Ian Rogers <irogers@google.com> wrote:
>
> The files.max is the maximum valid fd in the files array and so
> freeing the values needs to be inclusive of the max value.
>
> Add missing thread__put of the found parent thread in
> thread__e_machine.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Howard Chu <howardchu95@gmail.com>

Thanks,
Howard