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-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 19 Nov 2015 03:46:25 +0000
From:	平松雅巳 / HIRAMATU，MASAMI 
	<masami.hiramatsu.pt@...achi.com>
To:	"'Arnaldo Carvalho de Melo'" <acme@...nel.org>
CC:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"linux-kernel@...r.kernel.org" <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

From: Arnaldo Carvalho de Melo [mailto:acme@...nel.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?

Ah, right. actually this is the first part where I fixed, and at that
point I thought this is better. But afterwards, I changed my mind to
return strdup directly. (If there is no memory caller must handle it)
So, I think the above should be

	if (env && *env)
		return strdup(env);

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

I wonder too. But as far as I took a look, I couldn't find such functions.

Thank you,

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