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

record: Fix crash when the root is not existed #1979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zyxeeker
Copy link

@zyxeeker zyxeeker commented Nov 2, 2024

This patch to fix crash when the root is not existed, due to some embed devices maybe not have it.
source:

static void unlink_shmem_list(void)
{
        ...
	list_for_each_entry_safe(sl, tmp, &shmem_need_unlink, list) {
		...
		struct dirent **shmem_bufs;
		sscanf(sl->id, "/uftrace-%[^-]-%*d-%*d", shmem_session);
		...
		free(shmem_bufs);
                ...
	}
}

My SOC do not have /dev/shm/ and shmem_bufs's value has not initialized, so scandir() not flush shmem_bufs and it has bad address, then bad thing happened:

/userdata # ./uftrace record -vv ./prog_test
...
uftrace: flushing /uftrace-bd6de237f8a0b25b-1383-000
uftrace: unlink for session: bd6de237f8a0b25b
munmap_chunk(): invalid pointer
Aborted (core dumped)

So I add access() to fix it:

static void unlink_shmem_list(void)
{
        ...
	list_for_each_entry_safe(sl, tmp, &shmem_need_unlink, list) {
		...
		/* check the root is existed (due to some embed devices maybe not have it) */
		if (access(uftrace_shmem_root(), F_OK) == 0) {
			num = scandir(uftrace_shmem_root(), &shmem_bufs, filter_shmem, alphasort);
			for (i = 0; i < num; i++) {
				...
			}

			free(shmem_bufs);
		}
		else {
			pr_dbg2("access %s failed, err: %s\n", uftrace_shmem_root(), strerror(errno));
		}
                ...
	}
}

@zyxeeker zyxeeker marked this pull request as ready for review November 2, 2024 03: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.

Thanks for your fix. It looks good but we have 3 calls to uftrace_shmem_root() now. I think it's better to add a local variable for it.

@namhyung
Copy link
Owner

namhyung commented Nov 5, 2024

Also I'm curious if your system doesn't have a temp directory at all.

@zyxeeker
Copy link
Author

zyxeeker commented Nov 6, 2024

Thanks your reply!

Thanks for your fix. It looks good but we have 3 calls to uftrace_shmem_root() now. I think it's better to add a local variable for it.

The suggestion is good and I will check it again, so I will convert the issue to draft.

Also I'm curious if your system doesn't have a temp directory at all.

I have it, but many embed devices will use static /dev when RAM is limited, so maybe not have shm.
I will change pr_dbg2 to pr_warn to remind others.

@zyxeeker zyxeeker marked this pull request as draft November 6, 2024 02:50
@zyxeeker zyxeeker marked this pull request as ready for review November 6, 2024 06:04
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.

I think you can saved the result of uftrace_shmem_root() instead.

But I'm curious if it works on a system without shmem..

@zyxeeker
Copy link
Author

I think you can saved the result of uftrace_shmem_root() instead.

Thanks, I will modify it

But I'm curious if it works on a system without shmem..

Sorry for my bad explain, I mean mostly embedded devices will use static /dev and maybe not have shm folder.
Although create shm folder will solve this problem, but I think that`s not a best way.

This patch to fix crash when the root is not existed, due to some embed
devices maybe not have it

Signed-off-by: zyxeeker <[email protected]>
@zyxeeker zyxeeker marked this pull request as ready for review November 11, 2024 02:51
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.

2 participants