by Ian Rogers

[permalink] [raw]

Subject: Re: [PATCH v2 17/18] perf header: Fix various error path memory leaks

On Sun, Oct 8, 2023 at 11:57 PM Namhyung Kim <[email protected]> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <[email protected]> wrote:
> >
> > Memory leaks were detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/header.c | 63 ++++++++++++++++++++++++----------------
> > 1 file changed, 38 insertions(+), 25 deletions(-)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index d812e1e371a7..41b78e40b22b 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2598,8 +2598,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> > goto error;
> >
> > /* include a NULL character at the end */
> > - if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > + if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > + free(str);
> > goto error;
> > + }
> > size += string_size(str);
> > free(str);
> > }
> > @@ -2617,8 +2619,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> > goto error;
> >
> > /* include a NULL character at the end */
> > - if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > + if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > + free(str);
> > goto error;
> > + }
> > size += string_size(str);
> > free(str);
> > }
> > @@ -2681,8 +2685,10 @@ static int process_cpu_topology(struct feat_fd *ff, void *data __maybe_unused)
> > goto error;
> >
> > /* include a NULL character at the end */
> > - if (strbuf_add(&sb, str, strlen(str) + 1) < 0)
> > + if (strbuf_add(&sb, str, strlen(str) + 1) < 0) {
> > + free(str);
> > goto error;
> > + }
> > size += string_size(str);
> > free(str);
> > }
>
> For these cases, it'd be simpler to free it in the error path.

It was a slightly larger change to do it that way, but I'll alter it.
The issue is not getting a double free on str, which is most easily
handled by switching frees to zfrees.

