[pacman-dev] [RFC] Correct directory permissions if the extracting package is the only owner

[Andrew Gregory]

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 at 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