Skip to contentSkip to conversation

perf: Make perf_exec_path always returns malloc'd string

PATCH New Arnaldo Carvalho de Melo 3 participants 4 messages Nov 18, 2015 – Nov 23, 2015 perf events

lore
Share & tools
lore

Arnaldo Carvalho de Melo

author maintainer
#0 lore raw

Em Wed, Nov 18, 2015 at 03:40:14PM +0900, Masami Hiramatsu escreveu:

Quoted 12 lines

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

Review

Quoted 9 lines
(quoted diff) +3 -3
-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

Review

Quoted 7 lines
(quoted diff) +3 -1
 	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

Quoted 61 lines
(quoted diff) +3 -1
 	}
 
 	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);
tools/perf/util/exec_cmd.h +3 -2
index bc4b915..48b4175 100644
@@ -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 */
tools/perf/util/help.c +3 -2
index 86c37c4..fa1fc4a 100644
@@ -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();

tools/perf/util/help.c void list_commands(const char *title, struct cmdnames *main_cmds, · line 217

Quoted 10 lines
tools/perf/util/help.c +1 -0
 		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 -----

#1 lore raw

From: Arnaldo Carvalho de Melo [mailto:[email protected]]

Quoted 27 lines

Em Wed, Nov 18, 2015 at 03:40:14PM +0900, Masami Hiramatsu escreveu:

Quoted 12 lines

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

Review

Quoted 9 lines
(quoted diff) +3 -3
-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

Quoted 1 line
Quoted 10 lines

Review

Quoted 6 lines
(quoted diff) +3 -1
 	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);
Quoted 5 lines

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,

Quoted 76 lines

- Arnaldo

Quoted 61 lines
(quoted diff) +3 -1
 	}
 
 	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);
tools/perf/util/exec_cmd.h +3 -2
index bc4b915..48b4175 100644
@@ -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 */
tools/perf/util/help.c +3 -2
index 86c37c4..fa1fc4a 100644
@@ -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();

tools/perf/util/help.c void list_commands(const char *title, struct cmdnames *main_cmds, · line 217

Quoted 10 lines
tools/perf/util/help.c +1 -0
 		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 -----

#2 lore raw

PATCH v2 1/1 perf: Make perf_exec_path always returns malloc'd string

In reply to Arnaldo Carvalho de Melo

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.

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v2:
- do not change the behavior if strdup is failed.

---
 tools/perf/util/exec_cmd.c |   21 +++++++++++----------
 tools/perf/util/exec_cmd.h |    5 +++--
 tools/perf/util/help.c     |    6 ++++--
 3 files changed, 18 insertions(+), 14 deletions(-)
tools/perf/util/exec_cmd.c +11 -10
index 7adf4ad..1099e92 100644
@@ -9,17 +9,17 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 	static const char *prefix = PREFIX;
 	struct strbuf d = STRBUF_INIT;
 
 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);
 
 	strbuf_addf(&d, "%s/%s", prefix, path);
 	path = strbuf_detach(&d, NULL);
-	return path;
+	return (char *)path;
 }
 
 const char *perf_extract_argv0_path(const char *argv0)

@@ -52,17 +52,16 @@ void perf_set_argv_exec_path(const char *exec_path)
 
 
 /* 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);
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
-	}
+	if (env && *env)
+		return strdup(env);
 
 	return system_path(PERF_EXEC_PATH);
 }

@@ -83,9 +82,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);
tools/perf/util/exec_cmd.h +3 -2
index bc4b915..48b4175 100644
@@ -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 */
tools/perf/util/help.c +4 -2
index 86c37c4..fa1fc4a 100644
@@ -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) {
merge bot
#3 lore raw

[tip:perf/core] perf tools: Make perf_exec_path() always return malloc'd string

perf tools: Make perf_exec_path() always return malloc'd string
Commit-ID:  c4068f51d40df151a661a384ab1309b11d7f012e
Gitweb:     http://git.kernel.org/tip/c4068f51d40df151a661a384ab1309b11d7f012e
Author:     Masami Hiramatsu <[email protected]>
AuthorDate: Thu, 19 Nov 2015 15:04:53 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 19 Nov 2015 13:19:19 -0300

perf tools: Make perf_exec_path() always return malloc'd string

Since system_path() returns malloc'd string if given path is not an absolute path, perf_exec_path() sometimes returns a static string and sometimes returns a malloc'd string depending on the environment variables or command options.

This may cause a memory leak because the caller can not unconditionally free the returned string.

This fixes perf_exec_path() and system_path() to always return a malloc'd string, so the caller can always free it.

Signed-off-by Masami Hiramatsu <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/exec_cmd.c | 21 +++++++++++----------
 tools/perf/util/exec_cmd.h |  5 +++--
 tools/perf/util/help.c     |  6 ++++--
 3 files changed, 18 insertions(+), 14 deletions(-)
tools/perf/util/exec_cmd.c +11 -10
index 7adf4ad..1099e92 100644
@@ -9,17 +9,17 @@
 static const char *argv_exec_path;
 static const char *argv0_path;
 
-const char *system_path(const char *path)
+char *system_path(const char *path)
 {
 	static const char *prefix = PREFIX;
 	struct strbuf d = STRBUF_INIT;
 
 	if (is_absolute_path(path))
-		return path;
+		return strdup(path);
 
 	strbuf_addf(&d, "%s/%s", prefix, path);
 	path = strbuf_detach(&d, NULL);
-	return path;
+	return (char *)path;
 }
 
 const char *perf_extract_argv0_path(const char *argv0)

@@ -52,17 +52,16 @@ void perf_set_argv_exec_path(const char *exec_path)
 
 
 /* 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);
 
 	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
-	}
+	if (env && *env)
+		return strdup(env);
 
 	return system_path(PERF_EXEC_PATH);
 }

@@ -83,9 +82,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);
tools/perf/util/exec_cmd.h +3 -2
index bc4b915..48b4175 100644
@@ -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 */
tools/perf/util/help.c +4 -2
index 86c37c4..fa1fc4a 100644
@@ -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) {