* [PATCH v1] perf symbol-minimal: Fix double free in filename__read_build_id
@ 2025-05-01 7:00 Ian Rogers
2025-05-01 20:25 ` Namhyung Kim
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2025-05-01 7:00 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, linux-perf-users,
linux-kernel
Running the "perf script task-analyzer tests" with address sanitizer
showed a double free:
```
FAIL: "test_csv_extended_times" Error message: "Failed to find required string:'Out-Out;'."
=================================================================
==19190==ERROR: AddressSanitizer: attempting double-free on 0x50b000017b10 in thread T0:
#0 0x55da9601c78a in free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
#1 0x55da96640c63 in filename__read_build_id tools/perf/util/symbol-minimal.c:221:2
0x50b000017b10 is located 0 bytes inside of 112-byte region [0x50b000017b10,0x50b000017b80)
freed by thread T0 here:
#0 0x55da9601ce40 in realloc (perf+0x260e40) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
#1 0x55da96640ad6 in filename__read_build_id tools/perf/util/symbol-minimal.c:204:10
previously allocated by thread T0 here:
#0 0x55da9601ca23 in malloc (perf+0x260a23) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
#1 0x55da966407e7 in filename__read_build_id tools/perf/util/symbol-minimal.c:181:9
SUMMARY: AddressSanitizer: double-free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a) in free
==19190==ABORTING
FAIL: "invocation of perf script report task-analyzer --csv-summary csvsummary --summary-extended command failed" Error message: ""
FAIL: "test_csvsummary_extended" Error message: "Failed to find required string:'Out-Out;'."
---- end(-1) ----
132: perf script task-analyzer tests : FAILED!
```
The buf_size if always set to phdr->p_filesz, but that may be 0
causing a free and realloc to return NULL. This is treated in
filename__read_build_id like a failure and the buffer is freed again.
To avoid this problem only grow buf, meaning the buf_size will never
be 0. This also reduces the number of memory (re)allocations.
Fixes: b691f64360ec ("perf symbols: Implement poor man's ELF parser")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/symbol-minimal.c | 34 +++++++++++++++++---------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index c6f369b5d893..d8da3da01fe6 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -147,18 +147,19 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
if (phdr->p_type != PT_NOTE)
continue;
- buf_size = phdr->p_filesz;
offset = phdr->p_offset;
- tmp = realloc(buf, buf_size);
- if (tmp == NULL)
- goto out_free;
-
- buf = tmp;
+ if (phdr->p_filesz > buf_size) {
+ buf_size = phdr->p_filesz;
+ tmp = realloc(buf, buf_size);
+ if (tmp == NULL)
+ goto out_free;
+ buf = tmp;
+ }
fseek(fp, offset, SEEK_SET);
- if (fread(buf, buf_size, 1, fp) != 1)
+ if (fread(buf, phdr->p_filesz, 1, fp) != 1)
goto out_free;
- ret = read_build_id(buf, buf_size, bid, need_swap);
+ ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
if (ret == 0) {
ret = bid->size;
break;
@@ -199,18 +200,19 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
if (phdr->p_type != PT_NOTE)
continue;
- buf_size = phdr->p_filesz;
offset = phdr->p_offset;
- tmp = realloc(buf, buf_size);
- if (tmp == NULL)
- goto out_free;
-
- buf = tmp;
+ if (phdr->p_filesz > buf_size) {
+ buf_size = phdr->p_filesz;
+ tmp = realloc(buf, buf_size);
+ if (tmp == NULL)
+ goto out_free;
+ buf = tmp;
+ }
fseek(fp, offset, SEEK_SET);
- if (fread(buf, buf_size, 1, fp) != 1)
+ if (fread(buf, phdr->p_filesz, 1, fp) != 1)
goto out_free;
- ret = read_build_id(buf, buf_size, bid, need_swap);
+ ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
if (ret == 0) {
ret = bid->size;
break;
--
2.49.0.967.g6a0df3ecc3-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] perf symbol-minimal: Fix double free in filename__read_build_id
2025-05-01 7:00 [PATCH v1] perf symbol-minimal: Fix double free in filename__read_build_id Ian Rogers
@ 2025-05-01 20:25 ` Namhyung Kim
2025-05-02 16:32 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 3+ messages in thread
From: Namhyung Kim @ 2025-05-01 20:25 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Kan Liang, linux-perf-users, linux-kernel
Hi Ian,
On Thu, May 01, 2025 at 12:00:03AM -0700, Ian Rogers wrote:
> Running the "perf script task-analyzer tests" with address sanitizer
> showed a double free:
> ```
> FAIL: "test_csv_extended_times" Error message: "Failed to find required string:'Out-Out;'."
> =================================================================
> ==19190==ERROR: AddressSanitizer: attempting double-free on 0x50b000017b10 in thread T0:
> #0 0x55da9601c78a in free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> #1 0x55da96640c63 in filename__read_build_id tools/perf/util/symbol-minimal.c:221:2
>
> 0x50b000017b10 is located 0 bytes inside of 112-byte region [0x50b000017b10,0x50b000017b80)
> freed by thread T0 here:
> #0 0x55da9601ce40 in realloc (perf+0x260e40) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> #1 0x55da96640ad6 in filename__read_build_id tools/perf/util/symbol-minimal.c:204:10
>
> previously allocated by thread T0 here:
> #0 0x55da9601ca23 in malloc (perf+0x260a23) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> #1 0x55da966407e7 in filename__read_build_id tools/perf/util/symbol-minimal.c:181:9
>
> SUMMARY: AddressSanitizer: double-free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a) in free
> ==19190==ABORTING
> FAIL: "invocation of perf script report task-analyzer --csv-summary csvsummary --summary-extended command failed" Error message: ""
> FAIL: "test_csvsummary_extended" Error message: "Failed to find required string:'Out-Out;'."
> ---- end(-1) ----
> 132: perf script task-analyzer tests : FAILED!
> ```
>
> The buf_size if always set to phdr->p_filesz, but that may be 0
> causing a free and realloc to return NULL. This is treated in
> filename__read_build_id like a failure and the buffer is freed again.
>
> To avoid this problem only grow buf, meaning the buf_size will never
> be 0. This also reduces the number of memory (re)allocations.
Thanks for fixing this!
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
>
> Fixes: b691f64360ec ("perf symbols: Implement poor man's ELF parser")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/symbol-minimal.c | 34 +++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
> index c6f369b5d893..d8da3da01fe6 100644
> --- a/tools/perf/util/symbol-minimal.c
> +++ b/tools/perf/util/symbol-minimal.c
> @@ -147,18 +147,19 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
> if (phdr->p_type != PT_NOTE)
> continue;
>
> - buf_size = phdr->p_filesz;
> offset = phdr->p_offset;
> - tmp = realloc(buf, buf_size);
> - if (tmp == NULL)
> - goto out_free;
> -
> - buf = tmp;
> + if (phdr->p_filesz > buf_size) {
> + buf_size = phdr->p_filesz;
> + tmp = realloc(buf, buf_size);
> + if (tmp == NULL)
> + goto out_free;
> + buf = tmp;
> + }
> fseek(fp, offset, SEEK_SET);
> - if (fread(buf, buf_size, 1, fp) != 1)
> + if (fread(buf, phdr->p_filesz, 1, fp) != 1)
> goto out_free;
>
> - ret = read_build_id(buf, buf_size, bid, need_swap);
> + ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
> if (ret == 0) {
> ret = bid->size;
> break;
> @@ -199,18 +200,19 @@ int filename__read_build_id(const char *filename, struct build_id *bid)
> if (phdr->p_type != PT_NOTE)
> continue;
>
> - buf_size = phdr->p_filesz;
> offset = phdr->p_offset;
> - tmp = realloc(buf, buf_size);
> - if (tmp == NULL)
> - goto out_free;
> -
> - buf = tmp;
> + if (phdr->p_filesz > buf_size) {
> + buf_size = phdr->p_filesz;
> + tmp = realloc(buf, buf_size);
> + if (tmp == NULL)
> + goto out_free;
> + buf = tmp;
> + }
> fseek(fp, offset, SEEK_SET);
> - if (fread(buf, buf_size, 1, fp) != 1)
> + if (fread(buf, phdr->p_filesz, 1, fp) != 1)
> goto out_free;
>
> - ret = read_build_id(buf, buf_size, bid, need_swap);
> + ret = read_build_id(buf, phdr->p_filesz, bid, need_swap);
> if (ret == 0) {
> ret = bid->size;
> break;
> --
> 2.49.0.967.g6a0df3ecc3-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] perf symbol-minimal: Fix double free in filename__read_build_id
2025-05-01 20:25 ` Namhyung Kim
@ 2025-05-02 16:32 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-05-02 16:32 UTC (permalink / raw)
To: Namhyung Kim
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang,
linux-perf-users, linux-kernel
On Thu, May 01, 2025 at 01:25:27PM -0700, Namhyung Kim wrote:
> Hi Ian,
>
> On Thu, May 01, 2025 at 12:00:03AM -0700, Ian Rogers wrote:
> > Running the "perf script task-analyzer tests" with address sanitizer
> > showed a double free:
> > ```
> > FAIL: "test_csv_extended_times" Error message: "Failed to find required string:'Out-Out;'."
> > =================================================================
> > ==19190==ERROR: AddressSanitizer: attempting double-free on 0x50b000017b10 in thread T0:
> > #0 0x55da9601c78a in free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> > #1 0x55da96640c63 in filename__read_build_id tools/perf/util/symbol-minimal.c:221:2
> >
> > 0x50b000017b10 is located 0 bytes inside of 112-byte region [0x50b000017b10,0x50b000017b80)
> > freed by thread T0 here:
> > #0 0x55da9601ce40 in realloc (perf+0x260e40) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> > #1 0x55da96640ad6 in filename__read_build_id tools/perf/util/symbol-minimal.c:204:10
> >
> > previously allocated by thread T0 here:
> > #0 0x55da9601ca23 in malloc (perf+0x260a23) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a)
> > #1 0x55da966407e7 in filename__read_build_id tools/perf/util/symbol-minimal.c:181:9
> >
> > SUMMARY: AddressSanitizer: double-free (perf+0x26078a) (BuildId: e7ef50e08970f017a96fde6101c5e2491acc674a) in free
> > ==19190==ABORTING
> > FAIL: "invocation of perf script report task-analyzer --csv-summary csvsummary --summary-extended command failed" Error message: ""
> > FAIL: "test_csvsummary_extended" Error message: "Failed to find required string:'Out-Out;'."
> > ---- end(-1) ----
> > 132: perf script task-analyzer tests : FAILED!
> > ```
> >
> > The buf_size if always set to phdr->p_filesz, but that may be 0
> > causing a free and realloc to return NULL. This is treated in
> > filename__read_build_id like a failure and the buffer is freed again.
> >
> > To avoid this problem only grow buf, meaning the buf_size will never
> > be 0. This also reduces the number of memory (re)allocations.
>
> Thanks for fixing this!
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks, applied to perf-tools-next,
- Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-05-02 16:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-01 7:00 [PATCH v1] perf symbol-minimal: Fix double free in filename__read_build_id Ian Rogers
2025-05-01 20:25 ` Namhyung Kim
2025-05-02 16:32 ` Arnaldo Carvalho de Melo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.