Date:   Sun, 28 Nov 2021 16:46:54 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Ian Rogers <irogers@google.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH] perf header: Fix memory leaks
Message-ID: <YaOkbojVb2gZtfCk@krava>
References: <20211118201730.2302927-1-irogers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211118201730.2302927-1-irogers@google.com>
Precedence: bulk

On Thu, Nov 18, 2021 at 12:17:30PM -0800, Ian Rogers wrote:
> These leaks were found with leak sanitizer running "perf pipe recording
> and injection test". In pipe mode feat_fd may hold onto an events struct
> that needs freeing. When string features are processed they may
> overwrite an already created string, so free this before the overwrite.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/header.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 79cce216727e..e3c1a532d059 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -2321,6 +2321,7 @@ static int perf_header__read_build_ids(struct perf_header *header,
>  #define FEAT_PROCESS_STR_FUN(__feat, __feat_env) \
>  static int process_##__feat(struct feat_fd *ff, void *data __maybe_unused) \
>  {\
> +	free(ff->ph->env.__feat_env);		     \

hm, how is this set before this callback is triggered?

jirka

>  	ff->ph->env.__feat_env = do_read_string(ff); \
>  	return ff->ph->env.__feat_env ? 0 : -ENOMEM; \
>  }
> @@ -4124,6 +4125,7 @@ int perf_event__process_feature(struct perf_session *session,
>  	struct perf_record_header_feature *fe = (struct perf_record_header_feature *)event;
>  	int type = fe->header.type;
>  	u64 feat = fe->feat_id;
> +	int ret = 0;
>  
>  	if (type < 0 || type >= PERF_RECORD_HEADER_MAX) {
>  		pr_warning("invalid record type %d in pipe-mode\n", type);
> @@ -4141,11 +4143,13 @@ int perf_event__process_feature(struct perf_session *session,
>  	ff.size = event->header.size - sizeof(*fe);
>  	ff.ph = &session->header;
>  
> -	if (feat_ops[feat].process(&ff, NULL))
> -		return -1;
> +	if (feat_ops[feat].process(&ff, NULL)) {
> +		ret = -1;
> +		goto out;
> +	}
>  
>  	if (!feat_ops[feat].print || !tool->show_feat_hdr)
> -		return 0;
> +		goto out;
>  
>  	if (!feat_ops[feat].full_only ||
>  	    tool->show_feat_hdr >= SHOW_FEAT_HEADER_FULL_INFO) {
> @@ -4154,8 +4158,9 @@ int perf_event__process_feature(struct perf_session *session,
>  		fprintf(stdout, "# %s info available, use -I to display\n",
>  			feat_ops[feat].name);
>  	}
> -
> -	return 0;
> +out:
> +	free_event_desc(ff.events);
> +	return ret;
>  }
>  
>  size_t perf_event__fprintf_event_update(union perf_event *event, FILE *fp)
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
>