Re: [PATCH perf/core 02/13] perf: Make perf_exec_path always returns malloc'd string

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

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