Ingo Schwarze <schwa...@usta.de> writes:
Hi,
> Hi,
>
> Gregoire Jadi wrote on Fri, Mar 30, 2018 at 06:07:42PM +0200:
>
>> While working on a port of keyringer, I observed the following behavior
>> of rm(1) with the -P option: if the file does not have write permission,
>> the file is removed without being overwritten.
>>
>> This is not the same behavior as shred(1) (from sysutils/coreutils) which
>> do not remove the file if it cannot be overwritten. If the -f option is
>> used with shred(1), the permission of the file are changed to allow
>> writing if necessary.
>>
>> Is the current behavior desired? (need to update the manpage?)
>>
>> Or should `rm -P` and `rm -Pf` have the same behavior as shred. That is:
>> `rm -P` should fail without removing the file if the file cannot be
>> overwritten and `rm -Pf` should change the permission and overwrite
>> file.
>
> That makes more sense to me than the current behaviour.
>
> In addition, when running with -P and without -f and the user
> interactive confirms that the missing write protection shall be
> overriden, changing the permissions seems right to me, too.
>
> Here is a patch implementing that.
>
> Lightly tested by hand by running various combinations of rm(1)
> * with and without -P
> * with and without -f
> * on files with different permissions
>
> Feedback is welcome both on the general idea and on the specific
> implementation.
Thank you for working on this. I have tested your patch and both `rm -P`
and `rm -Pf` work as expected.
I have no opinion on your implementation but it does solve the problem
and make rm(1) more consistent with gshred(1).
Best,
> Note that the return value of rm_overwrite() is currently unused.
> So we are free to change it to reflect success or failure, such
> that deleting the file can be skipped in case of failure even
> after trying to relax the permissions. Also note that checking
> fflag in rw_overwrite is not needed (and would even be wrong)
> because the program doesn't get there in the first place unless
> fflag was given or deletion was interactively confirmed.
>
> Yours,
> Ingo
>
> Index: rm.c
> ===================================================================
> RCS file: /cvs/src/bin/rm/rm.c,v
> retrieving revision 1.42
> diff -u -p -r1.42 rm.c
> --- rm.c 27 Jun 2017 21:49:47 -0000 1.42
> +++ rm.c 31 Mar 2018 17:02:37 -0000
> @@ -103,7 +103,7 @@ main(int argc, char *argv[])
> argv += optind;
>
> if (Pflag) {
> - if (pledge("stdio rpath wpath cpath getpw", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath fattr getpw", NULL) == -1)
> err(1, "pledge");
> } else {
> if (pledge("stdio rpath cpath getpw", NULL) == -1)
> @@ -213,9 +213,9 @@ rm_tree(char **argv)
>
> case FTS_F:
> case FTS_NSOK:
> - if (Pflag)
> - rm_overwrite(p->fts_accpath, p->fts_info ==
> - FTS_NSOK ? NULL : p->fts_statp);
> + if (Pflag && !rm_overwrite(p->fts_accpath,
> + p->fts_info == FTS_NSOK ? NULL : p->fts_statp))
> + continue;
> /* FALLTHROUGH */
> default:
> if (!unlink(p->fts_accpath) ||
> @@ -264,8 +264,8 @@ rm_file(char **argv)
> else if (S_ISDIR(sb.st_mode))
> rval = rmdir(f);
> else {
> - if (Pflag)
> - rm_overwrite(f, &sb);
> + if (Pflag && !rm_overwrite(f, &sb))
> + continue;
> rval = unlink(f);
> }
> if (rval && (!fflag || errno != ENOENT)) {
> @@ -308,9 +308,12 @@ rm_overwrite(char *file, struct stat *sb
> if (sbp->st_nlink > 1) {
> warnx("%s (inode %llu): not overwritten due to multiple links",
> file, (unsigned long long)sbp->st_ino);
> - return (0);
> + return (1);
> }
> - if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1)
> + if ((fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1 &&
> + (errno != EACCES ||
> + chmod(file, sb.st_mode | S_IWUSR) == -1 ||
> + (fd = open(file, O_WRONLY|O_NONBLOCK|O_NOFOLLOW, 0)) == -1))
> goto err;
> if (fstat(fd, &sb2))
> goto err;
