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 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/
