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

[Allan McRae]

On 03/08/16 08:43, Joseph Burt wrote:
> The symlink itself should be checked, not its target.
> 
> Signed-off-by: Joseph Burt <caseorum at gmail.com>
> ---
> 
> 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 <dirent.h>
>  #include <regex.h>
>  #include <unistd.h>
> +#include <fcntl.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  
> @@ -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"),
>