Re: Should rm(1) -Pf change file permission?
Ingo Schwarze writes: Hello Ingo, > Hi Gregoire, > > Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200: >> Ingo Schwarze writes: > >>> Feedback is welcome both on the general idea and on the specific >>> implementation. > > The result of that feeback was "we don't want it, the whole > idea of changing this is pointless and dangerous", so i deleted > my patch quite some time ago. Ok. > The conversation resulted in a clarification in the manual page, > though. Thanks for the clarification. It's clear now it's a "best effort". >> Thank you for working on this. I have tested your patch >> and both `rm -P` and `rm -Pf` work as expected. > > Sorry, it won't be committed, it was definitely rejected. > Besides, it wasn't correct, there were race conditions. > Working on improving it would be a waste of time. Ok. So, this issue is closed. :) Best, > Yours, > Ingo
Re: Should rm(1) -Pf change file permission?
Hi Gregoire, Gregoire Jadi wrote on Tue, Apr 17, 2018 at 11:44:41AM +0200: > Ingo Schwarze writes: >> Feedback is welcome both on the general idea and on the specific >> implementation. The result of that feeback was "we don't want it, the whole idea of changing this is pointless and dangerous", so i deleted my patch quite some time ago. The conversation resulted in a clarification in the manual page, though. > Thank you for working on this. I have tested your patch > and both `rm -P` and `rm -Pf` work as expected. Sorry, it won't be committed, it was definitely rejected. Besides, it wasn't correct, there were race conditions. Working on improving it would be a waste of time. Yours, Ingo
Re: Should rm(1) -Pf change file permission?
Ingo Schwarze 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 - 1.42
> +++ rm.c 31 Mar 2018 17:02:37 -
> @@ -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;
Re: Should rm(1) -Pf change file permission?
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.
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.c27 Jun 2017 21:49:47 - 1.42
+++ rm.c31 Mar 2018 17:02:37 -
@@ -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;
Re: Should rm(1) -Pf change file permission?
Hi Grégoire/all, On Fri, 30 Mar 2018 18:07:42 +0200 Grégoire Jadi wrote: > ... here is a small test to demonstrate ... Same behaviour noticed and previously bugged:- http://openbsd-archive.7691.n7.nabble.com/rm-P-doesn-t-overwrite-a-user-owned-read-only-file-td266276.html Regards, -- Craig Skinner | http://linkd.in/yGqkv7
Should rm(1) -Pf change file permission?
Hello, 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. Here are excerpts from rm(1) and shred(1) manpages: rm(1) -f Attempt to remove the files without prompting for confirmation, regardless of the file's permissions. If the file does not exist, do not display a diagnostic message or modify the exit status to reflect an error. The -f option overrides any previous -i options. -P Overwrite regular files before deleting them. Files are overwritten once with a random pattern. Files with multiple links will be unlinked but not overwritten. shred(1) -f, --force change permissions to allow writing if necessary And here is a small test to demonstrate this behavior: $ echo bar > foo $ chmod -w foo $ rm -P foo override r--r--r-- daimrod/wheel for foo? y rm: foo: Permission denied $ ls -l foo ls: foo: No such file or directory $ echo bar > foo $ chmod -w foo $ rm -Pf foo rm: foo: Permission denied $ ls -l foo ls: foo: No such file or directory $ echo bar > foo $ chmod -w foo $ gshred -u foo gshred: foo: failed to open for writing: Permission denied $ ls -l foo -r--r--r-- 1 daimrod wheel 4 Mar 30 17:45 foo $ gshred -uf foo $ ls -l foo ls: foo: No such file or directory Best,
