[pacman-dev] [PATCH] libalpm: never attempt to remove a mountpoint

[Dave Reisner]

On Wed, Feb 13, 2013 at 11:31:54AM +1000, Allan McRae wrote:
> From: Dave Reisner <dreisner at archlinux.org>
> 
> Arch Linux typically runs into this with /sys when upgrading the
> filesystem package in build chroots, but LXC users might also run into
> this, since their /sys is shared from the host and must, for security
> reasons, be mounted RO.
> 
> I've neglected to add any tests for this because they would require root
> in order to run. Current tests all pass with this patch and I've
> confirmed the desired behavior in a VM. Incidentally, the first hunk of
> this patch (skipping can_remove_file checks for directories) resolves the
> case of API mountpoints being removed since they eventually fall into
> unlink_file and fail with "contains files". However, this patch should
> still be the Right Thing To Do™, as we can't possibly remove a directory
> that is also a mountpoint.
> 
> Signed-off-by: Dave Reisner <dreisner at archlinux.org>
> 
> [Allan] Do not skip checking if directories can be removed. Instead test
> if directories are mountpoints in can_remove_file.
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---
> 
> This is an updated version of Dave's original patch.  Instead of skipping
> all directory checks which could result in us trying to remove empty directories
> on read-only file systems, test all directories to see if they are a mountpoint
> during can_remove_file.   This is hugely inefficient...  it results in two stat
> calls per directory, but it fixes the current issue.  A better fix would require
> completely rewriting the pre-removal checks to be done globally and not on a per
> package basis.

Looks good to me. Thanks for cleaning this up.

>  lib/libalpm/remove.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index 50fc93c..64888c5 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -267,6 +267,37 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data)
>  	return 0;
>  }
>  
> +static int dir_is_mountpoint(alpm_handle_t *handle, const char *directory,
> +		const struct stat *stbuf)
> +{
> +	char parent_dir[PATH_MAX];
> +	struct stat parent_stbuf;
> +	dev_t dir_st_dev;
> +
> +	if(stbuf == NULL) {
> +		struct stat dir_stbuf;
> +		if(stat(directory, &dir_stbuf) < 0) {
> +			_alpm_log(handle, ALPM_LOG_DEBUG,
> +					"failed to stat directory %s: %s",
> +					directory, strerror(errno));
> +			return 0;
> +		}
> +		dir_st_dev = dir_stbuf.st_dev;
> +	} else {
> +		dir_st_dev = stbuf->st_dev;
> +	}
> +
> +	snprintf(parent_dir, PATH_MAX, "%s..", directory);
> +	if(stat(parent_dir, &parent_stbuf) < 0) {
> +		_alpm_log(handle, ALPM_LOG_DEBUG,
> +				"failed to stat parent of %s: %s: %s",
> +				directory, parent_dir, strerror(errno));
> +		return 0;
> +	}
> +
> +	return dir_st_dev != parent_stbuf.st_dev;
> +}
> +
>  /**
>   * @brief Check if alpm can delete a file.
>   *
> @@ -286,6 +317,12 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file,
>  		return 1;
>  	}
>  
> +	if(file->name[strlen(file->name) - 1] == '/' &&
> +			dir_is_mountpoint(handle, file->name, NULL)) {
> +		/* we do not remove mountpoints */
> +		return 1;
> +	}
> +
>  	snprintf(filepath, PATH_MAX, "%s%s", handle->root, file->name);
>  	/* If we fail write permissions due to a read-only filesystem, abort.
>  	 * Assume all other possible failures are covered somewhere else */
> @@ -427,6 +464,9 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg,
>  					fileobj->name)) {
>  			_alpm_log(handle, ALPM_LOG_DEBUG,
>  					"keeping directory %s (in new package)\n", file);
> +		} else if(dir_is_mountpoint(handle, file, &buf)) {
> +			_alpm_log(handle, ALPM_LOG_DEBUG,
> +					"keeping directory %s (mountpoint)\n", file);
>  		} else {
>  			/* one last check- does any other package own this file? */
>  			alpm_list_t *local, *local_pkgs;
> -- 
> 1.8.1.3
> 
>