[PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[Kuan-Wei Chiu]

In the previous code, there was a memory leak issue where the previously
allocated memory was not freed upon a failed lseek operation. This patch
addresses the problem by releasing the old memory before returning -errno
in case of a lseek failure. This ensures that memory is properly managed
and avoids potential memory leaks.

Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
---
 tools/perf/util/hisi-ptt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
index 45b614bb73bf..43bd1ca62d58 100644
--- a/tools/perf/util/hisi-ptt.c
+++ b/tools/perf/util/hisi-ptt.c
@@ -108,8 +108,10 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
 		data_offset = 0;
 	} else {
 		data_offset = lseek(fd, 0, SEEK_CUR);
-		if (data_offset == -1)
+		if (data_offset == -1) {
+			free(data);
 			return -errno;
+		}
 	}
 
 	err = readn(fd, data, size);
-- 
2.25.1

Re: [PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[Namhyung Kim]

Hello,

On Sat, Sep 30, 2023 at 12:27 AM Kuan-Wei Chiu <visitorckw@gmail.com> wrote:
>
> In the previous code, there was a memory leak issue where the previously
> allocated memory was not freed upon a failed lseek operation. This patch
> addresses the problem by releasing the old memory before returning -errno
> in case of a lseek failure. This ensures that memory is properly managed
> and avoids potential memory leaks.
>
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/util/hisi-ptt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> index 45b614bb73bf..43bd1ca62d58 100644
> --- a/tools/perf/util/hisi-ptt.c
> +++ b/tools/perf/util/hisi-ptt.c
> @@ -108,8 +108,10 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
>                 data_offset = 0;
>         } else {
>                 data_offset = lseek(fd, 0, SEEK_CUR);
> -               if (data_offset == -1)
> +               if (data_offset == -1) {
> +                       free(data);
>                         return -errno;
> +               }
>         }
>
>         err = readn(fd, data, size);
> --
> 2.25.1
>

Re: [PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[Jonathan Cameron]

On Mon, 2 Oct 2023 21:48:14 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> Hello,
> 
> On Sat, Sep 30, 2023 at 12:27 AM Kuan-Wei Chiu <visitorckw@gmail.com> wrote:
> >
> > In the previous code, there was a memory leak issue where the previously
> > allocated memory was not freed upon a failed lseek operation. This patch
> > addresses the problem by releasing the old memory before returning -errno
> > in case of a lseek failure. This ensures that memory is properly managed
> > and avoids potential memory leaks.
> >
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>  
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> 
FYI, Ian Rogers has a series with a similar fix that goes a little further
and drops the assignment of data_offset seen just at the top of the context
below as it is unused.

https://lore.kernel.org/all/20231005230851.3666908-5-irogers@google.com/

I haven't replied to this yet as want to check some related handling
with one of my China based colleagues and it's a festival this week.

One of us will get back on this next week,

Jonathan

> Thanks,
> Namhyung
> 
> 
> > ---
> >  tools/perf/util/hisi-ptt.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> > index 45b614bb73bf..43bd1ca62d58 100644
> > --- a/tools/perf/util/hisi-ptt.c
> > +++ b/tools/perf/util/hisi-ptt.c
> > @@ -108,8 +108,10 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
> >                 data_offset = 0;
> >         } else {
> >                 data_offset = lseek(fd, 0, SEEK_CUR);
> > -               if (data_offset == -1)
> > +               if (data_offset == -1) {
> > +                       free(data);
> >                         return -errno;
> > +               }
> >         }
> >
> >         err = readn(fd, data, size);
> > --
> > 2.25.1
> >  
> 

Re: [PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[Namhyung Kim]

On Mon, Oct 2, 2023 at 9:48 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hello,
>
> On Sat, Sep 30, 2023 at 12:27 AM Kuan-Wei Chiu <visitorckw@gmail.com> wrote:
> >
> > In the previous code, there was a memory leak issue where the previously
> > allocated memory was not freed upon a failed lseek operation. This patch
> > addresses the problem by releasing the old memory before returning -errno
> > in case of a lseek failure. This ensures that memory is properly managed
> > and avoids potential memory leaks.
> >
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Applied to perf-tools-next, thanks!

Re: [PATCH] perf hisi-ptt: Fix memory leak in lseek failure handling

[Yicong Yang]

On 2023/9/30 15:27, Kuan-Wei Chiu wrote:
> In the previous code, there was a memory leak issue where the previously
> allocated memory was not freed upon a failed lseek operation. This patch
> addresses the problem by releasing the old memory before returning -errno
> in case of a lseek failure. This ensures that memory is properly managed
> and avoids potential memory leaks.
> 
> Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com>
> ---
>  tools/perf/util/hisi-ptt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/hisi-ptt.c b/tools/perf/util/hisi-ptt.c
> index 45b614bb73bf..43bd1ca62d58 100644
> --- a/tools/perf/util/hisi-ptt.c
> +++ b/tools/perf/util/hisi-ptt.c
> @@ -108,8 +108,10 @@ static int hisi_ptt_process_auxtrace_event(struct perf_session *session,
>  		data_offset = 0;
>  	} else {
>  		data_offset = lseek(fd, 0, SEEK_CUR);
> -		if (data_offset == -1)
> +		if (data_offset == -1) {
> +			free(data);
>  			return -errno;
> +		}

Sorry for the late reply, this looks correct to me:

Acked-by: Yicong Yang <yangyicong@hisilicon.com>

This patch makes me think whether data_offset is necessary for PTT.
The PTT only supports dump raw trace data and since this is an
uncore PMU we only have one single data source so we are not using
the auxtrace_queues for sorting or ordering the buffers. It seems
there's no need to record the data_offset for later mmap(),
just read trace data out and dump them is enough. I'll further check
and have some tests on this.

I see Namhyung has already taken this. Thanks for the fix.

>  	}
>  
>  	err = readn(fd, data, size);
>