[pacman-dev] [RFC] Correct directory permissions if the extracting package is the only owner
Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update: warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0 If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file. This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet. Signed-off-by: Allan McRae <allan@archlinux.org> --- This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed. Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade(). I'd like to push this to maint if people agree. These "false" warnings are causing too many issues. lib/libalpm/add.c | 61 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 82461ee..60db22f 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -167,6 +167,20 @@ static int extract_db_file(alpm_handle_t *handle, struct archive *archive, return perform_extraction(handle, archive, entry, filename); } +static int other_directory_owners(alpm_handle_t *handle, const char *entryname, const char *pkgname) { + alpm_list_t *packages = alpm_db_get_pkgcache(handle->db_local); + alpm_list_t *i; + + for(i = packages; i ; i = alpm_list_next(i)) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), entryname) && + strcmp(((alpm_pkg_t *)(i->data))->name, pkgname) != 0) { + return 1; + } + } + + return 0; +} + static int extract_single_file(alpm_handle_t *handle, struct archive *archive, struct archive_entry *entry, alpm_pkg_t *newpkg, alpm_pkg_t *oldpkg) { @@ -232,25 +246,42 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, /* case 6: existing dir, ignore it */ if(lsbuf.st_mode != entrymode) { - /* if filesystem perms are different than pkg perms, warn user */ mode_t mask = 07777; - _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" - "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, - entrymode & mask); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory permissions differ on %s\n" - "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, - entrymode & mask); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct permissions */ + if(chmod(filename, entrymode) != 0) { + goto warn_permissions; + } + } else { +warn_permissions: + /* if filesystem perms are different than pkg perms, warn user */ + _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" + "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, + entrymode & mask); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory permissions differ on %s\n" + "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, + entrymode & mask); + + } } if((entryuid != lsbuf.st_uid) || (entrygid != lsbuf.st_gid)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n"), filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n", filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct owners */ + if(chown(filename, entryuid, entrygid) != 0) { + goto warn_owners; + } + } else { +warn_owners: + _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n"), filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n", filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + } } _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping dir extraction of %s\n", -- 2.2.2
On Sun, 1 Feb 2015 21:09:56 +1000 Allan McRae <allan@archlinux.org> wrote:
Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update:
warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0
If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file.
This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet.
Signed-off-by: Allan McRae <allan@archlinux.org> ---
This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed.
Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade().
I'd like to push this to maint if people agree. These "false" warnings are causing too many issues.
So what can the user do if they changed it themselves and want different permissions than what's in the package?
On 02/02/15 03:30, Doug Newgard wrote:
On Sun, 1 Feb 2015 21:09:56 +1000 Allan McRae <allan@archlinux.org> wrote:
Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update:
warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0
If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file.
This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet.
Signed-off-by: Allan McRae <allan@archlinux.org> ---
This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed.
Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade().
I'd like to push this to maint if people agree. These "false" warnings are causing too many issues.
So what can the user do if they changed it themselves and want different permissions than what's in the package?
The same thing as always... Add it to NoExtract A
On 02/02/15 20:21, Allan McRae wrote:
On 02/02/15 03:30, Doug Newgard wrote:
On Sun, 1 Feb 2015 21:09:56 +1000 Allan McRae <allan@archlinux.org> wrote:
Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update:
warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0
If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file.
This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet.
Signed-off-by: Allan McRae <allan@archlinux.org> ---
This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed.
Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade().
I'd like to push this to maint if people agree. These "false" warnings are causing too many issues.
So what can the user do if they changed it themselves and want different permissions than what's in the package?
The same thing as always... Add it to NoExtract
And to show that works... NoExtract = usr/foo/ [22:49:43] debug: usr/foo/ is in NoExtract, skipping extraction of /usr/foo/
On 02/01/15 at 09:09pm, Allan McRae wrote:
Adjusting permissions of directories in their install scripts results in warnings such as the following when the package update:
warning: directory ownership differs on /var/lib/postfix/ filesystem: 73:0 package: 0:0
If the package being installed is the only owner of the directory, update the directory's permissions to those in the package file.
This also allows directory permissions for a package to be changed directly in the PKGBUILD file rather that requiring adjustment in an install scriptlet.
Signed-off-by: Allan McRae <allan@archlinux.org> ---
This does require searching through all installed packages filelist to see if any other directories own this file, which does create a slowdown in these error cases. Once the ability to use symbolic names for owners is added, this code path will run only if there is an acutal error or in the rare case that directory permissions are genuinely being changed.
Packages can currently avoid this being run by reverting their directory permission changes in pre_upgrade().
I'd like to push this to maint if people agree. These "false" warnings are causing too many issues.
lib/libalpm/add.c | 61 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 46 insertions(+), 15 deletions(-)
Constantly shuffling the owner between root and another user feels very hackish to me just to avoid false positives. Perhaps we should just remove the warning until we have a proper way for packages to set ownership. If we do go ahead and reset ownership, I think it should also respect noupgrade. Style nitpicks below.
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 82461ee..60db22f 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -167,6 +167,20 @@ static int extract_db_file(alpm_handle_t *handle, struct archive *archive, return perform_extraction(handle, archive, entry, filename); }
+static int other_directory_owners(alpm_handle_t *handle, const char *entryname, const char *pkgname) {
Wrap at 80 columns and move opening brace to its own line.
+ alpm_list_t *packages = alpm_db_get_pkgcache(handle->db_local); + alpm_list_t *i; + + for(i = packages; i ; i = alpm_list_next(i)) { + if(alpm_filelist_contains(alpm_pkg_get_files(i->data), entryname) && + strcmp(((alpm_pkg_t *)(i->data))->name, pkgname) != 0) { + return 1; + } + } + + return 0; +} + static int extract_single_file(alpm_handle_t *handle, struct archive *archive, struct archive_entry *entry, alpm_pkg_t *newpkg, alpm_pkg_t *oldpkg) { @@ -232,25 +246,42 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
/* case 6: existing dir, ignore it */ if(lsbuf.st_mode != entrymode) { - /* if filesystem perms are different than pkg perms, warn user */ mode_t mask = 07777; - _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" - "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, - entrymode & mask); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory permissions differ on %s\n" - "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, - entrymode & mask); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct permissions */ + if(chmod(filename, entrymode) != 0) { + goto warn_permissions; + } + } else { +warn_permissions:
I would prefer this was written as a single condition: if(noupgrade || other_directory_owners(...) != 0 || chmod(...) != 0) { /* warn user */ }
+ /* if filesystem perms are different than pkg perms, warn user */ + _alpm_log(handle, ALPM_LOG_WARNING, _("directory permissions differ on %s\n" + "filesystem: %o package: %o\n"), filename, lsbuf.st_mode & mask, + entrymode & mask); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory permissions differ on %s\n" + "filesystem: %o package: %o\n", filename, lsbuf.st_mode & mask, + entrymode & mask); + + } }
if((entryuid != lsbuf.st_uid) || (entrygid != lsbuf.st_gid)) { - _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n"), filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); - alpm_logaction(handle, ALPM_CALLER_PREFIX, - "warning: directory ownership differs on %s\n" - "filesystem: %u:%u package: %u:%u\n", filename, - lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + if(other_directory_owners(handle, entryname, newpkg->name) == 0) { + /* no other package owns this directory, "extract" with correct owners */ + if(chown(filename, entryuid, entrygid) != 0) { + goto warn_owners; + } + } else { +warn_owners:
Same as above.
+ _alpm_log(handle, ALPM_LOG_WARNING, _("directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n"), filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + alpm_logaction(handle, ALPM_CALLER_PREFIX, + "warning: directory ownership differs on %s\n" + "filesystem: %u:%u package: %u:%u\n", filename, + lsbuf.st_uid, lsbuf.st_gid, entryuid, entrygid); + } }
_alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping dir extraction of %s\n", -- 2.2.2
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Doug Newgard