lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Yg+U0dRdmHvO6sku@kernel.org>
Date:   Fri, 18 Feb 2022 09:45:05 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Antonov <alexander.antonov@...ux.intel.com>,
        Alexei Budankov <abudankov@...wei.com>
Subject: Re: [PATCH] perf data: Fix double free in perf_session__delete

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@...ux.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