cmds: static variable used stack after return #1924

yskelg · 2024-05-11T16:24:07Z

ASAN detect stack after return on this scenario.

$ uftrace record --agent --trace=off ./valkey-server

$ uftrace --pid `pidof valkey-server` --trace=on
$ uftrace --pid `pidof valkey-server` --trace=off

This patch fix #1915 static tmp_dirname variable not pointed command_live()'s char template array stack variable.

yskelg · 2024-05-11T16:30:27Z

cmds/live.c

 static void cleanup_tempdir(void)
 {
-	if (!tmp_dirname)
-		return;
-
 	remove_directory(tmp_dirname);


I think wrong dirname would be covered with opendir() cheking.

But let's check if tmp_dirname has a valid string first.

I think it's ok to check the first byte only.

if (tmp_dirname[0] == '\0') return;

I agree, Done!

namhyung · 2024-05-12T04:35:25Z

cmds/live.c

+#define TMP_DIR_NAME_SIZE 32
+
+static char tmp_dirname[TMP_DIR_NAME_SIZE] = {
+	'\0',


I think you can initialize it with LIVE_NAME below.

Done @namhyung 👍

namhyung · 2024-05-12T04:35:51Z

cmds/live.c

 static void cleanup_tempdir(void)
 {
-	if (!tmp_dirname)
-		return;
-
 	remove_directory(tmp_dirname);


But let's check if tmp_dirname has a valid string first.

namhyung · 2024-05-12T04:36:10Z

cmds/live.c

 	remove_directory(tmp_dirname);
-	tmp_dirname = NULL;
+	memset(tmp_dirname, 0, sizeof(char) * TMP_DIR_NAME_SIZE);


You can just use sizeof(tmp_dirname);

Thank for the idea!

namhyung · 2024-05-12T04:36:28Z

cmds/live.c

@@ -416,15 +417,14 @@ static int forward_options(struct uftrace_opts *opts)
 int command_live(int argc, char *argv[], struct uftrace_opts *opts)
 {
 #define LIVE_NAME "uftrace-live-XXXXXX"
-	char template[32] = "/tmp/" LIVE_NAME;
+	char template[TMP_DIR_NAME_SIZE] = "/tmp/" LIVE_NAME;


Now we can remove the template.

namhyung · 2024-05-12T04:40:08Z

cmds/live.c


+		unlink(template);
+
+		snprintf(tmp_dirname, strlen(template), "%s", template);


You should use sizeof(tmp_dirname) instead of strlen(template), otherwise it might not set the last NUL byte.

Thank you @namhyung. good advice! 👍

paranlee · 2024-05-15T14:51:53Z

cmds/live.c

 			if (errno != EPERM && errno != ENOENT)
-				pr_err("cannot access to /tmp");
+				pr_err("cannot access to " TMP_LIVE_NAME);


I think non-sense situation that someone makes directory or symbolic link named /tmp/uftrace-live-XXXXXX.

mkstemp would replace "XXXXXX" to a random string. You need to print the actual string in tmp_dirname.

paranlee · 2024-05-15T14:51:58Z

cmds/live.c

+#define LIVE_NAME "uftrace-live-XXXXXX"
+#define TMP_LIVE_NAME "/tmp/" LIVE_NAME


String comparison makes macro going to top of the code.

namhyung · 2024-05-18T00:45:31Z

cmds/live.c

 static void cleanup_tempdir(void)
 {
-	if (!tmp_dirname)
+	if (strncmp(TMP_LIVE_NAME, tmp_dirname, strlen(TMP_LIVE_NAME)) != 0 ||
+	    strncmp(LIVE_NAME, tmp_dirname, strlen(LIVE_NAME)) != 0) {


This condition is always true. You need to check if it has non-zero contents.

Done @namhyung 👍

namhyung · 2024-05-18T00:46:47Z

cmds/live.c

 			if (errno != EPERM && errno != ENOENT)
-				pr_err("cannot access to /tmp");
+				pr_err("cannot access to " TMP_LIVE_NAME);


mkstemp would replace "XXXXXX" to a random string. You need to print the actual string in tmp_dirname.

namhyung · 2024-05-18T00:47:01Z

cmds/live.c


 			if (fd < 0)
-				pr_err("cannot create temp name");
-			tmp_dirname = template;
+				pr_err("cannot create " LIVE_NAME);


Thank you @namhyung, I understand the mkstemp would replace "XXXXXX". Done!

namhyung

Also, this change is specific to live command. So the commit subject should start with "live:" instead of generic "cmds:".

ASAN detect stack after return on this scenario. $ uftrace record --agent --trace=off ./valkey-server $ uftrace --pid `pidof valkey-server` --trace=on $ uftrace --pid `pidof valkey-server` --trace=off This patch fix static tmp_dirname variable not pointed command_live()'s char template array stack variable. Signed-off-by: Yunseong Kim <yskelg@gmail.com>

namhyung

LGTM

yskelg force-pushed the update-ended-stack-pointer branch from 1d534cc to 4e913bf Compare May 11, 2024 16:27

yskelg commented May 11, 2024

View reviewed changes

yskelg force-pushed the update-ended-stack-pointer branch from 4e913bf to f5f513c Compare May 11, 2024 16:33

namhyung reviewed May 12, 2024

View reviewed changes

yskelg force-pushed the update-ended-stack-pointer branch 3 times, most recently from e3a4078 to 9afc188 Compare May 15, 2024 14:46

paranlee reviewed May 15, 2024

View reviewed changes

yskelg closed this May 15, 2024

yskelg force-pushed the update-ended-stack-pointer branch from 9afc188 to d79deba Compare May 15, 2024 14:56

yskelg reopened this May 15, 2024

namhyung reviewed May 18, 2024

View reviewed changes

yskelg force-pushed the update-ended-stack-pointer branch from 3532ea8 to 5218f60 Compare May 18, 2024 05:34

namhyung reviewed May 18, 2024

View reviewed changes

yskelg force-pushed the update-ended-stack-pointer branch from 5218f60 to 91e10b0 Compare May 18, 2024 15:50

yskelg force-pushed the update-ended-stack-pointer branch from 91e10b0 to ea31643 Compare May 18, 2024 15:54

namhyung approved these changes May 19, 2024

View reviewed changes

namhyung merged commit a4963f8 into namhyung:master May 21, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmds: static variable used stack after return #1924

cmds: static variable used stack after return #1924

yskelg commented May 11, 2024 •

edited

yskelg May 11, 2024

namhyung May 12, 2024

namhyung May 18, 2024

yskelg May 18, 2024

namhyung May 12, 2024

paranlee May 15, 2024

namhyung May 12, 2024

namhyung May 12, 2024

paranlee May 15, 2024 •

edited

namhyung May 12, 2024

paranlee May 15, 2024 •

edited

namhyung May 12, 2024

paranlee May 15, 2024

paranlee May 15, 2024 •

edited

namhyung May 18, 2024

paranlee May 15, 2024 •

edited

namhyung May 18, 2024

yskelg May 18, 2024

namhyung May 18, 2024

namhyung May 18, 2024

yskelg May 18, 2024

namhyung left a comment

namhyung left a comment


		unlink(template);

		snprintf(tmp_dirname, strlen(template), "%s", template);

		#define LIVE_NAME "uftrace-live-XXXXXX"
		#define TMP_LIVE_NAME "/tmp/" LIVE_NAME

cmds: static variable used stack after return #1924

cmds: static variable used stack after return #1924

Conversation

yskelg commented May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranlee May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranlee May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranlee May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranlee May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namhyung left a comment

Choose a reason for hiding this comment

namhyung left a comment

Choose a reason for hiding this comment

yskelg commented May 11, 2024 •

edited

paranlee May 15, 2024 •

edited

paranlee May 15, 2024 •

edited

paranlee May 15, 2024 •

edited

paranlee May 15, 2024 •

edited