Re: [PATCH] perf data: Fix double free in perf_session__delete

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Feb 18, 2022 at 09:39:13AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Feb 18, 2022 at 02:30:08PM +0300, Alexey Bayduraev escreveu:
> > When perf_data__create_dir fails, it calls close_dir, but
> > perf_session__delete also calls close_dir and since dir.version and
> > dir.nr was initialized by perf_data__create_dir, a double free occurs.
> > This patch moves the initialization of dir.version and dir.nr after
> > successful initialization of dir.files, that prevents double freeing.
> > This behavior is already implemented in perf_data__open_dir. The patch
> > also adds a missing error message in case data directory creation fails.
> 
> Is this for perf/urgent or for perf/core? Probably perf/core as that
> record__threads_enabled(rec) call hints.
> 
> Please state for which branch the patch should be applied, something
> like;
> 
> [PATCH next] perf data: Fix double free in perf_session__delete()
> 
> Or:
> 
> [PATCH urgent] perf data: Fix double free in perf_session__delete()


So, please break this into two patches, one adding the pr_err(), because
it touches things that are not in perf/urgent, the threaded work, and
another fixing the double free at perf_session__delete(), because that
one should go in perf/urgent, as this problem predates the threaded
record patch series:

Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions")

Please add the above fixes for the patch fixing the double free.

Always think twice when writing 'this patch also does this extra thing',
usually this is an indication the patch should be split :-)

Thanks!

- Arnaldo
 
> > Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> > ---
> >  tools/perf/builtin-record.c | 4 +++-
> >  tools/perf/util/data.c      | 7 +++----
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 0bc6529814b2..0306d5911de2 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
> >  
> >  	if (record__threads_enabled(rec)) {
> >  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
> > -		if (ret)
> > +		if (ret) {
> > +			pr_err("Failed to create data directory: %s\n", strerror(errno));
> >  			return ret;
> > +		}
> >  		for (i = 0; i < evlist->core.nr_mmaps; i++) {
> >  			if (evlist->mmap)
> >  				evlist->mmap[i].file = &rec->data.dir.files[i];
> > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> > index f5d260b1df4d..15a4547d608e 100644
> > --- a/tools/perf/util/data.c
> > +++ b/tools/perf/util/data.c
> > @@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr)
> >  	if (!files)
> >  		return -ENOMEM;
> >  
> > -	data->dir.version = PERF_DIR_VERSION;
> > -	data->dir.files   = files;
> > -	data->dir.nr      = nr;
> > -
> >  	for (i = 0; i < nr; i++) {
> >  		struct perf_data_file *file = &files[i];
> >  
> > @@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr)
> >  		file->fd = ret;
> >  	}
> >  
> > +	data->dir.version = PERF_DIR_VERSION;
> > +	data->dir.files   = files;
> > +	data->dir.nr      = nr;
> >  	return 0;
> >  
> >  out_err:
> > -- 
> > 2.19.0