From: Arnaldo Carvalho de Melo [mailto:[email protected]]
>
>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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
