Hi Namhyung,
On Thu, Sep 27, 2012 at 01:02:05PM +0900, Namhyung Kim wrote:
> Hi Feng,
>
> On Wed, 26 Sep 2012 15:57:07 +0800, Feng Tang wrote:
> > On Tue, Sep 25, 2012 at 08:17:03AM -0300, Arnaldo Carvalho de Melo wrote:
> >> Em Tue, Sep 25, 2012 at 04:20:53PM +0800, Feng Tang escreveu:
> >> > On Tue, 25 Sep 2012 11:11:21 +0900
> >> > Namhyung Kim <namhy...@kernel.org> wrote:
> >> > > Ditto. Plus it might leak previous input_name.
> >> >
> >> > Nice catch, will check the return value of "strdup".
> >> >
> >> > For input_name mem leak, in some cases the input_name can't be called
> >> > with free(), like those got from parse "-i" option. In case the old
> >> > input_name is got from malloc through strdup, I think it's not a big
> >> > issue given that buffer will be freed any way when the application exit.
> >>
> >> I think this is a matter of discipline, leaking is bad, kernel or
> >> userspace, why special case it?
> >>
> >
> > I see, then we need make sure all "input_name" point to a malloced buffer,
> > here is a initial debug patch will only touch the annotate/report/script
> > cmds, pls review and more suggestions are welcomed:
>
> Well, how about adding a flag like "input_name_alloced" and checking it
> before new allocation? This way we can avoid needless strdup when
> runtime switching is not used.
Right, myself have checked that only the runtime switch will malloc a
buffer for the new "input_name", IOW "input_name" in other case can't be
called with free(). So one flag only inside hists.c is ok to cover this,
like the following code.
btw, thanks for your patch about the parse-events()! will try it out.
- Feng
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6048820..c29335a 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1125,6 +1125,13 @@ static inline void free_popup_options(char **options,
int n)
}
}
+/*
+ * Only runtime switching of perf data file will make "input_name" point
+ * to a malloced buffer. So add "is_input_name_malloced" flag to decide
+ * whether we need to call free() for current "input_name" during the switch.
+ */
+static bool is_input_name_malloced = false;
+
static int switch_data_file(void)
{
char *pwd, *options[256], *abs_path[256], *tmp;
@@ -1186,8 +1193,10 @@ close_file_and_continue:
if (choice < nr_options && choice >= 0) {
tmp = strdup(abs_path[choice]);
if (tmp) {
+ if (is_input_name_malloced)
+ free((void *)input_name);
input_name = tmp;
+ is_input_name_malloced = true;
ret = 0;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
