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

Use *at functions for directory creation #124

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

Conversation

Vogtinator
Copy link
Member

Perform path traversal manually and do not follow symlinks.

Draft because some TODOs still open. Also absolutely untested.

Copy link

@jsegitz jsegitz 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 the changes. Since the path sources come from trusted system data that seems good enough. I added two suggestions

sbin/create_dirs_from_rpmdb.c Outdated Show resolved Hide resolved
sbin/create_dirs_from_rpmdb.c Outdated Show resolved Hide resolved
sbin/create_dirs_from_rpmdb.c Outdated Show resolved Hide resolved
Perform path traversal manually and do not follow symlinks.
continue;
}

if (mkdirat(parentdirfd, node_basename, node->fmode) < 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder whether there is a bug lingering here with umask and the mode parameter.

Copy link

Choose a reason for hiding this comment

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

I don't see a problem with this. Why is this concerning to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the desired mode of the directory has bits set that are umasked away, they'd be missing.

Copy link

Choose a reason for hiding this comment

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

since this is running in a special context I assumed it has no umask set. But yes, if there's on this will be a problem. So setting the umask explicitly is probably a good idea

@Vogtinator
Copy link
Member Author

Vogtinator commented Oct 18, 2024

Should be functionally complete now, but still untested. I converted some leftover uses of node_basename to use the directory's FD instead as well.

I don't like the goto driven cleanup, I'll try converting that to __attribute__((cleanup(...)).

@jsegitz
Copy link

jsegitz commented Jan 20, 2025

Should be functionally complete now, but still untested. I converted some leftover uses of node_basename to use the directory's FD instead as well.

I agree. Do we have tests for this in openQA that we can use to test it?

I don't like the goto driven cleanup, I'll try converting that to __attribute__((cleanup(...)).

Agreed. But from my POV that's not in scope for this, but feel free to add it

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