Re: [pacman-dev] [PATCH] can_remove_file: do not follow symlinks

On 03/08/16 08:43, Joseph Burt wrote:

The symlink itself should be checked, not its target.

Signed-off-by: Joseph Burt ---

This check also does more harm than good at the moment. If it fails once, the rest of the package files are silently left on the filesystem. Without this check, at least only the ones that failed to unlink would be left, and the failures logged.

What if the check for removal permissions happened at the very beginning of a package removal, and aborted it on failure? Would I step on any toes there?

A check at the beginning of a package removal would be of interest.

lib/libalpm/remove.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 45f7c2f..48945ad 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -29,6 +29,7 @@ #include #include #include +#include #include #include

@@ -337,8 +338,8 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file)

/* If we fail write permissions due to a read-only filesystem, abort. * Assume all other possible failures are covered somewhere else */ - if(_alpm_access(handle, NULL, filepath, W_OK) == -1) { - if(errno != EACCES && errno != ETXTBSY && access(filepath, F_OK) == 0) { + if(faccessat(AT_FDCWD, filepath, W_OK, AT_SYMLINK_NOFOLLOW) == -1) { + if(errno != EACCES && errno != ETXTBSY && faccessat(AT_FDCWD, filepath, F_OK, AT_SYMLINK_NOFOLLOW) == 0) {

It would be better to fix _alpm_access and fix all occurrences of this rather than replace this one instance. Note AT_SYMLINK_NOFOLLOW is not available on OSX. I believe pacman still builds there currently, but I can't remember if we decided not to support it...

/* only return failure if the file ACTUALLY exists and we can't write to * it - ignore "chmod -w" simple permission failures */ _alpm_log(handle, ALPM_LOG_ERROR, _("cannot remove file '%s': %s\n"),