On Wed, Feb 13, 2013 at 11:31:54AM +1000, Allan McRae wrote:
> From: Dave Reisner <[email protected]>
>
> 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 <[email protected]>
>
> [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 <[email protected]>
> ---
>
> 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
>
>
