Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmds: static variable used stack after return #1924

Merged
merged 1 commit into from
May 21, 2024

Conversation

yskelg
Copy link
Contributor

@yskelg yskelg commented May 11, 2024

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 yskelg force-pushed the update-ended-stack-pointer branch from 1d534cc to 4e913bf Compare May 11, 2024 16:27
static void cleanup_tempdir(void)
{
if (!tmp_dirname)
return;

remove_directory(tmp_dirname);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, Done!

@yskelg yskelg force-pushed the update-ended-stack-pointer branch from 4e913bf to f5f513c Compare May 11, 2024 16:33
cmds/live.c Outdated
#define TMP_DIR_NAME_SIZE 32

static char tmp_dirname[TMP_DIR_NAME_SIZE] = {
'\0',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can initialize it with LIVE_NAME below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done @namhyung 👍

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

remove_directory(tmp_dirname);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cmds/live.c Outdated
remove_directory(tmp_dirname);
tmp_dirname = NULL;
memset(tmp_dirname, 0, sizeof(char) * TMP_DIR_NAME_SIZE);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use sizeof(tmp_dirname);

Copy link
Contributor

@paranlee paranlee May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the idea!

cmds/live.c Outdated
@@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can remove the template.

Copy link
Contributor

@paranlee paranlee May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. 👍

cmds/live.c Outdated

unlink(template);

snprintf(tmp_dirname, strlen(template), "%s", template);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @namhyung. good advice! 👍

@yskelg yskelg force-pushed the update-ended-stack-pointer branch 3 times, most recently from e3a4078 to 9afc188 Compare May 15, 2024 14:46
cmds/live.c Outdated
if (errno != EPERM && errno != ENOENT)
pr_err("cannot access to /tmp");
pr_err("cannot access to " TMP_LIVE_NAME);
Copy link
Contributor

@paranlee paranlee May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +16 to +17
#define LIVE_NAME "uftrace-live-XXXXXX"
#define TMP_LIVE_NAME "/tmp/" LIVE_NAME
Copy link
Contributor

@paranlee paranlee May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String comparison makes macro going to top of the code.

@yskelg yskelg closed this May 15, 2024
@yskelg yskelg force-pushed the update-ended-stack-pointer branch from 9afc188 to d79deba Compare May 15, 2024 14:56
@yskelg yskelg reopened this May 15, 2024
cmds/live.c Outdated
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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done @namhyung 👍

cmds/live.c Outdated
if (errno != EPERM && errno != ENOENT)
pr_err("cannot access to /tmp");
pr_err("cannot access to " TMP_LIVE_NAME);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cmds/live.c Outdated

if (fd < 0)
pr_err("cannot create temp name");
tmp_dirname = template;
pr_err("cannot create " LIVE_NAME);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@namhyung namhyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yskelg yskelg force-pushed the update-ended-stack-pointer branch from 5218f60 to 91e10b0 Compare May 18, 2024 15:50
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>
@yskelg yskelg force-pushed the update-ended-stack-pointer branch from 91e10b0 to ea31643 Compare May 18, 2024 15:54
Copy link
Owner

@namhyung namhyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@namhyung namhyung merged commit a4963f8 into namhyung:master May 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmds: static tmp_dirname used stack after return
3 participants