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

vartree: update owner and mode on replaced directories #1381

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

Conversation

henning-schild
Copy link

No description provided.

Signed-off-by: Henning Schild <henning@hennsch.de>
@henning-schild
Copy link
Author

This patch is likely not fully correct yet and the whole story might deserve a bug or i misunderstand something.

Why do i propose this patch:
I recently found that /var/spool/mail on two of my machines has permission and ownership bits not matching what "acct-user/mail" actually wants to set. I discovered that because local mail could not be delivered or read by a user who is in the "mail" group. The directory must be from old days before acct-user and acct-group.

When an existing directory is replaced its wanted owernship and permissions are simply kept, which might be wrong like in my case. And this can likely be called a bug. Or maybe we at least need some warning here, in case it is intentional.

repro yourself:

  1. manually chown i.e. /var/spool/mail and maybe also chmod
  2. merge "acct-user/mail"
  3. inspect directory ownership and perms and compare to the values requested in the ebuild

So these ebuilds could in fact never update perms and owners if they needed to, i tried that as well by modifying one. And i also tried "acct-user/cron". In fact in the case of "acct-user/cron" and "acct-user/mail" we have other packaging dropper their keep files, but these spool dirs will fill up anyhow i.e. with emails. So the dirs will never be deleted and never be updated.

expected result:

  • pre-exisiting directories should have perms and owners as specified in ebuild (very likely not limited to acct-user/*)
  • if that is intended a warning should be printed to allow users to update manually if they see fit

@henning-schild
Copy link
Author

Please let me know how to proceed, probably open a bug in some category and provide more detail. I would be happy to write the fixing patch myself, should we come to the conclusion that we have a bug that needs fixing.

mydstat.st_uid != mystat.st_uid
or mydstat.st_gid != mystat.st_gid
):
os.chown(mydest, mystat[4], mystat[5])
Copy link
Author

Choose a reason for hiding this comment

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

this and the later chown can probably be done unconditionally, maybe even for symlinks

the code is already pretty wild and hard to read

but for now i opted for a conservative way where addition warning prints could be added

Copy link
Author

Choose a reason for hiding this comment

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

should be rebased on top of #1382 and not use magic numbers

@henning-schild
Copy link
Author

The way i found this was because a user receiving mails could not write to the spool, so perms for that case where too restrictive. I can imagine that in other scenarios where perms have been too loose it would in fact be hard (postinst) to adjust them later, and whatever one would specify in an ebuild would not have an effect on updates or reinstalls over keepdirs, just for fresh installs.

@henning-schild
Copy link
Author

It seems there is a whole series of bugs around that topic already, including a suggestion to add a FEATURE.

From this bug https://bugs.gentoo.org/654138 one can find related bugs and duplicates. But i could not really parse why nothing was done so far. Especially i did not really find what the benefit of the current implementation is, it seems to be intentional but really weird and confusing. Looking through patches in i.e. acct-user/* i see several commits that likely did not work as intended for users who had the packages installed before.

@zmedico

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.

1 participant