Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933803AbbKSDqd (ORCPT ); Wed, 18 Nov 2015 22:46:33 -0500 Received: from mail7.hitachi.co.jp ([133.145.228.42]:42004 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933683AbbKSDqb convert rfc822-to-8bit (ORCPT ); Wed, 18 Nov 2015 22:46:31 -0500 From: =?iso-2022-jp?B?GyRCSj8+PjJtTCYbKEIgLyBISVJBTUFUVRskQiEkGyhCTUFTQU1J?= To: "'Arnaldo Carvalho de Melo'" CC: Peter Zijlstra , "linux-kernel@vger.kernel.org" , Adrian Hunter , "Ingo Molnar" , Namhyung Kim , Jiri Olsa Subject: RE: [PATCH perf/core 02/13] perf: Make perf_exec_path always returns malloc'd string Thread-Topic: [PATCH perf/core 02/13] perf: Make perf_exec_path always returns malloc'd string Thread-Index: AQHRIk+v20JT7sbbU0eSwQuAuI4KZZ6iibcQ Date: Thu, 19 Nov 2015 03:46:25 +0000 Message-ID: <50399556C9727B4D88A595C8584AAB3752624B05@GSjpTKYDCembx32.service.hitachi.net> References: <20151118222302.GZ22729@kernel.org> In-Reply-To: <20151118222302.GZ22729@kernel.org> Accept-Language: ja-JP, en-US Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.198.220.53] Content-Type: text/plain; charset="iso-2022-jp" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4682 Lines: 138 From: Arnaldo Carvalho de Melo [mailto: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? 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/