Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756913AbbKRWXO (ORCPT ); Wed, 18 Nov 2015 17:23:14 -0500 Received: from mail.kernel.org ([198.145.29.136]:35535 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbbKRWXM (ORCPT ); Wed, 18 Nov 2015 17:23:12 -0500 Date: Wed, 18 Nov 2015 19:23:02 -0300 From: Arnaldo Carvalho de Melo To: Masami Hiramatsu Cc: Peter Zijlstra , linux-kernel@vger.kernel.org, Adrian Hunter , Ingo Molnar , Namhyung Kim , Jiri Olsa Subject: Re: [PATCH perf/core 02/13] perf: Make perf_exec_path always returns malloc'd string Message-ID: <20151118222302.GZ22729@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4159 Lines: 122 Em Wed, Nov 18, 2015 at 03:40:14PM +0900, Masami Hiramatsu escreveu: > Since system_path() returns malloc'd string if given path is > not an absolute path, perf_exec_path sometimes returns static > string and sometimes returns malloc'd string depends on the > environment variables or command options. > > This causes a memory leak because caller can not free the > returned string. > > This fixes perf_exec_path and system_path to always return > malloc'd string, so caller can always free it. > /* Returns the highest-priority, location to look for perf programs. */ > -const char *perf_exec_path(void) > +char *perf_exec_path(void) > { > - const char *env; > + char *env; > > if (argv_exec_path) > - return argv_exec_path; > + return strdup(argv_exec_path); So here you return strdup without checking if it fails > > env = getenv(EXEC_PATH_ENVIRONMENT); > if (env && *env) { > - return env; > + env = strdup(env); > + if (env) > + return env; But here you change the behaviour by, having a EXEC_PATH_ENVIRONMENT, fallback to PERF_EXEC_PATH if strdup fails, that in turn can end up returning NULL, since system_path() doesn't check the strdup() result? I wonder if in those cases we couldn't check the address range for the pointer being freed and realize it is in .bss, .data or that it is a stack variable? Maybe there is some malloc() friend that, given a pointer, tells that yeah, it was allocated? - Arnaldo > } > > return system_path(PERF_EXEC_PATH); > @@ -83,9 +85,11 @@ void setup_path(void) > { > const char *old_path = getenv("PATH"); > struct strbuf new_path = STRBUF_INIT; > + char *tmp = perf_exec_path(); > > - add_path(&new_path, perf_exec_path()); > + add_path(&new_path, tmp); > add_path(&new_path, argv0_path); > + free(tmp); > > if (old_path) > strbuf_addstr(&new_path, old_path); > diff --git a/tools/perf/util/exec_cmd.h b/tools/perf/util/exec_cmd.h > index bc4b915..48b4175 100644 > --- a/tools/perf/util/exec_cmd.h > +++ b/tools/perf/util/exec_cmd.h > @@ -3,10 +3,11 @@ > > extern void perf_set_argv_exec_path(const char *exec_path); > extern const char *perf_extract_argv0_path(const char *path); > -extern const char *perf_exec_path(void); > extern void setup_path(void); > extern int execv_perf_cmd(const char **argv); /* NULL terminated */ > extern int execl_perf_cmd(const char *cmd, ...); > -extern const char *system_path(const char *path); > +/* perf_exec_path and system_path return malloc'd string, caller must free it */ > +extern char *perf_exec_path(void); > +extern char *system_path(const char *path); > > #endif /* __PERF_EXEC_CMD_H */ > diff --git a/tools/perf/util/help.c b/tools/perf/util/help.c > index 86c37c4..fa1fc4a 100644 > --- a/tools/perf/util/help.c > +++ b/tools/perf/util/help.c > @@ -159,7 +159,7 @@ void load_command_list(const char *prefix, > struct cmdnames *other_cmds) > { > const char *env_path = getenv("PATH"); > - const char *exec_path = perf_exec_path(); > + char *exec_path = perf_exec_path(); > > if (exec_path) { > list_commands_in_dir(main_cmds, exec_path, prefix); > @@ -187,6 +187,7 @@ void load_command_list(const char *prefix, > sizeof(*other_cmds->names), cmdname_compare); > uniq(other_cmds); > } > + free(exec_path); > exclude_cmds(other_cmds, main_cmds); > } > > @@ -203,13 +204,14 @@ void list_commands(const char *title, struct cmdnames *main_cmds, > longest = other_cmds->names[i]->len; > > if (main_cmds->cnt) { > - const char *exec_path = perf_exec_path(); > + char *exec_path = perf_exec_path(); > printf("available %s in '%s'\n", title, exec_path); > printf("----------------"); > mput_char('-', strlen(title) + strlen(exec_path)); > putchar('\n'); > pretty_print_string_list(main_cmds, longest); > putchar('\n'); > + free(exec_path); > } > > if (other_cmds->cnt) { ----- End forwarded message ----- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/