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

[Allan McRae]

On 07/01/13 10:08, Dave Reisner wrote:
> 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>
> ---
> This sort of addresses some of the concerns in FS#21971 and FS#30884, but
> note that it doesn't actually deal with strictly read only filesystems.
> 
>  lib/libalpm/remove.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index b965391..548102f 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -299,6 +299,22 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file,
>  	return 1;
>  }
>  
> +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;
> +
> +	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 stbuf->st_dev != parent_stbuf.st_dev;
> +}
> +
>  /**
>   * @brief Unlink a package file, backing it up if necessary.
>   *
> @@ -352,6 +368,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;
> @@ -479,6 +498,10 @@ static int remove_package_files(alpm_handle_t *handle,
>  	filelist = alpm_pkg_get_files(oldpkg);
>  	for(i = 0; i < filelist->count; i++) {
>  		alpm_file_t *file = filelist->files + i;
> +		/* ignore directories */
> +		if(file->name[strlen(file->name) - 1] == '/') {
> +			continue;
> +		}
>  		if(!can_remove_file(handle, file, skip_remove)) {
>  			_alpm_log(handle, ALPM_LOG_DEBUG,
>  					"not removing package '%s', can't remove all files\n",
> 

I don't like this last bit...  Imagine /foo is a read-only mount-point
and I have a directory /foo/bar in my package with no files inside it -
many other packages contain /foo.  Now it gets through to attempting to
remove files and fails due to being unable to remove /foo/bar half way
through package removal.

Instead, we should check if the directory is a mount point when the
can_remove_file check fails (or add that check in can remove file so it
does not).


(I really want to rewrite the whole package removal code so that all
checks about ability to remove files are done prior to starting any
removal - this is at least a step in the right direction.)

Allan