Message-ID: <CAP-5=fWtYPzGw0zvPd1VJc8RWi9cRQDg=ZF3Wjxf9yyY1QuPGA@mail.gmail.com>
Date:   Mon, 9 Oct 2023 10:13:20 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Yicong Yang <yangyicong@...ilicon.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        Yang Jihong <yangjihong1@...wei.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Ming Wang <wangming01@...ngson.cn>,
        Huacai Chen <chenhuacai@...nel.org>,
        Sean Christopherson <seanjc@...gle.com>,
        K Prateek Nayak <kprateek.nayak@....com>,
        Yanteng Si <siyanteng@...ngson.cn>,
        Yuan Can <yuancan@...wei.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        James Clark <james.clark@....com>, llvm@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        bpf@...r.kernel.org
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 <namhyung@...nel.org> wrote:
>
> On Thu, Oct 5, 2023 at 4:09 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > Memory leaks were detected by clang-tidy.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  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
> >

Powered by blists - more mailing lists