linux-kernel - Re: [PATCH perf/core 02/13] perf: Make perf_exec

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-next>] [day] [month] [year] [list]

Message-ID: <20151118222302.GZ22729@kernel.org>
Date:	Wed, 18 Nov 2015 19:23:02 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-kernel@...r.kernel.org,
	Adrian Hunter <adrian.hunter@...el.com>,
	Ingo Molnar <mingo@...hat.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH perf/core 02/13] perf: Make perf_exec_path always returns
 malloc'd string

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/