>
>
> > @@ -2736,10 +2742,9 @@ static int process_numa_topology(struct feat_fd *ff, void *data __maybe_unused)
> > goto error;
> >
> > n->map = perf_cpu_map__new(str);
> > + free(str);
> > if (!n->map)
> > goto error;
> > -
> > - free(str);
> > }
> > ff->ph->env.nr_numa_nodes = nr;
> > ff->ph->env.numa_nodes = nodes;
> > @@ -2913,10 +2918,10 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
> > return -1;
> >
> > for (i = 0; i < cnt; i++) {
> > - struct cpu_cache_level c;
> > + struct cpu_cache_level *c = &caches[i];
> >
> > #define _R(v) \
> > - if (do_read_u32(ff, &c.v))\
> > + if (do_read_u32(ff, &c->v)) \
> > goto out_free_caches; \
> >
> > _R(level)
> > @@ -2926,22 +2931,25 @@ static int process_cache(struct feat_fd *ff, void *data __maybe_unused)
> > #undef _R
> >
> > #define _R(v) \
> > - c.v = do_read_string(ff); \
> > - if (!c.v) \
> > - goto out_free_caches;
> > + c->v = do_read_string(ff); \
> > + if (!c->v) \
> > + goto out_free_caches; \
> >
> > _R(type)
> > _R(size)
> > _R(map)
> > #undef _R
> > -
> > - caches[i] = c;
> > }
> >
> > ff->ph->env.caches = caches;
> > ff->ph->env.caches_cnt = cnt;
> > return 0;
> > out_free_caches:
> > + for (i = 0; i < cnt; i++) {
> > + free(caches[i].type);
> > + free(caches[i].size);
> > + free(caches[i].map);
> > + }
> > free(caches);
> > return -1;
> > }
>
> Looks ok.
>
>
> > @@ -3585,18 +3593,16 @@ static int perf_header__adds_write(struct perf_header *header,
> > struct feat_copier *fc)
> > {
> > int nr_sections;
> > - struct feat_fd ff;
> > + struct feat_fd ff = {
> > + .fd = fd,
> > + .ph = header,
> > + };
>
> I'm fine with this change.
>
>
> > struct perf_file_section *feat_sec, *p;
> > int sec_size;
> > u64 sec_start;
> > int feat;
> > int err;
> >
> > - ff = (struct feat_fd){
> > - .fd = fd,
> > - .ph = header,
> > - };
> > -
> > nr_sections = bitmap_weight(header->adds_features, HEADER_FEAT_BITS);
> > if (!nr_sections)
> > return 0;
> > @@ -3623,6 +3629,7 @@ static int perf_header__adds_write(struct perf_header *header,
> > err = do_write(&ff, feat_sec, sec_size);
> > if (err < 0)
> > pr_debug("failed to write feature section\n");
> > + free(ff.buf);
>
> But it looks like false alarams. Since the feat_fd has fd
> and no buf, it won't allocate the buffer in do_write() and
> just use __do_write_fd(). So no need to free the buf.

This code looks like it has had a half-done optimization applied to it
- why have >1 buffer? why are we making the code's life hard? In
__do_write_buf there is a test that can be true even when a buf is
provided:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n125
```
if (ff->size < new_size) {
addr = realloc(ff->buf, new_size);
if (!addr)
return -ENOMEM;
ff->buf = addr;
```
In the case clang-tidy (I'll put the analysis below) has determined
that new_size may be greater than size (I believe the intent in the
code is they both evaluate to 0) and this causes the memory leak being
fixed here. I'll add a TODO comment in v3 but things like 'buf' are
opaque and not intention revealing in the code, which makes any kind
of interpretation hard.

I think again this is a good signal that there is worthwhile
simplification/cleanup in this code.

Thanks,
Ian

```
tools/perf/util/header.c:3628:6: warning: Potential leak of memory
pointed to by 'ff.buf' [clang-analyzer-unix.Malloc]
err = do_write(&ff, feat_sec, sec_size);
^
tools/perf/util/header.c:3778:9: note: Calling 'perf_session__do_write_header'
return perf_session__do_write_header(session, evlist, fd, true, fc);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3675:2: note: Loop condition is false.
Execution continues on line 3685
evlist__for_each_entry(session->evlist, evsel) {
^
tools/perf/util/evlist.h:276:2: note: expanded from macro
'evlist__for_each_entry'
__evlist__for_each_entry(&(evlist)->core.entries, evsel)
^
tools/perf/util/evlist.h:268:9: note: expanded from macro
'__evlist__for_each_entry'
list_for_each_entry(evsel, list, core.node)
^
tools/include/linux/list.h:458:2: note: expanded from macro
'list_for_each_entry'
for (pos = list_first_entry(head, typeof(*pos), member); \
^
tools/perf/util/header.c:3687:2: note: Loop condition is false.
Execution continues on line 3711
evlist__for_each_entry(evlist, evsel) {
^
tools/perf/util/evlist.h:276:2: note: expanded from macro
'evlist__for_each_entry'
__evlist__for_each_entry(&(evlist)->core.entries, evsel)
^
tools/perf/util/evlist.h:268:9: note: expanded from macro
'__evlist__for_each_entry'
list_for_each_entry(evsel, list, core.node)
^
tools/include/linux/list.h:458:2: note: expanded from macro
'list_for_each_entry'
for (pos = list_first_entry(head, typeof(*pos), member); \
^
tools/perf/util/header.c:3711:6: note: Assuming field 'data_offset' is
not equal to 0
if (!header->data_offset)
^~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3711:2: note: Taking false branch
if (!header->data_offset)
^
tools/perf/util/header.c:3715:6: note: 'at_exit' is true
if (at_exit) {
^~~~~~~
tools/perf/util/header.c:3715:2: note: Taking true branch
if (at_exit) {
^
tools/perf/util/header.c:3716:9: note: Calling 'perf_header__adds_write'
err = perf_header__adds_write(header, evlist, fd, fc);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3606:6: note: Assuming 'nr_sections' is not equal
to 0
if (!nr_sections)
^~~~~~~~~~~~
tools/perf/util/header.c:3606:2: note: Taking false branch
if (!nr_sections)
^
tools/perf/util/header.c:3610:6: note: Assuming 'feat_sec' is not equal to
NULL
if (feat_sec == NULL)
^~~~~~~~~~~~~~~~
tools/perf/util/header.c:3610:2: note: Taking false branch
if (feat_sec == NULL)
^
tools/perf/util/header.c:3618:2: note: Assuming 'feat' is < HEADER_FEAT_BITS
for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
^
tools/include/linux/bitops.h:54:7: note: expanded from macro 'for_each_set_bit'
(bit) < (size); \
^~~~~~~~~~~~~~
tools/perf/util/header.c:3618:2: note: Loop condition is true.
Entering loop body
for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
^
tools/include/linux/bitops.h:53:2: note: expanded from macro 'for_each_set_bit'
for ((bit) = find_first_bit((addr), (size)); \
^
tools/perf/util/header.c:3619:3: note: Taking true branch
if (do_write_feat(&ff, feat, &p, evlist, fc))
^
tools/perf/util/header.c:3618:2: note: Assuming 'feat' is >= HEADER_FEAT_BITS
for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
tools/include/linux/bitops.h:54:7: note: expanded from macro 'for_each_set_bit'
(bit) < (size); \
^~~~~~~~~~~~~~
tools/perf/util/header.c:3618:2: note: Loop condition is false.
Execution continues on line 3623
for_each_set_bit(feat, header->adds_features, HEADER_FEAT_BITS) {
^
tools/include/linux/bitops.h:53:2: note: expanded from macro 'for_each_set_bit'
for ((bit) = find_first_bit((addr), (size)); \
^
tools/perf/util/header.c:3628:8: note: Calling 'do_write'
err = do_write(&ff, feat_sec, sec_size);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:141:6: note: Assuming field 'buf' is non-null
if (!ff->buf)
^~~~~~~~
tools/perf/util/header.c:141:2: note: Taking false branch
if (!ff->buf)
^
tools/perf/util/header.c:143:9: note: Calling '__do_write_buf'
return __do_write_buf(ff, buf, size);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:117:6: note: Assuming the condition is false
if (size + ff->offset > max_size)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:117:2: note: Taking false branch
if (size + ff->offset > max_size)
^
tools/perf/util/header.c:120:9: note: Assuming the condition is true
while (size > (new_size - ff->offset))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:120:2: note: Loop condition is true.
Entering loop body
while (size > (new_size - ff->offset))
^
tools/perf/util/header.c:120:9: note: Assuming the condition is false
while (size > (new_size - ff->offset))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:120:2: note: Loop condition is false.
Execution continues on line 122
while (size > (new_size - ff->offset))
^
tools/perf/util/header.c:122:13: note: Assuming '_min1' is >= '_min2'
new_size = min(max_size, new_size);
^
tools/include/linux/kernel.h:53:2: note: expanded from macro 'min'
_min1 < _min2 ? _min1 : _min2; })
^~~~~~~~~~~~~
tools/perf/util/header.c:122:13: note: '?' condition is false
new_size = min(max_size, new_size);
^
tools/include/linux/kernel.h:53:2: note: expanded from macro 'min'
_min1 < _min2 ? _min1 : _min2; })
^
tools/perf/util/header.c:124:6: note: Assuming 'new_size' is > field 'size'
if (ff->size < new_size) {
^~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:124:2: note: Taking true branch
if (ff->size < new_size) {
^
tools/perf/util/header.c:125:10: note: Memory is allocated
addr = realloc(ff->buf, new_size);
^~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:126:7: note: Assuming 'addr' is non-null
if (!addr)
^~~~~
tools/perf/util/header.c:126:3: note: Taking false branch
if (!addr)
^
tools/perf/util/header.c:143:9: note: Returned allocated memory
return __do_write_buf(ff, buf, size);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3628:8: note: Returned allocated memory
err = do_write(&ff, feat_sec, sec_size);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/perf/util/header.c:3628:6: note: Potential leak of memory
pointed to by 'ff.buf'
err = do_write(&ff, feat_sec, sec_size);
```

> Thanks,
> Namhyung
>
>
> > free(feat_sec);
> > return err;
> > }
> > @@ -3630,11 +3637,11 @@ static int perf_header__adds_write(struct perf_header *header,
> > int perf_header__write_pipe(int fd)
> > {
> > struct perf_pipe_file_header f_header;
> > - struct feat_fd ff;
> > + struct feat_fd ff = {
> > + .fd = fd,
> > + };
> > int err;
> >
> > - ff = (struct feat_fd){ .fd = fd };
> > -
> > f_header = (struct perf_pipe_file_header){
> > .magic = PERF_MAGIC,
> > .size = sizeof(f_header),
> > @@ -3645,7 +3652,7 @@ int perf_header__write_pipe(int fd)
> > pr_debug("failed to write perf pipe header\n");
> > return err;
> > }
> > -
> > + free(ff.buf);
> > return 0;
> > }
> >
> > @@ -3658,11 +3665,12 @@ static int perf_session__do_write_header(struct perf_session *session,
> > struct perf_file_attr f_attr;
> > struct perf_header *header = &session->header;
> > struct evsel *evsel;
> > - struct feat_fd ff;
> > + struct feat_fd ff = {
> > + .fd = fd,
> > + };
> > u64 attr_offset;
> > int err;
> >
> > - ff = (struct feat_fd){ .fd = fd};
> > lseek(fd, sizeof(f_header), SEEK_SET);
> >
> > evlist__for_each_entry(session->evlist, evsel) {
> > @@ -3670,6 +3678,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> > err = do_write(&ff, evsel->core.id, evsel->core.ids * sizeof(u64));
> > if (err < 0) {
> > pr_debug("failed to write perf header\n");
> > + free(ff.buf);
> > return err;
> > }
> > }
> > @@ -3695,6 +3704,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> > err = do_write(&ff, &f_attr, sizeof(f_attr));
> > if (err < 0) {
> > pr_debug("failed to write perf header attribute\n");
> > + free(ff.buf);
> > return err;
> > }
> > }
> > @@ -3705,8 +3715,10 @@ static int perf_session__do_write_header(struct perf_session *session,
> >
> > if (at_exit) {
> > err = perf_header__adds_write(header, evlist, fd, fc);
> > - if (err < 0)
> > + if (err < 0) {
> > + free(ff.buf);
> > return err;
> > + }
> > }
> >
> > f_header = (struct perf_file_header){
> > @@ -3728,6 +3740,7 @@ static int perf_session__do_write_header(struct perf_session *session,
> >
> > lseek(fd, 0, SEEK_SET);
> > err = do_write(&ff, &f_header, sizeof(f_header));
> > + free(ff.buf);
> > if (err < 0) {
> > pr_debug("failed to write perf header\n");
> > return err;
> > --
> > 2.42.0.609.gbb76f46606-goog
> >