GNU bug report logs -
#15926
RFE: unlink command already uses 'unlink' call; make 'rm' use 'remove' call
Previous Next
Reported by: Linda Walsh <coreutils <at> tlinx.org>
Date: Tue, 19 Nov 2013 11:58:02 UTC
Severity: normal
Tags: notabug, patch
Merged with 15943
Done: Assaf Gordon <assafgordon <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 15926 in the body.
You can then email your comments to 15926 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 11:58:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Linda Walsh <coreutils <at> tlinx.org>:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org.
(Tue, 19 Nov 2013 11:58:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
Since there is already an unlink command that corresponds to unlinking a file,
but there seems to be no command corresponding to the POSIX
remove command, it seems upgrading 'rm' to use the 'remove'
POSIX call would be a beneficial move of all the recent POSIX
changes.
So how about upgrading 'rm' to use the remove function so
it would work on empty directories as well.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 12:16:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 15926 <at> debbugs.gnu.org (full text, mbox):
tag 15926 notabug
close 15926
stop
On 11/19/2013 11:56 AM, Linda Walsh wrote:
> Since there is already an unlink command that corresponds to unlinking a file,
> but there seems to be no command corresponding to the POSIX
> remove command, it seems upgrading 'rm' to use the 'remove'
> POSIX call would be a beneficial move of all the recent POSIX
> changes.
>
> So how about upgrading 'rm' to use the remove function so
> it would work on empty directories as well.
Well we have the -d option to rm to explicitly do that.
That's a fairly fundamental change that would have backwards compat issues.
POSIX is explicit in the handling of directories also:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/rm.html
thanks,
Pádraig.
Added tag(s) notabug.
Request was from
Pádraig Brady <P <at> draigBrady.com>
to
control <at> debbugs.gnu.org.
(Tue, 19 Nov 2013 12:16:02 GMT)
Full text and
rfc822 format available.
bug closed, send any further explanations to
15926 <at> debbugs.gnu.org and Linda Walsh <coreutils <at> tlinx.org>
Request was from
Pádraig Brady <P <at> draigBrady.com>
to
control <at> debbugs.gnu.org.
(Tue, 19 Nov 2013 12:16:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 18:18:01 GMT)
Full text and
rfc822 format available.
Message #15 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 19/11/2013 04:15, Pádraig Brady wrote:
> tag 15926 notabug
> close 15926
> stop
>
> On 11/19/2013 11:56 AM, Linda Walsh wrote:
> > Since there is already an unlink command that corresponds to unlinking a file,
> > but there seems to be no command corresponding to the POSIX
> > remove command, it seems upgrading 'rm' to use the 'remove'
> > POSIX call would be a beneficial move of all the recent POSIX
> > changes.
> >
> > So how about upgrading 'rm' to use the remove function so
> > it would work on empty directories as well.
>
> Well we have the -d option to rm to explicitly do that.
---
Does the posix "remove" call require a -d?
I thought it made more sense to have rm correspond to remove
than create another command called remove that called remove.
Are you saying that you think it would be better to have a remove command
that does this?
As for the posix document you pointed at, I'm suggesting doing step 4
after step 1... same thing posix does, just reordering things a bit.
This came up, BTW, because a useless error message was added to find
such that if you used find dir/. -type d -empty -delete, it now exits with
an error status telling you nothing. other than it didn't delete the "."
entry from inside the dir (which most people know can't be done anyway).
But now there is no way to determine if the find command failed, or if it
is just making noise over things it never has done.
Alternatively, I suggested adding a -f flag that would occur, positionally
before any path arguments (like the -H/-L/-P opts now), that would silence
such fails and return a 0 status for delete failures that the OS cannot
do.
No useful purpose is served by the new error message, and creating it breaks
compatibility with scripts.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 18:35:02 GMT)
Full text and
rfc822 format available.
Message #18 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/19/2013 11:17 AM, Linda Walsh wrote:
>> Well we have the -d option to rm to explicitly do that.
> ---
> Does the posix "remove" call require a -d?
Huh? There is no POSIX remove(1), only remove(3), unlink(2), rmdir(2),
rm(1), rmdir(1), and unlink(1) (using the traditional notation of which
man page section the interface is described in). GNU's 'rm -d' is an
extension not required by POSIX, but consistent with BSD implementations.
>
> I thought it made more sense to have rm correspond to remove
> than create another command called remove that called remove.
Sorry, but to change that, you'd have to go back in time 30 or 40 years
to when rm(1) was first written. People have grown to rely on 'rm(1)'
being a wrapper around unlink(2), 'rmdir(1)' being a wrapper around
rumdir(2), and 'rm(1) -R' being a wrapper around rmdir(2) or unlink(2)
as needed.
>
> Are you saying that you think it would be better to have a remove command
> that does this?
No, because remove(3) is just a thin wrapper around unlink(2) or
rmdir(2), and most users are already familiar with how to remove files
according to their type. I see no reason to add a remove(1).
>
> As for the posix document you pointed at, I'm suggesting doing step 4
> after step 1... same thing posix does, just reordering things a bit.
Sorry, but doing things in rm(1) in a different order than POSIX
describes would lead to subtle breakage to lots of existing scripts.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 19:53:01 GMT)
Full text and
rfc822 format available.
Message #21 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 19/11/2013 10:34, Eric Blake wrote:
> On 11/19/2013 11:17 AM, Linda Walsh wrote:
>
> >> Well we have the -d option to rm to explicitly do that.
> > ---
> > Does the posix "remove" call require a -d?
>
> Huh? There is no POSIX remove(1),
----
Since when do you think of a "call" as being a command?
> Sorry, but to change that, you'd have to go back in time 30 or 40 years
> to when rm(1) was first written. People have grown to rely on 'rm(1)'
> being a wrapper around unlink(2), 'rmdir(1)' being a wrapper around
> rumdir(2), and 'rm(1) -R' being a wrapper around rmdir(2) or unlink(2)
> as needed.
----
People relied on rm removing files in a depth first search order for 30-40
years.
Posix changed that requiring special checks for ".". Scripts relied on that
behavior
for 30-40 years as well... If you want to use that reasoning, rm should go back
to doing depth first deletion and reporting an error with deleting "." when it
is finished.
>
> Sorry, but doing things in rm(1) in a different order than POSIX
> describes would lead to subtle breakage to lots of existing scripts.
----
I claim not. Come up with 1 case where scripts rely on the current
behavior -- to "die" before doing anything useful, vs. the pre-existing
behavior which was to issue an error (suppressible with "-f") on the
final deletion failing.
I am calling your bluff -- show me the scripts (other than a posix
conformance test script), that would fail -- subtly or otherwise.
I assert they don't exist for 2 reasons. The foremost being
that working scripts cannot rely on upon the deletion failure
stopping any useful work being done by the command. The 2nd
being it was a new change in posix that appeared in gnu utils
in only the past few years. The previous 25-35 years of scripts
would have relied on it working as *documented* (depth first).
Checking pathnames before you start depth first traversal is not
strictly depth first.
By your own standards for not changing something, "rm" should
be fixed to be the way it was for 30-40 years, as.
The problem is, is that by implementing that change, functionality
was lost and removed in "rm". The earlier version had more
functionality. So you can't come up with scripts that rely on
missing functionality to get things done. It's like relying on
missing water to water your plants or missing food to feed yourself.
You can't rely on the absence of a feature to do something positive
with it.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 19 Nov 2013 20:46:01 GMT)
Full text and
rfc822 format available.
Message #24 received at 15926 <at> debbugs.gnu.org (full text, mbox):
Linda Walsh wrote:
> but there seems to be no command corresponding to the POSIX
> remove command,
The language here is confusing. There isn't any "remove command".
There is only the remove(3) library call.
man 3 remove
> it seems upgrading 'rm' to use the 'remove' POSIX call would be a
> beneficial move of all the recent POSIX changes.
That would introduce a significant behavior change and with certainty
would introduce bugs. Therefore it shouldn't be done.
> Eric Blake wrote:
> > Linda Walsh wrote:
> > > Pádraig Brady wrote:
> > > > Linda Walsh wrote:
> > > > > So how about upgrading 'rm' to use the remove function so
> > > > > it would work on empty directories as well.
> > > >
> > > > Well we have the -d option to rm to explicitly do that.
> > >
> > > Does the posix "remove" call require a -d?
Right at this point there is confusion. The only POSIX remove() is
the libc library call remove(3).
> > Huh? There is no POSIX remove(1),
Yes. Huh?
> Since when do you think of a "call" as being a command?
We don't. But from what you wrote ("Does the posix "remove" call
require a -d?") makes it appear that you think the posix remove(3)
library call is a command. Because library calls do not take options
like that while commands do.
> > Sorry, but to change that, you'd have to go back in time 30 or 40 years
> > to when rm(1) was first written. People have grown to rely on 'rm(1)'
> > being a wrapper around unlink(2), 'rmdir(1)' being a wrapper around
> > rumdir(2), and 'rm(1) -R' being a wrapper around rmdir(2) or unlink(2)
> > as needed.
Agreed. Let's not break this.
> People relied on rm removing files in a depth first search order
> for 30-40 years.
When using 'rm -rf' they do. But only with the -r option. Never
without it.
The change you have proposed would for the first time in all of those
years have rm remove a directory without the -r option. That would be
bad because it would introduce bugs into scripts that are expecting
the existing behavior of rm not removing directories. (Unless the -r
option is given.)
> Posix changed that requiring special checks for ".". Scripts relied
> on that behavior for 30-40 years as well... If you want to use that
> reasoning, rm should go back to doing depth first deletion and
> reporting an error with deleting "." when it is finished.
I actually agree with you on that point. ;-)
(Regarding 'rm -rf .')
> > Sorry, but doing things in rm(1) in a different order than POSIX
> > describes would lead to subtle breakage to lots of existing scripts.
> ----
> I claim not. Come up with 1 case where scripts rely on the current
> behavior -- to "die" before doing anything useful, vs. the pre-existing
> behavior which was to issue an error (suppressible with "-f") on the
> final deletion failing.
I have seen many various script writing styles. Some of them are
atrocious indeed. I would expect to at some point see someone use rm
on file system objects with abandon thinking that they won't remove
directories. Suddenly they will be able to remove directories. That
would be a change in behavior. Changing established behavior is bad.
I routinely use the opposite side of things myself. I routinely use
rmdir on empty directories knowing that if the directory is not empty
that it will fail and not remove any actual file content. I could
definitely see someone counting on the behavior that rm removes files
but never removes directories. (Unless the -r option is given.)
> I am calling your bluff -- show me the scripts (other than a posix
> conformance test script), that would fail -- subtly or otherwise.
Anything I show would be contrived. But that doesn't mean that
changing the behavior should be done.
mkdir a b c
touch aa bb cc
for f in *; do rm "$f"; done
With existing behavior the directories would remain. With the new
proposed behavior the directories would be removed. That is a
behavior change that should not be made.
If you want that behavior then the -d option is present and
available. Simply use the -d option.
mkdir a b c
touch aa bb cc
for f in *; do rm -d "$f"; done
That gives you the behavior you are requesting.
> I assert they don't exist for 2 reasons. The foremost being
> that working scripts cannot rely on upon the deletion failure
> stopping any useful work being done by the command.
See the above example.
> The 2nd being it was a new change in posix that appeared in gnu
> utils in only the past few years. The previous 25-35 years of
> scripts would have relied on it working as *documented* (depth
> first).
Since you are mentioning depth first I can only think you are talking
about 'rm -r'. How does that tie into remove(3)?
> Checking pathnames before you start depth first traversal is not
> strictly depth first.
Again, what does depth first traversal have to do with remove(3)?
> By your own standards for not changing something, "rm" should
> be fixed to be the way it was for 30-40 years, as.
Huh? Stop here. Please explain. Your words here do not follow.
> The problem is, is that by implementing that change, functionality
> was lost and removed in "rm". The earlier version had more
> functionality. So you can't come up with scripts that rely on
> missing functionality to get things done. It's like relying on
> missing water to water your plants or missing food to feed yourself.
Are you talking about 'rm -rf .'? I agree with you on that one. That
was a bad change to disallow it. It just wasn't as severe as what you
are proposing. Disallowing 'rm -rf .' is a nanny-state type of
behavior. It annoys me. It shouldn't have been done. But changing
unlink(2) to remove(3) is much more severe and really, really
shouldn't be done.
Bob
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 20 Nov 2013 00:03:02 GMT)
Full text and
rfc822 format available.
Message #27 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 19/11/2013 12:45, Bob Proulx wrote:
>
>> Since when do you think of a "call" as being a command?
>
> We don't. But from what you wrote ("Does the posix "remove" call
> require a -d?") makes it appear that you think the posix remove(3)
> library call is a command. Because library calls do not take options
> like that while commands do.
----
Well, that was a bit of metaphore, insomuch as the POSIX
calls talk about flag passing with a value of AT_REMOVEDIR to do something
different... -d in a call would likely be passed as an option
>
>>> Sorry, but to change that, you'd have to go back in time 30 or 40 years
>>> to when rm(1) was first written. People have grown to rely on 'rm(1)'
>>> being a wrapper around unlink(2), 'rmdir(1)' being a wrapper around
>>> rumdir(2), and 'rm(1) -R' being a wrapper around rmdir(2) or unlink(2)
>>> as needed.
>
> Agreed. Let's not break this.
----
I'm not see what would break. instead of failing to remove a non-empty
directory, it would remove it (if it was empty and permissions allowed).
>
>> People relied on rm removing files in a depth first search order
>> for 30-40 years.
>
> When using 'rm -rf' they do. But only with the -r option. Never
> without it.
---
Absolutely... that's what I was referring to, sorry for unclarity.
>
> The change you have proposed would for the first time in all of those
> years have rm remove a directory without the -r option. That would be
> bad because it would introduce bugs into scripts that are expecting
> the existing behavior of rm not removing directories. (Unless the -r
> option is given.)
----
I would ask how it would fail in a script -- I.e. we are only talking
empty directories -- and currently an error would be returned if
a script tried to do that. If the script was catching errors or running
with "-e", the script would terminate under the current implementation.
So no script could ever "work" relying on itself to be fail -- it's a
contradiction in terms.
>
>> Posix changed that requiring special checks for ".". Scripts relied
>> on that behavior for 30-40 years as well... If you want to use that
>> reasoning, rm should go back to doing depth first deletion and
>> reporting an error with deleting "." when it is finished.
>
> I actually agree with you on that point. ;-)
> (Regarding 'rm -rf .')
----
Well, that's what much of my interest in this area
and the newly created bug in "find" hinges on.
>
>>> Sorry, but doing things in rm(1) in a different order than POSIX
>>> describes would lead to subtle breakage to lots of existing scripts.
>> ----
>> I claim not. Come up with 1 case where scripts rely on the current
>> behavior -- to "die" before doing anything useful, vs. the pre-existing
>> behavior which was to issue an error (suppressible with "-f") on the
>> final deletion failing.
>
> I have seen many various script writing styles. Some of them are
> atrocious indeed. I would expect to at some point see someone use rm
> on file system objects with abandon thinking that they won't remove
> directories. Suddenly they will be able to remove directories. That
> would be a change in behavior. Changing established behavior is bad.
-----
*empty directories*.. lets be clear. I am NOT proposing that
"rm" recursively remove anything without "-r"...
>
> I routinely use the opposite side of things myself. I routinely use
> rmdir on empty directories knowing that if the directory is not empty
> that it will fail and not remove any actual file content. I could
> definitely see someone counting on the behavior that rm removes files
> but never removes directories. (Unless the -r option is given.)
----
I use the fact that rm doesn't remove non-empty directories
as well.. I'm not questioning that behavior.
If someone relied on using "rm" to clean out a directory
of any "fluff" or "dander" that had built up... I strongly believe
that if "rm" also removed *empty* directories, it would be more likely
to be appreciated by soemone who was using it to get rid of all the
spurious file entries -- as empty directory entries are often just as
spurious -- I'm often cleaning out skeletal trees of directories on my
systems... I've even thought about the usefulness of "rmdir -r" --
which would recursively remove empty dirs starting at the top (with
a limitation for "-xdev"... (I know about rmdir -p a/b/c).. but empty, wider
skeletons are more common than skinny+tall dir-structures. (skinny=1 dir wide,
many dirs deep).
>
>> I am calling your bluff -- show me the scripts (other than a posix
>> conformance test script), that would fail -- subtly or otherwise.
>
> Anything I show would be contrived. But that doesn't mean that
> changing the behavior should be done.
>
> mkdir a b c
> touch aa bb cc
> for f in *; do rm "$f"; done
>
> With existing behavior the directories would remain. With the new
> proposed behavior the directories would be removed. That is a
> behavior change that should not be made.
----
Why not? if you want 1 command that does both, why not
implement a remove command == but wouldn't it commonly be
abbreviated as 'rm'...?
It would be a change. But it's not something that
would cause the level of problems associated with disabling "rm -fr ." --
and that crippling change was made w/o a blink of an eyelash.
(then Erik gets a bug up his **, about thinking
how "useful" it is to have "find . -type d -empty -delete" always
exist with a failure code. Just lovely.) ;-/
>
> If you want that behavior then the -d option is present and
> available. Simply use the -d option.
>
> mkdir a b c
> touch aa bb cc
> for f in *; do rm -d "$f"; done
---
I could alias that, I s'pose.. and see
if any scripts fail because of it... ;-)
>
> That gives you the behavior you are requesting.
>
>> I assert they don't exist for 2 reasons. The foremost being
>> that working scripts cannot rely on upon the deletion failure
>> stopping any useful work being done by the command.
>
> See the above example.
-----
a demo script isn't a working script, nor is it a
script that does any useful work. I specifically excluded
posix compliance scripts -- which the above would be be more
likely to be than a "working script"...
>> The 2nd being it was a new change in posix that appeared in gnu
>> utils in only the past few years. The previous 25-35 years of
>> scripts would have relied on it working as *documented* (depth
>> first).
>
> Since you are mentioning depth first I can only think you are talking
> about 'rm -r'. How does that tie into remove(3)?
----
Only insomuch as empty dir removal is less of a feature change
than aborting the entire command.
>
>> By your own standards for not changing something, "rm" should
>> be fixed to be the way it was for 30-40 years, as.
>
> Huh? Stop here. Please explain. Your words here do not follow.
---
Eric said that because rm was the way it was (failing on
dir deletes) for 30-40 years, was reasoning not to change it, when
the only consequence was ability to delete empty dirs, then
undoing the posix damage of removing the entire functionality of
being able to have rm create a pristine dir w/1 command (as well
as not crossing fs-boundaries). Even using "rm *" in the directory
if something in dir is a mount.. you end up deleting things on other
file systems (I got the T-shirt; ;~>).
>
>> The problem is, is that by implementing that change, functionality
>> was lost and removed in "rm". The earlier version had more
>> functionality. So you can't come up with scripts that rely on
>> missing functionality to get things done. It's like relying on
>> missing water to water your plants or missing food to feed yourself.
>
> Are you talking about 'rm -rf .'? I agree with you on that one. That
> was a bad change to disallow it. It just wasn't as severe as what you
> are proposing. Disallowing 'rm -rf .' is a nanny-state type of
> behavior. It annoys me. It shouldn't have been done. But changing
> unlink(2) to remove(3) is much more severe and really, really
> shouldn't be done.
----
I see the consequences of "rm -fr ." being worse, as the
work-arounds have more mine-fields -- like deleting things
on other file systems. As opposed to the unlink->remove change
which would delete *empty directories*... I don't see that
as nearly as harmful as deleting entire file-systems on
mounted dirs...
But to throw salt in the wound, Erik changed find so that
it complains if you try to delete "." as in
"find . -type d -empty -delete" -- my snapshot script used
that as well as "rm -fr ." to clean out directories to
be prepared to do "diffs" into. The find-clean empty dirs
command is more of a way to save about 1-2% on space, as
rsync likes to copy dirs over if the times are different
(even when it is told not to copy empty dirs)...
Anyway, that was the latest bug-up-my bonnet that caused
me to think about these issues again.. trying to come
up with anything useful.. like rm removing empty
dirs seemed like it would be a minimal damage thing compared
to other damage.
As for rm-fr. = nanny ... yes.. got the impression
it was implemented on BSD systems to protect the
college kids -- but that wouldn't prevent allowing
it with an ENV flag that lists commands to exclude
from nanniness... some
ENV flag EXPERT_MODE="rm(.),find(delete)"
commands as needed"
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 20 Nov 2013 01:30:02 GMT)
Full text and
rfc822 format available.
Message #30 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/19/2013 05:02 PM, Linda Walsh wrote:
> I'm not see what would break. instead of failing to remove a non-empty
> directory, it would remove it (if it was empty and permissions allowed).
That's what would break. Scripts have been written to assume that 'rm
empty_dir' will fail, and your proposal would change that assumption.
Falling through to subsequent commands when you used to bail out early
has all sorts of subtle implications, that we aren't willing to risk.
> I would ask how it would fail in a script -- I.e. we are only talking
> empty directories -- and currently an error would be returned if
> a script tried to do that. If the script was catching errors or running
> with "-e", the script would terminate under the current implementation.
'set -e' is useless. I highly recommend that script writers avoid it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 20 Nov 2013 02:20:03 GMT)
Full text and
rfc822 format available.
Message #33 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/19/2013 01:45 PM, Bob Proulx wrote:
>> Posix changed that requiring special checks for ".". Scripts relied
>> on that behavior for 30-40 years as well... If you want to use that
>> reasoning, rm should go back to doing depth first deletion and
>> reporting an error with deleting "." when it is finished.
>
I see nothing in
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/rm.html that
requires up-front special handling of trailing '.', only a callout to
what rmdir() is required to do. Ultimately, rmdir("dir/.") is required
to fail, but nothing in POSIX appears to allow us to short-circuit the
recursive cleanup before attempting the failing command.
> I actually agree with you on that point. ;-)
> (Regarding 'rm -rf .')
Yes, the 'rm -rf .' case appears to be a regression in coreutils that is
contrary to the behavior required by POSIX. That is:
$ mkdir /tmp/foo /tmp/foo/sub
$ cd /tmp/foo
$ rm -r .
rm: cannot remove directory: ‘.’
$ ls
sub
appears to be a bug in current coreutils, because it should have
successfully called rmdir("sub") prior to failing on the attempt to
rmdir(".").
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 20 Nov 2013 06:47:02 GMT)
Full text and
rfc822 format available.
Message #36 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/20/2013 03:19 AM, Eric Blake wrote:
> Yes, the 'rm -rf .' case appears to be a regression in coreutils that is
> contrary to the behavior required by POSIX. That is:
>
> $ mkdir /tmp/foo /tmp/foo/sub
> $ cd /tmp/foo
> $ rm -r .
> rm: cannot remove directory: ‘.’
> $ ls
> sub
>
> appears to be a bug in current coreutils, because it should have
> successfully called rmdir("sub") prior to failing on the attempt to
> rmdir(".").
I disagree: see at the top of:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/rm.html
If either of the files dot or dot-dot are specified as the basename
portion of an operand (that is, the final pathname component) or if
an operand resolves to the root directory, rm shall write a diagnostic
message to standard error and do nothing more with such operands.
Maybe "cannot remove directory" is a bit weak - it's more like
"refusing to remove dot|dot-dot|root directory".
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 20 Nov 2013 13:45:02 GMT)
Full text and
rfc822 format available.
Message #39 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/19/2013 11:45 PM, Bernhard Voelker wrote:
> On 11/20/2013 03:19 AM, Eric Blake wrote:
>> Yes, the 'rm -rf .' case appears to be a regression in coreutils that is
>> contrary to the behavior required by POSIX. That is:
>>
>> $ mkdir /tmp/foo /tmp/foo/sub
>> $ cd /tmp/foo
>> $ rm -r .
>> rm: cannot remove directory: ‘.’
>> $ ls
>> sub
>>
>> appears to be a bug in current coreutils, because it should have
>> successfully called rmdir("sub") prior to failing on the attempt to
>> rmdir(".").
>
> I disagree: see at the top of:
>
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/rm.html
>
> If either of the files dot or dot-dot are specified as the basename
> portion of an operand (that is, the final pathname component) or if
> an operand resolves to the root directory, rm shall write a diagnostic
> message to standard error and do nothing more with such operands.
Ah, that's where it is. I was looking in the numbered steps, not the
paragraph before the numbered steps. So it appears that coreutils is
compliant after all.
>
> Maybe "cannot remove directory" is a bit weak - it's more like
> "refusing to remove dot|dot-dot|root directory".
Indeed, a clearer error message would be possible.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 00:04:02 GMT)
Full text and
rfc822 format available.
Message #42 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/20/2013 02:44 PM, Eric Blake wrote:
> On 11/19/2013 11:45 PM, Bernhard Voelker wrote:
>> Maybe "cannot remove directory" is a bit weak - it's more like
>> "refusing to remove dot|dot-dot|root directory".
>
> Indeed, a clearer error message would be possible.
What about the following?
$ src/rm -r src/.
src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
I didn't want to explicitly mention POSIX here ... it's just to clarify
that rm(1) does not swallow errors from the kernel like EPERM, etc.
The texinfo file is enhanced in the patch below, too.
Have a nice day,
Berny
From f85ec85852ea62e4fe4510044ffb4b31880a6b7c Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail <at> bernhard-voelker.de>
Date: Thu, 21 Nov 2013 00:47:36 +0100
Subject: [PATCH] doc: enhance diagnostic when rm skips "." or ".." arguments
The error diagnostic
"rm: cannot remove directory: '.'"
does not give the user a hint for the reason.
Issue a clearer error message.
* src/remove.c (rm_fts): Enhance the error diagnostic in the above
case to emphasize that skipping is done deliberately.
In the corresponding comment, mention that POSIX mandates this
behavior. Likewise in the subsequent comment for skipping "/".
* doc/coreutils.texi (rm invocation): In the paragraph describing
the above behavior, mention that POSIX mandates it.
---
doc/coreutils.texi | 3 ++-
src/remove.c | 12 ++++++++----
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 035f2e6..64713dc 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -9281,7 +9281,8 @@ the @option{-f} or @option{--force} option is not given, or the
If the response is not affirmative, the file is skipped.
Any attempt to remove a file whose last file name component is
-@file{.} or @file{..} is rejected without any prompting.
+@file{.} or @file{..} is rejected without any prompting, as mandated
+by POSIX.
@emph{Warning}: If you use @command{rm} to remove a file, it is usually
possible to recover the contents of that file. If you want more assurance
diff --git a/src/remove.c b/src/remove.c
index cdbbec5..3d386cf 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -437,17 +437,21 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
/* Perform checks that can apply only for command-line arguments. */
if (ent->fts_level == FTS_ROOTLEVEL)
{
- /* If the basename of a command line argument is "." or "..",
+ /* POSIX says:
+ If the basename of a command line argument is "." or "..",
diagnose it and do nothing more with that argument. */
if (dot_or_dotdot (last_component (ent->fts_accpath)))
{
- error (0, 0, _("cannot remove directory: %s"),
- quote (ent->fts_path));
+ error (0, 0,
+ _("refusing to remove %s or %s directory: skipping %s"),
+ quote_n (0, "."), quote_n (1, ".."),
+ quote_n (2, ent->fts_path));
fts_skip_tree (fts, ent);
return RM_ERROR;
}
- /* If a command line argument resolves to "/" (and --preserve-root
+ /* POSIX also says:
+ If a command line argument resolves to "/" (and --preserve-root
is in effect -- default) diagnose and skip it. */
if (ROOT_DEV_INO_CHECK (x->root_dev_ino, ent->fts_statp))
{
--
1.8.3.1
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 00:49:02 GMT)
Full text and
rfc822 format available.
Message #45 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 20/11/2013 16:03, Bernhard Voelker wrote:
> $ src/rm -r src/.
> src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
That gets back to what Bob mentioned about it being a
nanny-restriction.
The inevitable comment to be asked by someone is
"Refuse?
Isn't it my computer? How do I override such a refusal?
I seem to remember reading that the "-f" flag was specifically
added to override such such a refusal w/no further comment."
Answer: well, yeah it was, but they caught MS-itus, and wanted
to put in "are you really sure? (y/[n])", but weren't allowed
to ask more questions, so it just wins because its not your system
anymore.
**- Is it true that you can override this with -supercalifragilisticexpialidocious
flag? 1/2:-)
I still think an ENV flag that lists the command
and behavior to override would be a specific enough, yet
generally enough solution to safely make a case for
allowing it.
I.e. _EXPERT_="rm(.) command(feature1 feature2) find(.error)"
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 02:08:02 GMT)
Full text and
rfc822 format available.
Message #48 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/20/2013 05:03 PM, Bernhard Voelker wrote:
> On 11/20/2013 02:44 PM, Eric Blake wrote:
>> On 11/19/2013 11:45 PM, Bernhard Voelker wrote:
>>> Maybe "cannot remove directory" is a bit weak - it's more like
>>> "refusing to remove dot|dot-dot|root directory".
>>
>> Indeed, a clearer error message would be possible.
>
> What about the following?
>
> $ src/rm -r src/.
> src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
That helps. But I'm also starting to think we should add a new long
option --no-preserve-dot, similar to how --no-preserve-root can be used
to work around the restriction. Then people that want to can create an
alias or other wrapper around rm to get the non-nanny behavior, while
the default behavior still complies with POSIX.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 02:09:01 GMT)
Full text and
rfc822 format available.
Message #51 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/20/2013 05:48 PM, Linda Walsh wrote:
> I still think an ENV flag that lists the command
Environment variables that modify behavior are nasty, and should
generally be avoided when simpler solutions exist. In this case, I'd
much rather add a long option, 'rm --no-preserve-dot', and let you write
an alias or other wrapper when you want the non-default behavior.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Forcibly Merged 15926 15943.
Request was from
Eric Blake <eblake <at> redhat.com>
to
control <at> debbugs.gnu.org.
(Thu, 21 Nov 2013 02:17:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 06:33:02 GMT)
Full text and
rfc822 format available.
Message #56 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 01:48 AM, Linda Walsh wrote:
> Isn't it my computer? How do I override such a refusal?
That riddle isn't too hard, is it? ;-)
POSIX (and common sense) forbids to remove something ending on ".".
Therefore just use the canonicalized name, e.g.:
$ mkdir /tmp/xx
$ cd /tmp/xx
$ rm -rv .
rm: refusing to remove ‘.’ or ‘..’ directory: skipping ‘.’
$ rm -rv "$(pwd -P)"
removed directory: ‘/tmp/xx’
or alternatively (just the rm invocation):
$ rm -rv "$(readlink -f .)"
removed directory: ‘/tmp/xx’
As you were mentioning MS: the above obviously won't probably work
in Cygwin as the underlying (file) system may return EBUSY.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 07:13:02 GMT)
Full text and
rfc822 format available.
Message #59 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 03:07 AM, Eric Blake wrote:
> On 11/20/2013 05:03 PM, Bernhard Voelker wrote:
>> What about the following?
>>
>> $ src/rm -r src/.
>> src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
>
> That helps.
Thanks, I'll push it unless someone comes up with a better wording.
> But I'm also starting to think we should add a new long
> option --no-preserve-dot, similar to how --no-preserve-root can be used
> to work around the restriction. Then people that want to can create an
> alias or other wrapper around rm to get the non-nanny behavior, while
> the default behavior still complies with POSIX.
Admittedly, compared to the academic question behind "--no-preserve-root"
(which is like "what happens to me when the globe under my feet disappears?"),
there may be more real-world reasons to remove ".".
However, as it's possible to pass the canonicalized file name of "."
or ".." to rm(1), I'm not yet convinced that it warrants adding a
new --no-preserve-dot-or-dotdot (and for symmetry reasons a new
--preserve-dot-or-dotdot) option.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 08:55:02 GMT)
Full text and
rfc822 format available.
Message #62 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 12:03 AM, Bernhard Voelker wrote:
> if (dot_or_dotdot (last_component (ent->fts_accpath)))
> {
> - error (0, 0, _("cannot remove directory: %s"),
> - quote (ent->fts_path));
> + error (0, 0,
> + _("refusing to remove %s or %s directory: skipping %s"),
> + quote_n (0, "."), quote_n (1, ".."),
> + quote_n (2, ent->fts_path));
That's a good clarification. Please push.
thanks!
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 08:57:01 GMT)
Full text and
rfc822 format available.
Message #65 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 07:12 AM, Bernhard Voelker wrote:
> On 11/21/2013 03:07 AM, Eric Blake wrote:
>> On 11/20/2013 05:03 PM, Bernhard Voelker wrote:
>>> What about the following?
>>>
>>> $ src/rm -r src/.
>>> src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
>>
>> That helps.
>
> Thanks, I'll push it unless someone comes up with a better wording.
>
>> But I'm also starting to think we should add a new long
>> option --no-preserve-dot, similar to how --no-preserve-root can be used
>> to work around the restriction. Then people that want to can create an
>> alias or other wrapper around rm to get the non-nanny behavior, while
>> the default behavior still complies with POSIX.
>
> Admittedly, compared to the academic question behind "--no-preserve-root"
> (which is like "what happens to me when the globe under my feet disappears?"),
> there may be more real-world reasons to remove ".".
>
> However, as it's possible to pass the canonicalized file name of "."
> or ".." to rm(1), I'm not yet convinced that it warrants adding a
> new --no-preserve-dot-or-dotdot (and for symmetry reasons a new
> --preserve-dot-or-dotdot) option.
I would say this does not need an option for the reason you describe.
thanks,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 10:45:02 GMT)
Full text and
rfc822 format available.
Message #68 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 20/11/2013 22:32, Bernhard Voelker wrote:
> On 11/21/2013 01:48 AM, Linda Walsh wrote:
>> Isn't it my computer? How do I override such a refusal?
> $ rm -rv "$(pwd -P)"
> removed directory: ‘/tmp/xx’
--
That doesn't give the same behavior and isn't what I want.
Compare to "cp".
Say I want to create a copy of what is in dir "a"
inside of a pre-existing dir "b". In dir "a" are files and
sub dirs. On some of those subdirs, other file systems *may*
be mounted -- EITHER in the dir immediately under "a", OR lower:
I would use "cp -alx a/. b/."
Sometime later, I want to remove the contents of 'b' w/o disturbing
'b'. 'b' may have file systems mounted under it or not.
Again, I would use the "dot" notation. "rm -fxr b/."
"rm -fxr <path>/b", as you suggest isn't the same thing.
Directories are containers. I want to work just with the
contents -- not the directory itself.
So how do I override the refusal -- and get the same results?
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 11:02:02 GMT)
Full text and
rfc822 format available.
Message #71 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 09:54 AM, Pádraig Brady wrote:
> On 11/21/2013 12:03 AM, Bernhard Voelker wrote:
>> if (dot_or_dotdot (last_component (ent->fts_accpath)))
>> {
>> - error (0, 0, _("cannot remove directory: %s"),
>> - quote (ent->fts_path));
>> + error (0, 0,
>> + _("refusing to remove %s or %s directory: skipping %s"),
>> + quote_n (0, "."), quote_n (1, ".."),
>> + quote_n (2, ent->fts_path));
>
> That's a good clarification. Please push.
Thanks, pushed:
http://git.sv.gnu.org/cgit/coreutils.git/commit/?id=2da7009d
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 13:40:01 GMT)
Full text and
rfc822 format available.
Message #74 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 12:12 AM, Bernhard Voelker wrote:
>
> Admittedly, compared to the academic question behind "--no-preserve-root"
> (which is like "what happens to me when the globe under my feet disappears?"),
> there may be more real-world reasons to remove ".".
But that's not what Linda is asking for. She is not asking to pull "."
out of under her feet. Instead, she wants a command that will
recursively remove the children of ".", but then leave "." itself
unremoved (whether by virtue of the fact that rmdir(".") must fail and
so the overall rm command fails, or by explicitly skipping the attempt
to rmdir(".") and letting rm succeed). Right now, the nanny rule of
POSIX is preventing the recursion, so you have to use contortions such
as the POSIX 'find . -depth ! -name . -exec rm {} +'. So I think it IS
useful to add an option that forces 'rm -r' to bypass the nanny rule and
recurse on ".".
Maybe naming it --no-preserve-dot is wrong. Maybe a better name is 'rm
-r --children-only .'. At which point, I would much rather see us skip
the rmdir(".") in order to allow rm to succeed. And it would also work
even for non-dot situations: 'rm -r --children-only dir'. In other
words, I _do_ see what Linda is asking for, and think it is worth providing.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 14:44:01 GMT)
Full text and
rfc822 format available.
Message #77 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 02:39 PM, Eric Blake wrote:
> [...] Instead, she wants a command that will
> recursively remove the children of ".", but then leave "." itself
> unremoved (whether by virtue of the fact that rmdir(".") must fail and
> so the overall rm command fails, or by explicitly skipping the attempt
> to rmdir(".") and letting rm succeed). Right now, the nanny rule of
> POSIX is preventing the recursion, so you have to use contortions such
> as the POSIX 'find . -depth ! -name . -exec rm {} +'. So I think it IS
> useful to add an option that forces 'rm -r' to bypass the nanny rule and
> recurse on ".".
I realized the difference when reading her latest message. (It's often
hard to follow her; sorry Linda.)
> Maybe naming it --no-preserve-dot is wrong. Maybe a better name is 'rm
> -r --children-only .'. At which point, I would much rather see us skip
> the rmdir(".") in order to allow rm to succeed. And it would also work
> even for non-dot situations: 'rm -r --children-only dir'. In other
> words, I_do_ see what Linda is asking for, and think it is worth providing.
Okay, I see the point: so now we're talking about the following
(with "." to be allowed instead of 'dir'):
$ mkdir dir dir/sub
$ touch dir/file dir/sub/another
$ rm -rv --children-only dir
removed 'dir/file'
removed 'dir/sub/another'
removed directory: 'dir/sub'
retaining directory argument due to --children-only: 'dir'
$ ls -d dir
dir
I'm not happy with the semantic as the difference to all other existing
uses of rm(1) would be that the argument is explicitly not removed,
but well, ...
Such --children-only would require some extra checking on arguments,
because only arguments of type directory are allowed.
And there'd be some strange corner cases to decide about, e.g. consider
rm -r --children dir/sub dir
Should "dir/sub" be removed/retained in this case? ...
Anyhow, handling such cases would add considerable bloat to the code.
Does anyone know of such an option in other rm(1) implementations?
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 15:13:02 GMT)
Full text and
rfc822 format available.
Message #80 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 07:42 AM, Bernhard Voelker wrote:
> I'm not happy with the semantic as the difference to all other existing
> uses of rm(1) would be that the argument is explicitly not removed,
> but well, ...
Such is life - and that's why it requires a new long option.
>
> Such --children-only would require some extra checking on arguments,
> because only arguments of type directory are allowed.
That's one idea. Another idea is to make the option handle directories
specially but have no change for regular files (similar to how 'rm -d'
handles directories specially but has no change for regular files).
> And there'd be some strange corner cases to decide about, e.g. consider
> rm -r --children dir/sub dir
> Should "dir/sub" be removed/retained in this case? ...
Removed; as always, rm processes command line arguments in order, so
even though dir/sub is left alone after the first argument is processed,
the second argument removes dir/sub without any care in the world what
processing the first argument did.
> Anyhow, handling such cases would add considerable bloat to the code.
Hard to say that it is considerable bloat without seeing a patch; we
already know when the top-level arguments are directories thanks to 'rm -d'.
>
> Does anyone know of such an option in other rm(1) implementations?
Not in rm(1) per se, but I _do_ know that the paradigm is common in
other hierarchical based systems. For example, in libvirt, you can
maintain a tree-based view of snapshots, and for removing a branch of
that tree, libvirt exposes the option to either remove a snapshot and
all its children, or to just remove the children but leave the snapshot
intact.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 15:44:02 GMT)
Full text and
rfc822 format available.
Message #83 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On Thu, Nov 21, 2013 at 5:39 AM, Eric Blake <eblake <at> redhat.com> wrote:
> On 11/21/2013 12:12 AM, Bernhard Voelker wrote:
...
> But that's not what Linda is asking for. She is not asking to pull "."
> out of under her feet. Instead, she wants a command that will
> recursively remove the children of ".", but then leave "." itself
> unremoved (whether by virtue of the fact that rmdir(".") must fail and
> so the overall rm command fails, or by explicitly skipping the attempt
> to rmdir(".") and letting rm succeed). Right now, the nanny rule of
> POSIX is preventing the recursion, so you have to use contortions such
> as the POSIX 'find . -depth ! -name . -exec rm {} +'. So I think it IS
> useful to add an option that forces 'rm -r' to bypass the nanny rule and
> recurse on ".".
>
> Maybe naming it --no-preserve-dot is wrong. Maybe a better name is 'rm
> -r --children-only .'. At which point, I would much rather see us skip
> the rmdir(".") in order to allow rm to succeed. And it would also work
> even for non-dot situations: 'rm -r --children-only dir'. In other
> words, I _do_ see what Linda is asking for, and think it is worth providing.
Thanks for clarifying, Eric. I am open to the idea. Amusingly,
when I first read your suggestion for --children-only, I thought,
"That's backwards, shouldn't it be '--adults-only' to go with the
no-nanny semantics?" Of course, "children" refers to tree parent/child
relationship, so would make sense in the manual, where there's no
talk of removing this nanny-like safeguard. But then I wondered about
non-directory arguments, and how the implied semantics of --children-only
applies (or not) to them. I don't particularly like the idea of adding
the long-named --(no-)?preserve-dot-or-dot-dot, because that would
render ambiguous any existing use of --(no-)?preserve- and all shorter
abbreviations of --(no-)?preserve-root.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 16:12:02 GMT)
Full text and
rfc822 format available.
Message #86 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On Wed, Nov 20, 2013 at 4:03 PM, Bernhard Voelker
<mail <at> bernhard-voelker.de> wrote:
> On 11/20/2013 02:44 PM, Eric Blake wrote:
>> On 11/19/2013 11:45 PM, Bernhard Voelker wrote:
>>> Maybe "cannot remove directory" is a bit weak - it's more like
>>> "refusing to remove dot|dot-dot|root directory".
>>
>> Indeed, a clearer error message would be possible.
>
> What about the following?
>
> $ src/rm -r src/.
> src/rm: refusing to remove '.' or '..' directory: skipping 'src/.'
>
> I didn't want to explicitly mention POSIX here ... it's just to clarify
> that rm(1) does not swallow errors from the kernel like EPERM, etc.
>
> The texinfo file is enhanced in the patch below, too.
...
> ---
> doc/coreutils.texi | 3 ++-
> src/remove.c | 12 ++++++++----
Thanks for doing that.
I was a little surprised to see an rm diagnostic change, but no
corresponding change in any test. Perhaps an opportunity for
increased test coverage?
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 16:19:01 GMT)
Full text and
rfc822 format available.
Message #89 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 04:06 PM, Eric Blake wrote:
> Hard to say that it is considerable bloat without seeing a patch; we
> already know when the top-level arguments are directories thanks to 'rm -d'.
Here's a draft - not tested more than this:
$ mkdir -p /tmp/dir /tmp/dir/sub
$ touch /tmp/dir/file /tmp/dir/sub/other
$ src/rm -rv --child /tmp/dir
removed ‘/tmp/dir/file’
removed ‘/tmp/dir/sub/other’
removed directory: ‘/tmp/dir/sub’
src/rm: skipping ‘/tmp/dir’, due to --children-only
$ src/rm -rv --child /tmp/dir
src/rm: skipping ‘/tmp/dir’, due to --children-only
$ src/rm -rv --child /tmp/dir/.
src/rm: skipping ‘/tmp/dir/.’, due to --children-only
Have a nice day,
Berny
From 03d58cc281c6155d50be9b770bbac7bf73cdaf92 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail <at> bernhard-voelker.de>
Date: Thu, 21 Nov 2013 17:11:27 +0100
Subject: [PATCH] rm: add --children-only option
FIXME
---
src/mv.c | 1 +
src/remove.c | 29 ++++++++++++++++++++++++-----
src/remove.h | 4 ++++
src/rm.c | 9 +++++++++
4 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/mv.c b/src/mv.c
index 1cfcd82..36e70a4 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -74,6 +74,7 @@ rm_option_init (struct rm_options *x)
{
x->ignore_missing_files = false;
x->remove_empty_directories = true;
+ x->children_only = false;
x->recursive = true;
x->one_file_system = false;
diff --git a/src/remove.c b/src/remove.c
index 3d386cf..eb05cb4 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -439,8 +439,10 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options
const *x)
{
/* POSIX says:
If the basename of a command line argument is "." or "..",
- diagnose it and do nothing more with that argument. */
- if (dot_or_dotdot (last_component (ent->fts_accpath)))
+ diagnose it and do nothing more with that argument.
+ FIXME: mention --children-only. */
+ if (! x->children_only
+ && dot_or_dotdot (last_component (ent->fts_accpath)))
{
error (0, 0,
_("refusing to remove %s or %s directory:
skipping %s"),
@@ -468,9 +470,17 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options
const *x)
if (s == RM_OK && is_empty_directory == T_YES)
{
- /* When we know (from prompt when in interactive mode)
- that this is an empty directory, don't prompt twice. */
- s = excise (fts, ent, x, true);
+ if (FTS_ROOTLEVEL == ent->fts_level && x->children_only)
+ {
+ error (0, 0, _("skipping %s, due to --children-only"),
+ quote (ent->fts_path));
+ }
+ else
+ {
+ /* When we know (from prompt when in interactive mode)
+ that this is an empty directory, don't prompt twice. */
+ s = excise (fts, ent, x, true);
+ }
fts_skip_tree (fts, ent);
}
@@ -492,6 +502,15 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options
const *x)
case FTS_NSOK: /* e.g., dangling symlink */
case FTS_DEFAULT: /* none of the above */
{
+ if (ent->fts_info == FTS_DP
+ && x->children_only
+ && FTS_ROOTLEVEL == ent->fts_level)
+ {
+ mark_ancestor_dirs (ent);
+ error (0, 0, _("skipping %s, due to --children-only"),
+ quote (ent->fts_path));
+ return RM_OK;
+ }
/* With --one-file-system, do not attempt to remove a mount point.
fts' FTS_XDEV ensures that we don't process any entries under
the mount point. */
diff --git a/src/remove.h b/src/remove.h
index 9ac54d4..248a470 100644
--- a/src/remove.h
+++ b/src/remove.h
@@ -52,6 +52,10 @@ struct rm_options
/* If true, remove empty directories. */
bool remove_empty_directories;
+ /* If true (and the -r option is also specified), remove all children
+ of directory arguments, yet retaining the directory itself. */
+ bool children_only;
+
/* Pointer to the device and inode numbers of '/', when --recursive
and preserving '/'. Otherwise NULL. */
struct dev_ino *root_dev_ino;
diff --git a/src/rm.c b/src/rm.c
index 7a51eef..0634855 100644
--- a/src/rm.c
+++ b/src/rm.c
@@ -51,6 +51,7 @@ enum
ONE_FILE_SYSTEM,
NO_PRESERVE_ROOT,
PRESERVE_ROOT,
+ CHILDREN_ONLY,
PRESUME_INPUT_TTY_OPTION
};
@@ -78,6 +79,7 @@ static struct option const long_opts[] =
{"recursive", no_argument, NULL, 'r'},
{"dir", no_argument, NULL, 'd'},
+ {"children-only", no_argument, NULL, CHILDREN_ONLY},
{"verbose", no_argument, NULL, 'v'},
{GETOPT_HELP_OPTION_DECL},
{GETOPT_VERSION_OPTION_DECL},
@@ -156,6 +158,8 @@ Remove (unlink) the FILE(s).\n\
--preserve-root do not remove '/' (default)\n\
-r, -R, --recursive remove directories and their contents
recursively\n\
-d, --dir remove empty directories\n\
+ --children-only remove only children, yet retaining the given\n\
+ directory arguments\n\
-v, --verbose explain what is being done\n\
"), stdout);
fputs (HELP_OPTION_DESCRIPTION, stdout);
@@ -192,6 +196,7 @@ rm_option_init (struct rm_options *x)
x->interactive = RMI_SOMETIMES;
x->one_file_system = false;
x->remove_empty_directories = false;
+ x->children_only = false;
x->recursive = false;
x->root_dev_ino = NULL;
x->stdin_tty = isatty (STDIN_FILENO);
@@ -296,6 +301,10 @@ main (int argc, char **argv)
preserve_root = true;
break;
+ case CHILDREN_ONLY:
+ x.children_only = true;
+ break;
+
case PRESUME_INPUT_TTY_OPTION:
x.stdin_tty = true;
break;
--
1.8.4.2
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 16:26:02 GMT)
Full text and
rfc822 format available.
Message #92 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 09:18 AM, Bernhard Voelker wrote:
> On 11/21/2013 04:06 PM, Eric Blake wrote:
>> Hard to say that it is considerable bloat without seeing a patch; we
>> already know when the top-level arguments are directories thanks to
>> 'rm -d'.
>
> Here's a draft - not tested more than this:
> +++ b/src/remove.h
> @@ -52,6 +52,10 @@ struct rm_options
> /* If true, remove empty directories. */
> bool remove_empty_directories;
>
> + /* If true (and the -r option is also specified), remove all children
> + of directory arguments, yet retaining the directory itself. */
> + bool children_only;
Should --children-only imply -r, rather than being a no-op when -r is
missing?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 16:35:01 GMT)
Full text and
rfc822 format available.
Message #95 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 05:25 PM, Eric Blake wrote:
> On 11/21/2013 09:18 AM, Bernhard Voelker wrote:
>> + /* If true (and the -r option is also specified), remove all children
>> + of directory arguments, yet retaining the directory itself. */
>> + bool children_only;
>
> Should --children-only imply -r, rather than being a no-op when -r is
> missing?
I thought about it, but as there's no way to --no-recursive,
the posibility to alias rm='/bin/rm --children-only' would be void.
There are surely other corner cases I didn't think about.
E.g. I was surprised to check for children-only in a second
place in the case "dir" is already empty.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 16:49:01 GMT)
Full text and
rfc822 format available.
Message #98 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 09:34 AM, Bernhard Voelker wrote:
> On 11/21/2013 05:25 PM, Eric Blake wrote:
>> On 11/21/2013 09:18 AM, Bernhard Voelker wrote:
>>> + /* If true (and the -r option is also specified), remove all children
>>> + of directory arguments, yet retaining the directory itself. */
>>> + bool children_only;
>>
>> Should --children-only imply -r, rather than being a no-op when -r is
>> missing?
>
> I thought about it, but as there's no way to --no-recursive,
> the posibility to alias rm='/bin/rm --children-only' would be void.
Good point (although someone using that as an alias for 'rm' is rather
brave! I'd personally create an alias with a different name than rm to
make it obvious that it doesn't remove the named argument; maybe alias
empty='rm --children-only -r' so that 'empty dir' is a self-describing
command to empty out the contents of dir). So I'm fine with your
proposal of still needing an explicit -r for it to make a difference.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:03:01 GMT)
Full text and
rfc822 format available.
Message #101 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On Thu, Nov 21, 2013 at 8:18 AM, Bernhard Voelker
<mail <at> bernhard-voelker.de> wrote:
> On 11/21/2013 04:06 PM, Eric Blake wrote:
>>
>> Hard to say that it is considerable bloat without seeing a patch; we
>> already know when the top-level arguments are directories thanks to 'rm
>> -d'.
>
>
> Here's a draft - not tested more than this:
>
> $ mkdir -p /tmp/dir /tmp/dir/sub
> $ touch /tmp/dir/file /tmp/dir/sub/other
>
> $ src/rm -rv --child /tmp/dir
> removed ‘/tmp/dir/file’
> removed ‘/tmp/dir/sub/other’
> removed directory: ‘/tmp/dir/sub’
> src/rm: skipping ‘/tmp/dir’, due to --children-only
>
> $ src/rm -rv --child /tmp/dir
> src/rm: skipping ‘/tmp/dir’, due to --children-only
>
> $ src/rm -rv --child /tmp/dir/.
> src/rm: skipping ‘/tmp/dir/.’, due to --children-only
>
>
> Have a nice day,
> Berny
>
> From 03d58cc281c6155d50be9b770bbac7bf73cdaf92 Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
> Date: Thu, 21 Nov 2013 17:11:27 +0100
> Subject: [PATCH] rm: add --children-only option
>
> FIXME
> ---
> src/mv.c | 1 +
> src/remove.c | 29 ++++++++++++++++++++++++-----
> src/remove.h | 4 ++++
> src/rm.c | 9 +++++++++
> 4 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/src/mv.c b/src/mv.c
> index 1cfcd82..36e70a4 100644
> --- a/src/mv.c
> +++ b/src/mv.c
> @@ -74,6 +74,7 @@ rm_option_init (struct rm_options *x)
> {
> x->ignore_missing_files = false;
> x->remove_empty_directories = true;
> + x->children_only = false;
> x->recursive = true;
> x->one_file_system = false;
>
> diff --git a/src/remove.c b/src/remove.c
> index 3d386cf..eb05cb4 100644
> --- a/src/remove.c
> +++ b/src/remove.c
> @@ -439,8 +439,10 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const
> *x)
> {
>
> /* POSIX says:
> If the basename of a command line argument is "." or "..",
> - diagnose it and do nothing more with that argument. */
> - if (dot_or_dotdot (last_component (ent->fts_accpath)))
> + diagnose it and do nothing more with that argument.
> + FIXME: mention --children-only. */
> + if (! x->children_only
> + && dot_or_dotdot (last_component (ent->fts_accpath)))
> {
> error (0, 0,
>
> _("refusing to remove %s or %s directory: skipping
> %s"),
> @@ -468,9 +470,17 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const
> *x)
>
> if (s == RM_OK && is_empty_directory == T_YES)
> {
> - /* When we know (from prompt when in interactive mode)
> - that this is an empty directory, don't prompt twice. */
> - s = excise (fts, ent, x, true);
> + if (FTS_ROOTLEVEL == ent->fts_level && x->children_only)
> + {
> + error (0, 0, _("skipping %s, due to --children-only"),
> + quote (ent->fts_path));
> + }
> + else
> + {
> + /* When we know (from prompt when in interactive mode)
> + that this is an empty directory, don't prompt twice. */
> + s = excise (fts, ent, x, true);
> + }
> fts_skip_tree (fts, ent);
> }
>
> @@ -492,6 +502,15 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const
> *x)
> case FTS_NSOK: /* e.g., dangling symlink */
> case FTS_DEFAULT: /* none of the above */
> {
> + if (ent->fts_info == FTS_DP
> + && x->children_only
> + && FTS_ROOTLEVEL == ent->fts_level)
> + {
> + mark_ancestor_dirs (ent);
> + error (0, 0, _("skipping %s, due to --children-only"),
> + quote (ent->fts_path));
> + return RM_OK;
> + }
> /* With --one-file-system, do not attempt to remove a mount point.
> fts' FTS_XDEV ensures that we don't process any entries under
> the mount point. */
> diff --git a/src/remove.h b/src/remove.h
> index 9ac54d4..248a470 100644
> --- a/src/remove.h
> +++ b/src/remove.h
> @@ -52,6 +52,10 @@ struct rm_options
> /* If true, remove empty directories. */
> bool remove_empty_directories;
>
> + /* If true (and the -r option is also specified), remove all children
> + of directory arguments, yet retaining the directory itself. */
> + bool children_only;
> +
> /* Pointer to the device and inode numbers of '/', when --recursive
> and preserving '/'. Otherwise NULL. */
> struct dev_ino *root_dev_ino;
> diff --git a/src/rm.c b/src/rm.c
> index 7a51eef..0634855 100644
> --- a/src/rm.c
> +++ b/src/rm.c
> @@ -51,6 +51,7 @@ enum
> ONE_FILE_SYSTEM,
> NO_PRESERVE_ROOT,
> PRESERVE_ROOT,
> + CHILDREN_ONLY,
> PRESUME_INPUT_TTY_OPTION
> };
>
> @@ -78,6 +79,7 @@ static struct option const long_opts[] =
>
> {"recursive", no_argument, NULL, 'r'},
> {"dir", no_argument, NULL, 'd'},
> + {"children-only", no_argument, NULL, CHILDREN_ONLY},
> {"verbose", no_argument, NULL, 'v'},
> {GETOPT_HELP_OPTION_DECL},
> {GETOPT_VERSION_OPTION_DECL},
> @@ -156,6 +158,8 @@ Remove (unlink) the FILE(s).\n\
> --preserve-root do not remove '/' (default)\n\
> -r, -R, --recursive remove directories and their contents
> recursively\n\
> -d, --dir remove empty directories\n\
> + --children-only remove only children, yet retaining the given\n\
> + directory arguments\n\
...
Thanks for the quick work.
What do you think about saying something simple yet imprecise in --help, like
"when recursive, suppress removal of any directory named on the command line"
with the caveat that the texinfo doc would add precision: but a
command-line-specified directory may still end up being removed when
it is also a subdirectory of another command line argument.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:19:02 GMT)
Full text and
rfc822 format available.
Message #104 received at 15926 <at> debbugs.gnu.org (full text, mbox):
Eric Blake wrote:
> But that's not what Linda is asking for. She is not asking to pull "."
> out of under her feet.
Actually as I understand it she is expecting the call to succeed if
the system kernel allows it. I believe that is the way rm used to
work before removing '.' was disallowed.
mkdir /tmp/testdir
cd /tmp/testdir
rmdir /tmp/testdir
echo $?
0
ls -ldog /tmp/testdir
ls: cannot access /tmp/testdir: No such file or directory
/bin/pwd
/bin/pwd: couldn't find directory entry in ‘..’ with matching i-node
That is fine in a flow such as a test flow or other where the action
is cleaning up on an exit for example. Or in other cases. It might
be making a new directory and changing to it afterward. Or something
else that makes sense in that case.
> Instead, she wants a command that will recursively remove the
> children of ".", but then leave "." itself unremoved (whether by
> virtue of the fact that rmdir(".") must fail
I am missing this part. Why must it fail? And in fact as per my test
case above it succeeds.
> Right now, the nanny rule of POSIX is preventing the recursion, so
> you have to use contortions such as the POSIX 'find . -depth ! -name
> . -exec rm {} +'. So I think it IS useful to add an option that
> forces 'rm -r' to bypass the nanny rule and recurse on ".".
Agreed.
> Maybe naming it --no-preserve-dot is wrong. Maybe a better name is 'rm
> -r --children-only .'. At which point, I would much rather see us skip
> the rmdir(".") in order to allow rm to succeed. And it would also work
> even for non-dot situations: 'rm -r --children-only dir'. In other
> words, I _do_ see what Linda is asking for, and think it is worth providing.
Agreed. And I rather like the --children-only semantics you have
proposed. It creates a generally useful behavior regardless of other
things. It isn't quite the same as bypassing the nanny rule for '.'
but it solves the issue while doing something generally useful at the
same time. Have to like it when it is two birds with one stone. :-)
Bob
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:35:02 GMT)
Full text and
rfc822 format available.
Message #107 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 10:18 AM, Bob Proulx wrote:
>
>> Instead, she wants a command that will recursively remove the
>> children of ".", but then leave "." itself unremoved (whether by
>> virtue of the fact that rmdir(".") must fail
>
> I am missing this part. Why must it fail? And in fact as per my test
> case above it succeeds.
No, if there were no nanny rule, then there is a difference between:
rm -r $PWD # Deletes dir out of under your feet, if allowed
rm -r . # leaves dir empty
rmdir(pwd) can succeed (true on Linux; it can also fail if the system
deems that a directory in use as a pwd can't be deleted - mingw is one
such system)
rmdir(".") MUST fail (rmdir(2) can only delete a directory if specified
by a non-. name)
Linda was asking to delete children but NOT the directory; she did NOT
want to use 'rm -r $PWD' because that attempts (and may succeed) at
deleting pwd. But she was ticked off that the nanny rules on "."
prevented recursion from even being attempted.
>
> Agreed. And I rather like the --children-only semantics you have
> proposed. It creates a generally useful behavior regardless of other
> things. It isn't quite the same as bypassing the nanny rule for '.'
Indeed - and if we still keep the nanny rule, then Linda would have to
use 'rm -r --children-only $PWD' because the nanny rule would block 'rm
-r --children-only .'; but since --children-only takes us out of the
realm of POSIX, we can also use it to skip the nanny rule.
> but it solves the issue while doing something generally useful at the
> same time. Have to like it when it is two birds with one stone. :-)
Yay - I'm glad that we appear to have come to what seems to be a useful
conclusion.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:36:02 GMT)
Full text and
rfc822 format available.
Message #110 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 01:39 PM, Eric Blake wrote:
> On 11/21/2013 12:12 AM, Bernhard Voelker wrote:
>
>>
>> Admittedly, compared to the academic question behind "--no-preserve-root"
>> (which is like "what happens to me when the globe under my feet disappears?"),
>> there may be more real-world reasons to remove ".".
>
> But that's not what Linda is asking for. She is not asking to pull "."
> out of under her feet. Instead, she wants a command that will
> recursively remove the children of ".", but then leave "." itself
> unremoved (whether by virtue of the fact that rmdir(".") must fail and
> so the overall rm command fails, or by explicitly skipping the attempt
> to rmdir(".") and letting rm succeed). Right now, the nanny rule of
> POSIX is preventing the recursion, so you have to use contortions such
> as the POSIX 'find . -depth ! -name . -exec rm {} +'. So I think it IS
> useful to add an option that forces 'rm -r' to bypass the nanny rule and
> recurse on ".".
>
> Maybe naming it --no-preserve-dot is wrong. Maybe a better name is 'rm
> -r --children-only .'. At which point, I would much rather see us skip
> the rmdir(".") in order to allow rm to succeed. And it would also work
> even for non-dot situations: 'rm -r --children-only dir'. In other
> words, I _do_ see what Linda is asking for, and think it is worth providing.
I'm not that enthusiastic for this new option,
as I don't see it as specific to rm.
I.E. other tools like chmod etc would have the same requirement,
and they might be handled with various shell globbing constructs.
Even more generally find(1) could be used to handle arbitrarily
many files and commands that don't support recursion internally.
Could you explain why rm would get this and say chmod would not?
thanks,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:39:02 GMT)
Full text and
rfc822 format available.
Message #113 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 10:35 AM, Pádraig Brady wrote:
> as I don't see it as specific to rm.
> I.E. other tools like chmod etc would have the same requirement,
> and they might be handled with various shell globbing constructs.
> Even more generally find(1) could be used to handle arbitrarily
> many files and commands that don't support recursion internally.
>
> Could you explain why rm would get this and say chmod would not?
Ideally, any command that implements recursion should have the option to
operate on children only. You are correct that rm should not be special
in this regards, so yes, I think chmod should also get it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:40:03 GMT)
Full text and
rfc822 format available.
Message #116 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 10:38 AM, Eric Blake wrote:
> On 11/21/2013 10:35 AM, Pádraig Brady wrote:
>> as I don't see it as specific to rm.
>> I.E. other tools like chmod etc would have the same requirement,
>> and they might be handled with various shell globbing constructs.
>> Even more generally find(1) could be used to handle arbitrarily
>> many files and commands that don't support recursion internally.
>>
>> Could you explain why rm would get this and say chmod would not?
>
> Ideally, any command that implements recursion should have the option to
> operate on children only. You are correct that rm should not be special
> in this regards, so yes, I think chmod should also get it.
Which says that maybe gnulib's fts module needs a new flag for recursion
without visiting the root node, rather than adding ad-hoc root node
exclusion into all clients.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:51:02 GMT)
Full text and
rfc822 format available.
Message #119 received at 15926 <at> debbugs.gnu.org (full text, mbox):
Eric Blake wrote:
> Pádraig Brady wrote:
> > as I don't see it as specific to rm.
> > I.E. other tools like chmod etc would have the same requirement,
> > and they might be handled with various shell globbing constructs.
> > Even more generally find(1) could be used to handle arbitrarily
> > many files and commands that don't support recursion internally.
> >
> > Could you explain why rm would get this and say chmod would not?
Argh! Feature creep!
The reason that rm should have it but chmod should not is that it is
to work around the POSIX nanny rule around '.' and '..'. Chmod does
not have such a nanny rule and therefore does not need that option.
> Ideally, any command that implements recursion should have the option to
> operate on children only. You are correct that rm should not be special
> in this regards, so yes, I think chmod should also get it.
This is actually the best argument against it. It is a slippery
slope. Let's not implement 'find' all over again.
Bob
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 17:56:01 GMT)
Full text and
rfc822 format available.
Message #122 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 21/11/2013 09:18, Bob Proulx wrote:
> Eric Blake wrote:
>> But that's not what Linda is asking for. She is not asking to pull "."
>> out of under her feet.
>
> Actually as I understand it she is expecting the call to succeed if
> the system kernel allows it. I believe that is the way rm used to
> work before removing '.' was disallowed.
----
Um... I *expect* ".* to be unremoveable. The entries "." and ".." are
required in all directories. These are required for a properly structured
directory. The only way to remove "." and ".." entries in
a directory is to remove the directory name itself:
rm -fr /tmp/mytmp
would remove mytmp + any contents (including the structural entries "." and
".." inside 'mytmp' . However, if I type "rm -fr /tmp/mytmp/." As is implemented
in most OS's, it would do a depth first traversal removal. At least on linux,
you couldn't remove "." as it is your current position. You can remove the
directory
which "." is in as you show below:
>
> mkdir /tmp/testdir
> cd /tmp/testdir
> rmdir /tmp/testdir
> echo $?
> 0
> ls -ldog /tmp/testdir
> ls: cannot access /tmp/testdir: No such file or directory
> /bin/pwd
> /bin/pwd: couldn't find directory entry in ‘..’ with matching i-node
So I expect anything containing "." foo/. to FAIL -- but only AFTER it has already
done depth first traversal. Adding the "-f" flag was to silence the error and
have the exit code set to '0' due to any failures.
Posix mandates checking "." *first*. when doing a recursive removal with "-f"..
So how about using -F as a gnu extension to ignore that case?
That POSIX would have declared "rm -fr ." illegal on nanny grounds goes against
the original spirit of why the "-f" flag was added in the 1st place. It meant
to "force" the issue, *if* possible (if permissions allowed). I have
no issue with error messages due to permission problems -- as they'd indicate
the directory wasn't "cleaned out" -- "rm -fr ." was to clean out the contents
of a dir to ready it for some reused.
So I propose adding a "-F" to force rm to adhere to its original algorithm and
override the POSIX restriction (as well as serving the purpose of "-f" to
force any removals.
>> Instead, she wants a command that will recursively remove the
>> children of ".", but then leave "." itself unremoved (whether by
>> virtue of the fact that rmdir(".") must fail
>
> I am missing this part. Why must it fail? And in fact as per my test
> case above it succeeds.
----
you didn't remove "." You removed the directory "." is
contained in. A direct removal of "." or ".." should be disallowed
because they are a required part of any directory. To removed them
you must remove the directory by name, but addressing the structural
entries must fail.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 18:02:01 GMT)
Full text and
rfc822 format available.
Message #125 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 10:50 AM, Bob Proulx wrote:
>>> Could you explain why rm would get this and say chmod would not?
>
> Argh! Feature creep!
Maybe, but it certainly seems like a useful feature.
>
> The reason that rm should have it but chmod should not is that it is
> to work around the POSIX nanny rule around '.' and '..'. Chmod does
> not have such a nanny rule and therefore does not need that option.
chmod -r $mod .
will change the permissions on '.'. POSIX has a nanny rule about 'rm -r
.', but it ALSO forbids rmdir("."). But the reason it has no nanny rule
for 'chmod -r .' is because chmod(".", mod) is allowed. There is NO way
with existing chmod to change the permissions on children but not on
chmod itself; you'd have to resort to 'find . -! -name . -exec chmod
$mod {} +'.
Doing a recursive action on children only IS a useful paradigm in
parent/child trees.
>
>> Ideally, any command that implements recursion should have the option to
>> operate on children only. You are correct that rm should not be special
>> in this regards, so yes, I think chmod should also get it.
>
> This is actually the best argument against it. It is a slippery
> slope. Let's not implement 'find' all over again.
If you are arguing that 'find' be the only program that implements
"recursion but leave the root alone", then Linda's counterargument of
needing a way to call remove() instead of unlink() when using 'rm'
instead of 'rm -r' is in effect; but we already demonstrated that our
extension of 'rm -d' serves that purpose. It's just that 'find . -depth
-! -name . -exec rm -d {} +' is a lot more typing than 'rm -r
--children-only .'.
At any rate, since both BSD and GNU rm had 'rm d', I'm going to submit a
bug to POSIX to request its standardization.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 18:02:02 GMT)
Full text and
rfc822 format available.
Message #128 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 05:39 PM, Eric Blake wrote:
> On 11/21/2013 10:38 AM, Eric Blake wrote:
>> On 11/21/2013 10:35 AM, Pádraig Brady wrote:
>>> as I don't see it as specific to rm.
>>> I.E. other tools like chmod etc would have the same requirement,
>>> and they might be handled with various shell globbing constructs.
>>> Even more generally find(1) could be used to handle arbitrarily
>>> many files and commands that don't support recursion internally.
>>>
>>> Could you explain why rm would get this and say chmod would not?
>>
>> Ideally, any command that implements recursion should have the option to
>> operate on children only. You are correct that rm should not be special
>> in this regards, so yes, I think chmod should also get it.
>
> Which says that maybe gnulib's fts module needs a new flag for recursion
> without visiting the root node, rather than adding ad-hoc root node
> exclusion into all clients.
That would be the right way to implement it,
_but_ the disadvantage is that this filtering
option is exposed to the users for all these commands.
I'm not sure it's useful enough functionality to expose,
and personally I'd use something like:
children() { find "$1" -maxdepth 1 -mindepth 1; }
or
children() { find "$1" | sed '1d'; }
and then...
children $dir | xargs rm -r
cheers,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 18:06:02 GMT)
Full text and
rfc822 format available.
Message #131 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 21/11/2013 09:50, Bob Proulx wrote:
> Eric Blake wrote:
>> P�draig Brady wrote:
>>> as I don't see it as specific to rm.
>>> I.E. other tools like chmod etc would have the same requirement,
>>> and they might be handled with various shell globbing constructs.
>>> Even more generally find(1) could be used to handle arbitrarily
>>> many files and commands that don't support recursion internally.
>>>
>>> Could you explain why rm would get this and say chmod would not?
>
> Argh! Feature creep!
>
> The reason that rm should have it but chmod should not is that it is
> to work around the POSIX nanny rule around '.' and '..'. Chmod does
> not have such a nanny rule and therefore does not need that option.
...
> This is actually the best argument against it. It is a slippery
> slope. Let's not implement 'find' all over again.
----
Let's just use '-F' to force "rm" to adhere to its original depth
first path examination. "-F" disallows applying any path related
rules until AFTER depth-first recursive execution has been completed
on the path.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 18:07:01 GMT)
Full text and
rfc822 format available.
Message #134 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/21/2013 11:01 AM, Pádraig Brady wrote:
> I'm not sure it's useful enough functionality to expose,
> and personally I'd use something like:
>
> children() { find "$1" -maxdepth 1 -mindepth 1; }
> or
> children() { find "$1" | sed '1d'; }
>
> and then...
>
> children $dir | xargs rm -r
Except that it falls over in the presence of file names with newline, so
you have to get fancier with find -print0 and xargs -0 to be robust.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 21 Nov 2013 20:03:01 GMT)
Full text and
rfc822 format available.
Message #137 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/19/2013 05:15 AM, Pádraig Brady wrote:
>> So how about upgrading 'rm' to use the remove function so
>> it would work on empty directories as well.
>
> Well we have the -d option to rm to explicitly do that.
> That's a fairly fundamental change that would have backwards compat issues.
> POSIX is explicit in the handling of directories also:
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/rm.html
I've filed a bug against POSIX requesting the standardization of 'rm -d':
http://austingroupbugs.net/view.php?id=802
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 14:57:02 GMT)
Full text and
rfc822 format available.
Message #140 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/21/2013 05:11 PM, Jim Meyering wrote:
> Thanks for doing that.
> I was a little surprised to see an rm diagnostic change, but no
> corresponding change in any test. Perhaps an opportunity for
> increased test coverage?
Sure! ;-)
Have a nice day,
Berny
From 2fa074ebd43cbfea0f6b4bc92a4099a273458562 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail <at> bernhard-voelker.de>
Date: Fri, 22 Nov 2013 15:54:06 +0100
Subject: [PATCH] tests: enhance rm test regarding "." and ".."
Recent commit 2da7009d changed the error diagnostic of rm(1) trying
to remove "." or "..". Enhance the corresponding test.
* tests/rm/r-4.sh: Ensure rm(1) outputs the expected error diagnostic.
---
tests/rm/r-4.sh | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/tests/rm/r-4.sh b/tests/rm/r-4.sh
index 94702a6..a3ceaf9 100755
--- a/tests/rm/r-4.sh
+++ b/tests/rm/r-4.sh
@@ -22,11 +22,26 @@ print_ver_ rm
mkdir d || framework_failure_
touch d/a || framework_failure_
-rm -fr d/. 2>/dev/null && fail=1
-rm -fr d/./ 2>/dev/null && fail=1
-rm -fr d/.//// 2>/dev/null && fail=1
-rm -fr d/.. 2>/dev/null && fail=1
-rm -fr d/../ 2>/dev/null && fail=1
+# Expected error diagnostic as grep pattern.
+exp="^rm: refusing to remove '\.' or '\.\.' directory: skipping '.*'\$"
+
+rmtest()
+{
+ # Try removing - expecting failure.
+ rm -fr "$1" 2> err && fail=1
+
+ # Ensure the expected error diagnostic is output.
+ grep "$exp" err || { cat err; fail=1; }
+
+ return $fail
+}
+
+rmtest 'd/.' || fail=1
+rmtest 'd/./' || fail=1
+rmtest 'd/.////' || fail=1
+rmtest 'd/..' || fail=1
+rmtest 'd/../' || fail=1
+
# This test is too dangerous -- if there's a bug you're wiped out!
# rm -fr / 2>/dev/null && fail=1
--
1.8.4.2
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 15:15:03 GMT)
Full text and
rfc822 format available.
Message #143 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/22/2013 07:56 AM, Bernhard Voelker wrote:
> On 11/21/2013 05:11 PM, Jim Meyering wrote:
>> Thanks for doing that.
>> I was a little surprised to see an rm diagnostic change, but no
>> corresponding change in any test. Perhaps an opportunity for
>> increased test coverage?
>
> Sure! ;-)
>
Just noticing this context...
>
> # This test is too dangerous -- if there's a bug you're wiped out!
> # rm -fr / 2>/dev/null && fail=1
What if we use chroot to create a safer /, where failing the test would
only wipe out the chroot?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 15:18:02 GMT)
Full text and
rfc822 format available.
Message #146 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/22/2013 02:56 PM, Bernhard Voelker wrote:
> On 11/21/2013 05:11 PM, Jim Meyering wrote:
>> Thanks for doing that.
>> I was a little surprised to see an rm diagnostic change, but no
>> corresponding change in any test. Perhaps an opportunity for
>> increased test coverage?
>
> Sure! ;-)
>
> Have a nice day,
> Berny
>
> From 2fa074ebd43cbfea0f6b4bc92a4099a273458562 Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
> Date: Fri, 22 Nov 2013 15:54:06 +0100
> Subject: [PATCH] tests: enhance rm test regarding "." and ".."
>
> Recent commit 2da7009d changed the error diagnostic of rm(1) trying
> to remove "." or "..". Enhance the corresponding test.
>
> * tests/rm/r-4.sh: Ensure rm(1) outputs the expected error diagnostic.
> ---
> tests/rm/r-4.sh | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tests/rm/r-4.sh b/tests/rm/r-4.sh
> index 94702a6..a3ceaf9 100755
> --- a/tests/rm/r-4.sh
> +++ b/tests/rm/r-4.sh
> @@ -22,11 +22,26 @@ print_ver_ rm
> mkdir d || framework_failure_
> touch d/a || framework_failure_
>
> -rm -fr d/. 2>/dev/null && fail=1
> -rm -fr d/./ 2>/dev/null && fail=1
> -rm -fr d/.//// 2>/dev/null && fail=1
> -rm -fr d/.. 2>/dev/null && fail=1
> -rm -fr d/../ 2>/dev/null && fail=1
> +# Expected error diagnostic as grep pattern.
> +exp="^rm: refusing to remove '\.' or '\.\.' directory: skipping '.*'\$"
> +
> +rmtest()
> +{
> + # Try removing - expecting failure.
> + rm -fr "$1" 2> err && fail=1
> +
> + # Ensure the expected error diagnostic is output.
> + grep "$exp" err || { cat err; fail=1; }
> +
> + return $fail
> +}
> +
> +rmtest 'd/.' || fail=1
> +rmtest 'd/./' || fail=1
> +rmtest 'd/.////' || fail=1
> +rmtest 'd/..' || fail=1
> +rmtest 'd/../' || fail=1
+1
thanks!
Pádraig
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 15:38:03 GMT)
Full text and
rfc822 format available.
Message #149 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/22/2013 04:14 PM, Eric Blake wrote:
> Just noticing this context...
>
>>
>> # This test is too dangerous -- if there's a bug you're wiped out!
>> # rm -fr / 2>/dev/null && fail=1
>
> What if we use chroot to create a safer /, where failing the test would
> only wipe out the chroot?
That's not that easy.
Alternatively, that test could be secured by "skip_if_root_"
plus intercepting the unlinkat() call via LD_PRELOAD.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 15:43:02 GMT)
Full text and
rfc822 format available.
Message #152 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/22/2013 08:36 AM, Bernhard Voelker wrote:
> On 11/22/2013 04:14 PM, Eric Blake wrote:
>> Just noticing this context...
>>
>>>
>>> # This test is too dangerous -- if there's a bug you're wiped out!
>>> # rm -fr / 2>/dev/null && fail=1
>>
>> What if we use chroot to create a safer /, where failing the test would
>> only wipe out the chroot?
>
> That's not that easy.
> Alternatively, that test could be secured by "skip_if_root_"
> plus intercepting the unlinkat() call via LD_PRELOAD.
Indeed, LD_PRELOAD is great for this - since the test passes when no
unlink/rmdir occurs, you just make the intercepts fail loudly if they
are invoked.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 16:06:01 GMT)
Full text and
rfc822 format available.
Message #155 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On Fri, Nov 22, 2013 at 6:56 AM, Bernhard Voelker
<mail <at> bernhard-voelker.de> wrote:
> On 11/21/2013 05:11 PM, Jim Meyering wrote:
>>
>> Thanks for doing that.
>> I was a little surprised to see an rm diagnostic change, but no
>> corresponding change in any test. Perhaps an opportunity for
>> increased test coverage?
>
> Sure! ;-)
...
> Subject: [PATCH] tests: enhance rm test regarding "." and ".."
>
> Recent commit 2da7009d changed the error diagnostic of rm(1) trying
> to remove "." or "..". Enhance the corresponding test.
>
> * tests/rm/r-4.sh: Ensure rm(1) outputs the expected error diagnostic.
Looks good. Thank you!
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 16:29:01 GMT)
Full text and
rfc822 format available.
Message #158 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/22/2013 05:04 PM, Jim Meyering wrote:
> Looks good. Thank you!
Thank you (both!) for the review.
Pushed;
http://git.sv.gnu.org/cgit/coreutils.git/commit/?id=2fa074eb
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 22 Nov 2013 17:40:02 GMT)
Full text and
rfc822 format available.
Message #161 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eric Blake wrote:
> Bernhard Voelker wrote:
> > Eric Blake wrote:
> >> Just noticing this context...
> >>> # This test is too dangerous -- if there's a bug you're wiped out!
> >>> # rm -fr / 2>/dev/null && fail=1
> >>
> >> What if we use chroot to create a safer /, where failing the test would
> >> only wipe out the chroot?
> >
> > That's not that easy.
I think that it would be too complicated to be desired in the test suite.
> > Alternatively, that test could be secured by "skip_if_root_"
> > plus intercepting the unlinkat() call via LD_PRELOAD.
>
> Indeed, LD_PRELOAD is great for this - since the test passes when no
> unlink/rmdir occurs, you just make the intercepts fail loudly if they
> are invoked.
Please make sure that if the LD_PRELOAD functionality fails that this
test is not run. Since it would be a live fire exercise and if
LD_PRELOAD doesn't function then the test would wipe the system out.
I don't think it is worth the risk for this piece of functionality.
Bob
[signature.asc (application/pgp-signature, inline)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 01:03:01 GMT)
Full text and
rfc822 format available.
Message #164 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/22/2013 06:39 PM, Bob Proulx wrote:
> Eric Blake wrote:
>> Bernhard Voelker wrote:
>>> Eric Blake wrote:
>>>> Just noticing this context...
>>>>> # This test is too dangerous -- if there's a bug you're wiped out!
>>>>> # rm -fr / 2>/dev/null && fail=1
>>>>
>>>> What if we use chroot to create a safer /, where failing the test would
>>>> only wipe out the chroot?
>>>
>>> That's not that easy.
>
> I think that it would be too complicated to be desired in the test suite.
>
>>> Alternatively, that test could be secured by "skip_if_root_"
>>> plus intercepting the unlinkat() call via LD_PRELOAD.
>>
>> Indeed, LD_PRELOAD is great for this - since the test passes when no
>> unlink/rmdir occurs, you just make the intercepts fail loudly if they
>> are invoked.
>
> Please make sure that if the LD_PRELOAD functionality fails that this
> test is not run. Since it would be a live fire exercise and if
> LD_PRELOAD doesn't function then the test would wipe the system out.
> I don't think it is worth the risk for this piece of functionality.
Sure, I tried to make it as defensive as possible.
WDYT?
Have a nice day,
Berny
From a87e3d0a8417648e65ee077ca6f70d5d19fa757a Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail <at> bernhard-voelker.de>
Date: Sat, 23 Nov 2013 01:55:36 +0100
Subject: [PATCH] tests: add a test for rm -rf "/"
* tests/rm/rm-root.sh: Add a non-root test.
* tests/local.mk (all_tests): Mention the test.
---
tests/local.mk | 1 +
tests/rm/rm-root.sh | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 160 insertions(+)
create mode 100755 tests/rm/rm-root.sh
diff --git a/tests/local.mk b/tests/local.mk
index 3c92425..1837690 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -208,6 +208,7 @@ all_tests = \
tests/rm/rm3.sh \
tests/rm/rm4.sh \
tests/rm/rm5.sh \
+ tests/rm/rm-root.sh \
tests/rm/sunos-1.sh \
tests/rm/unread2.sh \
tests/rm/unread3.sh \
diff --git a/tests/rm/rm-root.sh b/tests/rm/rm-root.sh
new file mode 100755
index 0000000..3882289
--- /dev/null
+++ b/tests/rm/rm-root.sh
@@ -0,0 +1,159 @@
+#!/bin/sh
+# Try to remove '/' recursively.
+
+# Copyright (C) 2013 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ rm
+
+# POSIX mandates rm(1) to skip '/' arguments. This test verifies this mandated
+# behavior as well as the --preserve-root and --no-preserve-root options.
+# Especially the latter case is a live fire exercise as rm(1) is supposed to
+# enter the unlinkat() system call. Therefore, limit the risk as much
+# as possible -- if there's a bug this test would wipe the system out!
+
+# Faint-hearted: skip this test for the 'root' user.
+skip_if_root_
+
+# Pull rm(1) the teeth by intercepting the unlinkat() system call via the
+# LD_PRELOAD environment variable. This requires shared libraries to work.
+require_gcc_shared_
+
+cat > k.c <<'EOF' || framework_failure_
+#include <stdio.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+int unlinkat(int dirfd, const char *pathname, int flags)
+{
+ /* Prove that LD_PRELOAD works. */
+ fclose (fopen ("x", "w"));
+
+ /* Immediately terminate. */
+ _exit(0);
+}
+EOF
+
+# Then compile/link it:
+gcc -Wall --std=gnu99 -shared -fPIC -ldl -O2 k.c -o k.so \
+ || framework_failure_ 'failed to build shared library'
+
+# Verify that "rm -rf dir" basically works.
+mkdir dir || framework_failure_
+rm -rf dir || framework_failure_
+test -d dir && framework_failure_
+
+# Now verify that intercepting unlinkat() works:
+# rm(1) must succeed as before, but this time both the evidence file "x"
+# and the test directory "dir" must exist afterwards.
+mkdir dir || framework_failure_
+LD_PRELOAD=./k.so \
+rm -rf dir || framework_failure_
+test -d dir || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
+test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
+
+# Remove the evidence file "x"; verify that.
+rm x || framework_failure_
+test -f x && framework_failure_
+
+#-------------------------------------------------------------------------------
+# exercise_rm_rf_root: shell function to test "rm -rf '/'"
+# Paranoia mode on:
+# For the worst case where both rm(1) would fail to refuse to process the "/"
+# argument (in the cases without the --no-preserve-root option), and
+# intercepting the unlinkat(1) system call would fail (which actually already
+# has been proven to work above), limit the damage to the current file system
+# via the --one-file-system option.
+# Furthermore, run rm(1) in the background and kill that process after
+# a maximum of 1 second or when the evidence file appears. This also
+# shortens the testing time.
+exercise_rm_rf_root ()
+{
+ local pid
+ LD_PRELOAD=./k.so \
+ rm -rfv --one-file-system $1 '/' > out 2> err & pid=$!
+
+ # Wait for the evidence file to appear, or until the process has terminated.
+ for i in $(seq 10); do
+ test -f x && break
+ kill -0 $pid || break
+ sleep .1
+ done
+
+ # At this point, rm(1) usually has already terminated. Kill it anyway.
+ kill -9 $pid
+
+ # Get the exit status.
+ wait $pid
+
+ return $?
+}
+
+# "rm -rf /" without --no-preserve-root should output the following
+# diagnostic error message.
+cat <<EOD > exp || framework_failure_
+rm: it is dangerous to operate recursively on '/'
+rm: use --no-preserve-root to override this failsafe
+EOD
+
+#-------------------------------------------------------------------------------
+# Exercise "rm -rf /" without the --preserve-root and --no-preserve-root option.
+# Expect a non-Zero exist status.
+exercise_rm_rf_root \
+ && fail=1
+
+# Expect nothing in 'out' and the above error diagnostic in 'err'.
+# As rm(1) should have skipped the "/" argument, it does not call unlinkat().
+# Therefore, the evidence file "x" should not exist.
+compare /dev/null out || fail=1
+compare exp err || fail=1
+test -f x && fail=1
+
+# Do nothing more if this test failed.
+test $fail = 1 && { cat out; cat err; Exit $fail; }
+
+#-------------------------------------------------------------------------------
+# Exercise "rm -rf --preserve-root /" which should behave the same as above.
+exercise_rm_rf_root --preserve-root \
+ && fail=1
+
+compare /dev/null out || fail=1
+compare exp err || fail=1
+test -f x && fail=1
+
+# Do nothing more if this test failed.
+test $fail = 1 && { cat out; cat err; Exit $fail; }
+
+#-------------------------------------------------------------------------------
+# Until now, it was all just fun.
+# Now exercise the --no-preserve-root option with which rm(1) should enter
+# the intercepted unlinkat() system call.
+# As the interception code terminates the process immediately via _exit(0),
+# the exist status should be 0.
+exercise_rm_rf_root --no-preserve-root \
+ || fail=1
+
+# The 'err' file should not contain the above error diagostic.
+grep "^rm: it is dangerous to operate recursively on '/'" err \
+ && fail=1
+
+# Instead, rm(1) should have called the intercepted unlinkat() function,
+# i.e. the evidence file "x" should exist.
+test -f x || fail=1
+
+test $fail = 1 && { cat out; cat err; Exit $fail; }
+
+Exit $fail
--
1.8.3.1
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 01:31:02 GMT)
Full text and
rfc822 format available.
Message #167 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/23/2013 01:02 AM, Bernhard Voelker wrote:
> On 11/22/2013 06:39 PM, Bob Proulx wrote:
>> Eric Blake wrote:
>>> Bernhard Voelker wrote:
>>>> Eric Blake wrote:
>>>>> Just noticing this context...
>>>>>> # This test is too dangerous -- if there's a bug you're wiped out!
>>>>>> # rm -fr / 2>/dev/null && fail=1
>>>>>
>>>>> What if we use chroot to create a safer /, where failing the test would
>>>>> only wipe out the chroot?
>>>>
>>>> That's not that easy.
>>
>> I think that it would be too complicated to be desired in the test suite.
>>
>>>> Alternatively, that test could be secured by "skip_if_root_"
>>>> plus intercepting the unlinkat() call via LD_PRELOAD.
>>>
>>> Indeed, LD_PRELOAD is great for this - since the test passes when no
>>> unlink/rmdir occurs, you just make the intercepts fail loudly if they
>>> are invoked.
>>
>> Please make sure that if the LD_PRELOAD functionality fails that this
>> test is not run. Since it would be a live fire exercise and if
>> LD_PRELOAD doesn't function then the test would wipe the system out.
>> I don't think it is worth the risk for this piece of functionality.
>
> Sure, I tried to make it as defensive as possible.
> WDYT?
>
> Have a nice day,
> Berny
>
>>From a87e3d0a8417648e65ee077ca6f70d5d19fa757a Mon Sep 17 00:00:00 2001
> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
> Date: Sat, 23 Nov 2013 01:55:36 +0100
> Subject: [PATCH] tests: add a test for rm -rf "/"
>
> * tests/rm/rm-root.sh: Add a non-root test.
> * tests/local.mk (all_tests): Mention the test.
> ---
> tests/local.mk | 1 +
> tests/rm/rm-root.sh | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 160 insertions(+)
> create mode 100755 tests/rm/rm-root.sh
>
> diff --git a/tests/local.mk b/tests/local.mk
> index 3c92425..1837690 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -208,6 +208,7 @@ all_tests = \
> tests/rm/rm3.sh \
> tests/rm/rm4.sh \
> tests/rm/rm5.sh \
> + tests/rm/rm-root.sh \
> tests/rm/sunos-1.sh \
> tests/rm/unread2.sh \
> tests/rm/unread3.sh \
> diff --git a/tests/rm/rm-root.sh b/tests/rm/rm-root.sh
> new file mode 100755
> index 0000000..3882289
> --- /dev/null
> +++ b/tests/rm/rm-root.sh
> @@ -0,0 +1,159 @@
> +#!/bin/sh
> +# Try to remove '/' recursively.
> +
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
> +print_ver_ rm
> +
> +# POSIX mandates rm(1) to skip '/' arguments. This test verifies this mandated
> +# behavior as well as the --preserve-root and --no-preserve-root options.
> +# Especially the latter case is a live fire exercise as rm(1) is supposed to
> +# enter the unlinkat() system call. Therefore, limit the risk as much
> +# as possible -- if there's a bug this test would wipe the system out!
> +
> +# Faint-hearted: skip this test for the 'root' user.
> +skip_if_root_
good :)
> +
> +# Pull rm(1) the teeth by intercepting the unlinkat() system call via the
s/the// ?
> +# LD_PRELOAD environment variable. This requires shared libraries to work.
> +require_gcc_shared_
> +
> +cat > k.c <<'EOF' || framework_failure_
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> +
> +int unlinkat(int dirfd, const char *pathname, int flags)
s/(/ (/
> +{
> + /* Prove that LD_PRELOAD works. */
> + fclose (fopen ("x", "w"));
> +
> + /* Immediately terminate. */
> + _exit(0);
s/(/ (/
> +}
> +EOF
> +
> +# Then compile/link it:
> +gcc -Wall --std=gnu99 -shared -fPIC -ldl -O2 k.c -o k.so \
> + || framework_failure_ 'failed to build shared library'
> +
> +# Verify that "rm -rf dir" basically works.
> +mkdir dir || framework_failure_
> +rm -rf dir || framework_failure_
> +test -d dir && framework_failure_
>
> +# Now verify that intercepting unlinkat() works:
> +# rm(1) must succeed as before, but this time both the evidence file "x"
> +# and the test directory "dir" must exist afterwards.
> +mkdir dir || framework_failure_
> +LD_PRELOAD=./k.so \
> +rm -rf dir || framework_failure_
> +test -d dir || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
> +test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
> +
> +# Remove the evidence file "x"; verify that.
> +rm x || framework_failure_
> +test -f x && framework_failure_
> +
> +#-------------------------------------------------------------------------------
> +# exercise_rm_rf_root: shell function to test "rm -rf '/'"
> +# Paranoia mode on:
> +# For the worst case where both rm(1) would fail to refuse to process the "/"
> +# argument (in the cases without the --no-preserve-root option), and
> +# intercepting the unlinkat(1) system call would fail (which actually already
> +# has been proven to work above), limit the damage to the current file system
> +# via the --one-file-system option.
> +# Furthermore, run rm(1) in the background and kill that process after
> +# a maximum of 1 second or when the evidence file appears. This also
> +# shortens the testing time.
> +exercise_rm_rf_root ()
> +{
> + local pid
> + LD_PRELOAD=./k.so \
> + rm -rfv --one-file-system $1 '/' > out 2> err & pid=$!
> +
> + # Wait for the evidence file to appear, or until the process has terminated.
> + for i in $(seq 10); do
> + test -f x && break
> + kill -0 $pid || break
> + sleep .1
> + done
better to use retry_delay_ here I think and just wait for x file
> + # At this point, rm(1) usually has already terminated. Kill it anyway.
> + kill -9 $pid
probably best use a timeout 10 ... on the original rm call
Oh you're trying to minimize run time. I wouldn't bother TBH,
you can't really be half dead.
> +
> + # Get the exit status.
> + wait $pid
> +
> + return $?
> +}
> +
> +# "rm -rf /" without --no-preserve-root should output the following
> +# diagnostic error message.
> +cat <<EOD > exp || framework_failure_
> +rm: it is dangerous to operate recursively on '/'
> +rm: use --no-preserve-root to override this failsafe
> +EOD
> +
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -rf /" without the --preserve-root and --no-preserve-root option.
> +# Expect a non-Zero exist status.
> +exercise_rm_rf_root \
> + && fail=1
> +
> +# Expect nothing in 'out' and the above error diagnostic in 'err'.
> +# As rm(1) should have skipped the "/" argument, it does not call unlinkat().
> +# Therefore, the evidence file "x" should not exist.
> +compare /dev/null out || fail=1
> +compare exp err || fail=1
> +test -f x && fail=1
> +
> +# Do nothing more if this test failed.
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
> +
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -rf --preserve-root /" which should behave the same as above.
> +exercise_rm_rf_root --preserve-root \
> + && fail=1
> +
> +compare /dev/null out || fail=1
> +compare exp err || fail=1
> +test -f x && fail=1
> +
> +# Do nothing more if this test failed.
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
> +
> +#-------------------------------------------------------------------------------
> +# Until now, it was all just fun.
> +# Now exercise the --no-preserve-root option with which rm(1) should enter
> +# the intercepted unlinkat() system call.
> +# As the interception code terminates the process immediately via _exit(0),
> +# the exist status should be 0.
s/exist/exit/
> +exercise_rm_rf_root --no-preserve-root \
> + || fail=1
> +
> +# The 'err' file should not contain the above error diagostic.
> +grep "^rm: it is dangerous to operate recursively on '/'" err \
> + && fail=1
> +
> +# Instead, rm(1) should have called the intercepted unlinkat() function,
> +# i.e. the evidence file "x" should exist.
> +test -f x || fail=1
> +
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
> +
> +Exit $fail
>
So the real case where this could not be handled (and what might
actually catch users out) is when various synonyms of '/' are specified.
i.e. // //. /./ /bin/..
It would be good to have tests for those.
cheers,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 02:30:04 GMT)
Full text and
rfc822 format available.
Message #170 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/22/2013 06:02 PM, Bernhard Voelker wrote:
> On 11/22/2013 06:39 PM, Bob Proulx wrote:
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -rf /" without the --preserve-root and --no-preserve-root option.
> +# Expect a non-Zero exist status.
> +exercise_rm_rf_root \
> + && fail=1
Maybe you should favor 'rm -r /' rather than 'rm -rf /'. That way, even
if all the failsafes are bypassed, you at least have better logging of
what started to go wrong rather than silence.
Also, probably worth testing:
rm -r / a
to make sure that 'a' DOES get deleted, even though we skipped over the
/ argument, and that we still get the final exit status representing
failure to remove /.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 02:33:01 GMT)
Full text and
rfc822 format available.
Message #173 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/22/2013 06:30 PM, Pádraig Brady wrote:
>> +
>> +# Pull rm(1) the teeth by intercepting the unlinkat() system call via the
>
> s/the// ?
Pull the teeth from rm(1) by...
>
> So the real case where this could not be handled (and what might
> actually catch users out) is when various synonyms of '/' are specified.
> i.e. // //. /./ /bin/..
> It would be good to have tests for those.
Careful on the // case, for systems like cygwin where // is a distinct
root from / (it has a separate inode, but should likewise be forbidden
to recursively delete from // - hmm, I don't know that I've ever been
brave enough to actually test whether --preserve-root preserves // on
cygwin).
Also, if a user does 'ln -s / root' and does 'rm -r root/.'
(technically, this should also be done for 'rm -r root/', but Linux
doesn't obey POSIX with regards to 'rm symlink-to-dir/').
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 10:57:02 GMT)
Full text and
rfc822 format available.
Message #176 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/23/2013 02:32 AM, Eric Blake wrote:
> On 11/22/2013 06:30 PM, Pádraig Brady wrote:
>>> +
>>> +# Pull rm(1) the teeth by intercepting the unlinkat() system call via the
>>
>> s/the// ?
>
> Pull the teeth from rm(1) by...
>
>>
>> So the real case where this could not be handled (and what might
>> actually catch users out) is when various synonyms of '/' are specified.
>> i.e. // //. /./ /bin/..
>> It would be good to have tests for those.
>
> Careful on the // case, for systems like cygwin where // is a distinct
> root from / (it has a separate inode, but should likewise be forbidden
> to recursively delete from // - hmm, I don't know that I've ever been
> brave enough to actually test whether --preserve-root preserves // on
> cygwin).
I would expect // to be treated the same on all systems in this regard
> Also, if a user does 'ln -s / root' and does 'rm -r root/.'
> (technically, this should also be done for 'rm -r root/', but Linux
> doesn't obey POSIX with regards to 'rm symlink-to-dir/').
If rm is going to do this at all it might as well
handle all corner cases (that are much more likely to
catch users out). I.E. it should be working on the
canonicalized name.
cheers,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 23 Nov 2013 11:50:01 GMT)
Full text and
rfc822 format available.
Message #179 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/23/2013 10:55 AM, Pádraig Brady wrote:
> On 11/23/2013 02:32 AM, Eric Blake wrote:
>> On 11/22/2013 06:30 PM, Pádraig Brady wrote:
>>>> +
>>>> +# Pull rm(1) the teeth by intercepting the unlinkat() system call via the
>>>
>>> s/the// ?
>>
>> Pull the teeth from rm(1) by...
>>
>>>
>>> So the real case where this could not be handled (and what might
>>> actually catch users out) is when various synonyms of '/' are specified.
>>> i.e. // //. /./ /bin/..
>>> It would be good to have tests for those.
as explained below, the names containing . or .. would not
contain this particular diagnostic, though we'd probably
want to test anyway and ensure ignored to protect against
future reordering of the checks in code.
Also a symlink test as described below would be good to add.
>> Careful on the // case, for systems like cygwin where // is a distinct
>> root from / (it has a separate inode, but should likewise be forbidden
>> to recursively delete from // - hmm, I don't know that I've ever been
>> brave enough to actually test whether --preserve-root preserves // on
>> cygwin).
>
> I would expect // to be treated the same on all systems in this regard
>
>> Also, if a user does 'ln -s / root' and does 'rm -r root/.'
>> (technically, this should also be done for 'rm -r root/', but Linux
>> doesn't obey POSIX with regards to 'rm symlink-to-dir/').
>
> If rm is going to do this at all it might as well
> handle all corner cases (that are much more likely to
> catch users out). I.E. it should be working on the
> canonicalized name.
To be clear, I think rm is working as expected already,
since it checks the dev,inode identity of the command argument
against that of '/'. So symlinks are handled as expected:
$ rm -ri ~/root/
src/rm: it is dangerous to operate recursively on ‘/home/padraig/root/’ (same as ‘/’)
src/rm: use --no-preserve-root to override this failsafe
Note the trailing '.' for the 'root/.' case will trigger that check first.
cheers,
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Mon, 25 Nov 2013 00:17:01 GMT)
Full text and
rfc822 format available.
Message #182 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/23/2013 02:30 AM, Pádraig Brady wrote:
> On 11/23/2013 01:02 AM, Bernhard Voelker wrote:
>> >From a87e3d0a8417648e65ee077ca6f70d5d19fa757a Mon Sep 17 00:00:00 2001
>> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
>> Date: Sat, 23 Nov 2013 01:55:36 +0100
>> Subject: [PATCH] tests: add a test for rm -rf "/"
Hi Padraig & Eric,
sorry for the late reply - my system had a few problems after
running this test. no, just kidding ;-)
First of all, thank you for the nice hints.
>> diff --git a/tests/rm/rm-root.sh b/tests/rm/rm-root.sh
>> new file mode 100755
>> index 0000000..3882289
>> [...]
>> +exercise_rm_rf_root ()
>> +{
>> + local pid
>> + LD_PRELOAD=./k.so \
>> + rm -rfv --one-file-system $1 '/' > out 2> err & pid=$!
>> +
>> + # Wait for the evidence file to appear, or until the process has terminated.
>> + for i in $(seq 10); do
>> + test -f x && break
>> + kill -0 $pid || break
>> + sleep .1
>> + done
>
> better to use retry_delay_ here I think and just wait for x file
>
>> + # At this point, rm(1) usually has already terminated. Kill it anyway.
>> + kill -9 $pid
>
> probably best use a timeout 10 ... on the original rm call
> Oh you're trying to minimize run time. I wouldn't bother TBH,
> you can't really be half dead.
I wanted to be as conservative as possible in this test.
Therefore I think in the worst case we shouldn't give 'rm' more
than 1 second to destroy possibly valuable user data.
Regarding half dead: I tried it: on a system where /home is on a separate
partition, "rm -rf --one-file-system --no-preserve-root /" did not remove
more than /var/spool/mail/$USER and the files in /tmp owned by that USER.
That's not too bad. ;-)
> So the real case where this could not be handled (and what might
> actually catch users out) is when various synonyms of '/' are specified.
> i.e. // //. /./ /bin/..
> It would be good to have tests for those.
I've added tests like this - which partially run into the "refusing to
remove '.' or '..'" case, as you pointed out in the other mails.
Regarding Cygwin:
'rm' - the version which is currently shipped there - behaves the same
on this platform: it skips /, //, ///, //// etc.
LD_PRELOAD seems to be defined on Cygwin, too, but as 'rm' invokes
a different system call there, this test will be skipped.
On 11/23/2013 03:28 AM, Eric Blake wrote:
> Maybe you should favor 'rm -r /' rather than 'rm -rf /'.
Fixed.
> Also, probably worth testing:
>
> rm -r / a
>
> to make sure that 'a' DOES get deleted, even though we skipped over the
> / argument, and that we still get the final exit status representing
> failure to remove /.
Added a test.
Attached is the new patch.
One question (which is also as a FIXME in the patch):
for /, rm outputs the diagnostic
rm: it is dangerous to operate recursively on '/'
while for // it outputs
rm: it is dangerous to operate recursively on '//' (same as '/')
However, for ///, //// etc it output again the former message.
Why?
Thanks & have a nice day,
Berny
[0001-tests-add-a-test-for-rm-rf-v2.patch (text/x-patch, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Mon, 25 Nov 2013 01:11:02 GMT)
Full text and
rfc822 format available.
Message #185 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/25/2013 12:15 AM, Bernhard Voelker wrote:
> On 11/23/2013 02:30 AM, Pádraig Brady wrote:
>> On 11/23/2013 01:02 AM, Bernhard Voelker wrote:
>>> >From a87e3d0a8417648e65ee077ca6f70d5d19fa757a Mon Sep 17 00:00:00 2001
>>> From: Bernhard Voelker <mail <at> bernhard-voelker.de>
>>> Date: Sat, 23 Nov 2013 01:55:36 +0100
>>> Subject: [PATCH] tests: add a test for rm -rf "/"
>
> Hi Padraig & Eric,
>
> sorry for the late reply - my system had a few problems after
> running this test. no, just kidding ;-)
>
> First of all, thank you for the nice hints.
>
>>> diff --git a/tests/rm/rm-root.sh b/tests/rm/rm-root.sh
>>> new file mode 100755
>>> index 0000000..3882289
>
>>> [...]
>
>>> +exercise_rm_rf_root ()
>>> +{
>>> + local pid
>>> + LD_PRELOAD=./k.so \
>>> + rm -rfv --one-file-system $1 '/' > out 2> err & pid=$!
>>> +
>>> + # Wait for the evidence file to appear, or until the process has terminated.
>>> + for i in $(seq 10); do
>>> + test -f x && break
>>> + kill -0 $pid || break
>>> + sleep .1
>>> + done
>>
>> better to use retry_delay_ here I think and just wait for x file
>>
>>> + # At this point, rm(1) usually has already terminated. Kill it anyway.
>>> + kill -9 $pid
>>
>> probably best use a timeout 10 ... on the original rm call
>> Oh you're trying to minimize run time. I wouldn't bother TBH,
>> you can't really be half dead.
>
> I wanted to be as conservative as possible in this test.
> Therefore I think in the worst case we shouldn't give 'rm' more
> than 1 second to destroy possibly valuable user data.
>
> Regarding half dead: I tried it: on a system where /home is on a separate
> partition, "rm -rf --one-file-system --no-preserve-root /" did not remove
> more than /var/spool/mail/$USER and the files in /tmp owned by that USER.
> That's not too bad. ;-)
>
>> So the real case where this could not be handled (and what might
>> actually catch users out) is when various synonyms of '/' are specified.
>> i.e. // //. /./ /bin/..
>> It would be good to have tests for those.
>
> I've added tests like this - which partially run into the "refusing to
> remove '.' or '..'" case, as you pointed out in the other mails.
>
> Regarding Cygwin:
> 'rm' - the version which is currently shipped there - behaves the same
> on this platform: it skips /, //, ///, //// etc.
> LD_PRELOAD seems to be defined on Cygwin, too, but as 'rm' invokes
> a different system call there, this test will be skipped.
>
> On 11/23/2013 03:28 AM, Eric Blake wrote:
>> Maybe you should favor 'rm -r /' rather than 'rm -rf /'.
>
> Fixed.
>
>> Also, probably worth testing:
>>
>> rm -r / a
>>
>> to make sure that 'a' DOES get deleted, even though we skipped over the
>> / argument, and that we still get the final exit status representing
>> failure to remove /.
>
> Added a test.
>
> Attached is the new patch.
>
> One question (which is also as a FIXME in the patch):
> for /, rm outputs the diagnostic
> rm: it is dangerous to operate recursively on '/'
> while for // it outputs
> rm: it is dangerous to operate recursively on '//' (same as '/')
> However, for ///, //// etc it output again the former message.
> Why?
Interesting. So the warning is from ROOT_DEV_INO_WARN (ent->fts_path)
and that uses STREQ("/",...) on the fts_path.
Perhaps fts is being conservative and treating '//' as a distinct root,
thus not canonicalizing it to '/'?
Now gnulib has a specific check for this:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/double-slash-root.m4;hb=HEAD
which gnulib and coreutils honors at least.
For this edge case I suppose you could call canonicalize within ROOT_DEV_INO_WARN()
though I'm not sure it's worth it.
> diff --git a/tests/rm/rm-root.sh b/tests/rm/rm-root.sh
> new file mode 100755
> index 0000000..cfce279
> --- /dev/null
> +++ b/tests/rm/rm-root.sh
> @@ -0,0 +1,248 @@
> +#!/bin/sh
> +# Try to remove '/' recursively.
> +
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
> +print_ver_ rm
> +
> +# POSIX mandates rm(1) to skip '/' arguments. This test verifies this mandated
> +# behavior as well as the --preserve-root and --no-preserve-root options.
> +# Especially the latter case is a live fire exercise as rm(1) is supposed to
> +# enter the unlinkat() system call. Therefore, limit the risk as much
> +# as possible -- if there's a bug this test would wipe the system out!
> +
> +# Faint-hearted: skip this test for the 'root' user.
> +skip_if_root_
> +
> +# Pull the teeth from rm(1) by intercepting the unlinkat() system call via the
> +# LD_PRELOAD environment variable. This requires shared libraries to work.
> +require_gcc_shared_
> +
> +cat > k.c <<'EOF' || framework_failure_
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int unlinkat (int dirfd, const char *pathname, int flags)
> +{
> + /* Prove that LD_PRELOAD works: create the evidence file "x". */
> + fclose (fopen ("x", "w"));
> +
> + /* Immediately terminate, unless indicated otherwise. */
> + if (! getenv("CU_TEST_SKIP_EXIT"))
> + _exit (0);
> +
> + /* Pretend success. */
> + return 0;
> +}
> +EOF
> +
> +# Then compile/link it:
> +gcc -Wall --std=gnu99 -shared -fPIC -ldl -O2 k.c -o k.so \
> + || framework_failure_ 'failed to build shared library'
> +
> +# Verify that "rm -r dir" basically works.
> +mkdir dir || framework_failure_
> +rm -r dir || framework_failure_
> +test -d dir && framework_failure_
> +
> +# Now verify that intercepting unlinkat() works:
> +# rm(1) must succeed as before, but this time both the evidence file "x"
> +# and the test directory "dir" must exist afterwards.
> +mkdir dir || framework_failure_
> +LD_PRELOAD=./k.so \
> +rm -r dir || framework_failure_
> +test -d dir || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
> +test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
> +
> +#-------------------------------------------------------------------------------
> +# exercise_rm_rf_root: shell function to test "rm -r '/'"
> +# The caller must provide the FILE to remove as well as any options
> +# which should be passed to 'rm'.
> +# Paranoia mode on:
> +# For the worst case where both rm(1) would fail to refuse to process the "/"
> +# argument (in the cases without the --no-preserve-root option), and
> +# intercepting the unlinkat(1) system call would fail (which actually already
> +# has been proven to work above), limit the damage to the current file system
> +# via the --one-file-system option.
, and the current non root user has write access to /, limit...
> +# Furthermore, run rm(1) in the background and kill that process after
> +# a maximum of 1 second or when the evidence file appears. This also
> +# shortens the testing time.
> +exercise_rm_rf_root ()
s/rf/r/
> +{
> + # Remove the evidence file "x"; verify that.
> + rm -f x || framework_failure_
> + test -f x && framework_failure_
> +
> + local pid
> + if [ "$CU_TEST_SKIP_EXIT" = 1 ]; then
> + # Pass on this variable into 'rm's environment.
> + LD_PRELOAD=./k.so CU_TEST_SKIP_EXIT=1 rm \
> + -rv --one-file-system "$@" > out 2> err & pid=$!
> + else
> + LD_PRELOAD=./k.so rm -rv --one-file-system "$@" > out 2> err & pid=$!
> + fi
> +
> + # Wait for the evidence file to appear, or until the process has terminated.
> + for i in $(seq 10); do
> + test -f x && break
> + kill -0 $pid || break
> + sleep .1
> + done
> +
> + # At this point, rm(1) usually has already terminated. Kill it anyway.
> + kill -9 $pid
So this might race with rm being preempted between the touch() and the exit(),
causing a false failure? It's probably best to not kill if the 'x' file exists,
as it's very unlikely that rm will run as normal in that case.
> +
> + # Get the exit status.
> + wait $pid
> +
> + return $?
> +}
> +
> +# "rm -r /" without --no-preserve-root should output the following
> +# diagnostic error message.
> +cat <<EOD > exp || framework_failure_
> +rm: it is dangerous to operate recursively on '/'
> +rm: use --no-preserve-root to override this failsafe
> +EOD
> +
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -r /" without and with the --preserve-root option.
> +# Expect a non-Zero exit status.
> +for opt in '' '--preserve-root'; do
> + exercise_rm_rf_root $opt '/' \
> + && fail=1
> +
> + # Expect nothing in 'out' and the above error diagnostic in 'err'.
> + # As rm(1) should have skipped the "/" argument, it does not call unlinkat().
> + # Therefore, the evidence file "x" should not exist.
> + compare /dev/null out || fail=1
> + compare exp err || fail=1
> + test -f x && fail=1
> + # Do nothing more if this test failed.
> + test $fail = 1 && { cat out; cat err; Exit $fail; }
> +done
> +
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -r file1 / file2".
> +# Expect a non-Zero exit status representing failure to remove "/",
> +# yet 'file1' and 'file2' should be removed.
> +: > file1 || framework_failure_
> +: > file2 || framework_failure_
> +
> +# Now that we know that 'rm' won't call the unlinkat() system function for "/",
> +# we could probably execute it without the LD_PRELOAD'ed safety net.
> +# Nevertheless, it's still better to use it for this test.
> +# Tell the unlinkat() replacement function to not _exit(0) immediately
> +# by setting the following variable.
> +CU_TEST_SKIP_EXIT=1
> +
> +exercise_rm_rf_root --preserve-root file1 '/' file2 \
> + && fail=1
> +
> +unset CU_TEST_SKIP_EXIT
> +
> +cat <<EOD > out_removed
> +removed 'file1'
> +removed 'file2'
> +EOD
> +
> +# The above error diagnostic should appear in 'err'.
> +# Both 'file1' and 'file2' should be removed. Simply verify that in the
> +# "out" file, as the replacement unlinkat() dummy did not remove them.
> +# Expect the evidence file "x" to exist.
> +compare out_removed out || fail=1
> +compare exp err || fail=1
> +test -f x || fail=1
> +
> +# Do nothing more if this test failed.
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
> +
> +#-------------------------------------------------------------------------------
> +# Exercise various synonyms of "/" including symlinks to it.
> +# The error diagnostic slightly differs from that of the basic "/" case above.
> +cat <<EOD > exp_same || framework_failure_
> +rm: it is dangerous to operate recursively on 'FILE' (same as '/')
> +rm: use --no-preserve-root to override this failsafe
> +EOD
> +
> +# Some combinations have a trailing "." or "..". This triggers another check
> +# in the code first and therefore leads to a different diagnostic. However,
> +# we want to test anyway to protect against future reordering of the checks
> +# in the code.
> +cat <<EOD > exp_dot || framework_failure_
> +rm: refusing to remove '.' or '..' directory: skipping 'FILE'
> +EOD
> +
> +# Prepare a few symlinks to "/".
> +ln -s / rootlink || framework_failure_
> +ln -s rootlink rootlink2 || framework_failure_
> +ln -s /bin/.. rootlink3 || framework_failure_
Are we guaranteed /bin everywhere?
Maybe we should do something like this to get a symlink to root?
ln -sr / rootlink3 || framework_failure_
> +# FIXME: for '///', '////', and more, "rm -r" outputs the error diagnostic
> +# as if the bare "/" was given. For '//' not. Why?!?
Possible explanation above
> +
> +for file in \
> + 'rootlink/' \
> + 'rootlink2/' \
> + 'rootlink3/' \
> + '//' \
> + '//.' \
> + '/./' \
> + '/../' \
> + '/.././' \
> + '/bin/..' ; do
> +
> + exercise_rm_rf_root --preserve-root "$file" \
> + && fail=1
> +
> + sed "s,FILE,$file," exp_same > exp2 || framework_failure_
> + sed "s,FILE,$file," exp_dot > exp_dot2 || framework_failure_
> +
> + # Check against the "refusing to remove '.' or '..'" diagnostic.
> + compare exp_dot2 err \
> + && continue
> +
> + compare /dev/null out || fail=1
> + compare exp2 err || fail=1
> + test -f x && fail=1
> +
> + # Do nothing more if this test failed.
> + test $fail = 1 && { cat out; cat err; Exit $fail; }
> +done
> +
> +#-------------------------------------------------------------------------------
> +# Until now, it was all just fun.
> +# Now exercise the --no-preserve-root option with which rm(1) should enter
> +# the intercepted unlinkat() system call.
> +# As the interception code terminates the process immediately via _exit(0),
> +# the exit status should be 0.
> +exercise_rm_rf_root --no-preserve-root '/' \
> + || fail=1
> +
> +# The 'err' file should not contain the above error diagostic.
s/diagostic/diagnostic/
> +grep "^rm: it is dangerous to operate recursively on '/'" err \
> + && fail=1
> +
> +# Instead, rm(1) should have called the intercepted unlinkat() function,
> +# i.e. the evidence file "x" should exist.
> +test -f x || fail=1
> +
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
No need to the Exit $fail here
> +Exit $fail
thanks!
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Mon, 25 Nov 2013 07:33:02 GMT)
Full text and
rfc822 format available.
Message #188 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/25/2013 02:10 AM, Pádraig Brady wrote:
> On 11/25/2013 12:15 AM, Bernhard Voelker wrote:
>> +cat > k.c <<'EOF' || framework_failure_
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <unistd.h>
>> +
>> +int unlinkat (int dirfd, const char *pathname, int flags)
>> +{
>> + /* Prove that LD_PRELOAD works: create the evidence file "x". */
>> + fclose (fopen ("x", "w"));
>> +
>> + /* Immediately terminate, unless indicated otherwise. */
>> + if (! getenv("CU_TEST_SKIP_EXIT"))
>> + _exit (0);
>> +
>> + /* Pretend success. */
>> + return 0;
>> +}
>> +EOF
>> +exercise_rm_rf_root ()
>
> s/rf/r/
>
>> +{
>> + # Remove the evidence file "x"; verify that.
>> + rm -f x || framework_failure_
>> + test -f x && framework_failure_
>> +
>> + local pid
>> + if [ "$CU_TEST_SKIP_EXIT" = 1 ]; then
>> + # Pass on this variable into 'rm's environment.
>> + LD_PRELOAD=./k.so CU_TEST_SKIP_EXIT=1 rm \
>> + -rv --one-file-system "$@" > out 2> err & pid=$!
>> + else
>> + LD_PRELOAD=./k.so rm -rv --one-file-system "$@" > out 2> err & pid=$!
>> + fi
>> +
>> + # Wait for the evidence file to appear, or until the process has terminated.
>> + for i in $(seq 10); do
>> + test -f x && break
>> + kill -0 $pid || break
>> + sleep .1
>> + done
>> +
>> + # At this point, rm(1) usually has already terminated. Kill it anyway.
>> + kill -9 $pid
>
> So this might race with rm being preempted between the touch() and the exit(),
> causing a false failure? It's probably best to not kill if the 'x' file exists,
> as it's very unlikely that rm will run as normal in that case.
outch, the race is even more likely for the newly added "rm -r file1 / file2"
case where unlinkat() is called twice without the immediate _exit() due to
CU_TEST_SKIP_EXIT=1.
I have to reconsider, but won't have time for that today.
Thanks!
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Tue, 26 Nov 2013 23:09:01 GMT)
Full text and
rfc822 format available.
Message #191 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/25/2013 08:31 AM, Bernhard Voelker wrote:
> On 11/25/2013 02:10 AM, Pádraig Brady wrote:
>> So this might race with rm being preempted between the touch() and the exit(),
>> causing a false failure? It's probably best to not kill if the 'x' file exists,
>> as it's very unlikely that rm will run as normal in that case.
>
> outch, the race is even more likely for the newly added "rm -r file1 / file2"
> case where unlinkat() is called twice without the immediate _exit() due to
> CU_TEST_SKIP_EXIT=1.
>
> I have to reconsider, but won't have time for that today.
Hi Padraig,
I simplified it by using timeout(1), as you already suggested:
if [ "$CU_TEST_SKIP_EXIT" = 1 ]; then
# Pass on this variable into 'rm's environment.
skip_exit='CU_TEST_SKIP_EXIT=1'
fi
timeout --signal=KILL 2 \
env LD_PRELOAD=./k.so $skip_exit \
rm -rv --one-file-system "$@" > out 2> err
I also changed the other things you mentioned.
Finally, I had to pass --interactive=never to the last test to
avoid the "descend into write-protected directory?" prompt.
This is since rm has been executed in the background.
Next try attached.
Have a nice day,
Berny
[tests-add-a-test-for-rm-r-root-v3.patch (text/x-patch, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 27 Nov 2013 03:15:02 GMT)
Full text and
rfc822 format available.
Message #194 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/26/2013 11:08 PM, Bernhard Voelker wrote:
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -r /" without and with the --preserve-root option.
> +# Also exercise the synonyms '///' and '////' which would normally go into
> +# the 'synonyms' test category below; but due to the way gnulib's fts_open()
> +# performs trimming of trailing slashes ("///" -> "/"), the error diagnostic
> +# is the same as for a bare "/", see the ROOT_DEV_INO_WARN macro.
> +# Expect a non-Zero exit status.
> +for opts in '/' '--preserve-root /' '///' '////'; do
This relies on that fts behavior, which I'm a bit wary about coupling to.
I'd be inclined to lump all the synonyms together and just
be less stringent in the error message we compare there.
> + exercise_rm_r_root $opts \
> + && fail=1
So this call has 2s to work or we fail the test.
That seems too short from experience of running many tests
in parallel on slow/loaded systems.
This is at odds of course with trying to get rm not run
for too long. As I said I wouldn't bother with that constraint,
and I can't think how to avoid false failures doing that.
> + # Expect nothing in 'out' and the above error diagnostic in 'err'.
> + # As rm(1) should have skipped the "/" argument, it does not call unlinkat().
> + # Therefore, the evidence file "x" should not exist.
> + compare /dev/null out || fail=1
> + compare exp err || fail=1
> + test -f x && fail=1
> +
> + # Do nothing more if this test failed.
> + test $fail = 1 && { cat out; cat err; Exit $fail; }
> +done
> +
> +#-------------------------------------------------------------------------------
> +# Exercise "rm -r file1 / file2".
> +# Expect a non-Zero exit status representing failure to remove "/",
> +# yet 'file1' and 'file2' should be removed.
> +: > file1 || framework_failure_
> +: > file2 || framework_failure_
> +
> +# Now that we know that 'rm' won't call the unlinkat() system function for "/",
> +# we could probably execute it without the LD_PRELOAD'ed safety net.
> +# Nevertheless, it's still better to use it for this test.
> +# Tell the unlinkat() replacement function to not _exit(0) immediately
> +# by setting the following variable.
> +CU_TEST_SKIP_EXIT=1
> +
> +exercise_rm_r_root --preserve-root file1 '/' file2 \
> + && fail=1
> +
> +unset CU_TEST_SKIP_EXIT
> +
> +cat <<EOD > out_removed
> +removed 'file1'
> +removed 'file2'
> +EOD
> +
> +# The above error diagnostic should appear in 'err'.
> +# Both 'file1' and 'file2' should be removed. Simply verify that in the
> +# "out" file, as the replacement unlinkat() dummy did not remove them.
> +# Expect the evidence file "x" to exist.
> +compare out_removed out || fail=1
> +compare exp err || fail=1
> +test -f x || fail=1
> +
> +# Do nothing more if this test failed.
> +test $fail = 1 && { cat out; cat err; Exit $fail; }
> +
> +#-------------------------------------------------------------------------------
> +# Exercise various synonyms of "/" including symlinks to it.
> +# The error diagnostic slightly differs from that of the basic "/" case above.
> +cat <<EOD > exp_same || framework_failure_
> +rm: it is dangerous to operate recursively on 'FILE' (same as '/')
> +rm: use --no-preserve-root to override this failsafe
> +EOD
> +
> +# Some combinations have a trailing "." or "..". This triggers another check
> +# in the code first and therefore leads to a different diagnostic. However,
> +# we want to test anyway to protect against future reordering of the checks
> +# in the code.
> +cat <<EOD > exp_dot || framework_failure_
> +rm: refusing to remove '.' or '..' directory: skipping 'FILE'
> +EOD
> +
> +# Prepare a few symlinks to "/".
> +ln -s / rootlink || framework_failure_
> +ln -s rootlink rootlink2 || framework_failure_
> +ln -sr / rootlink3 || framework_failure_
> +
> +for file in \
> + 'rootlink/' \
> + 'rootlink2/' \
> + 'rootlink3/' \
> + '//' \
> + '//.' \
> + '/./' \
> + '/../' \
> + '/.././' \
> + '/bin/..' ; do
I'm still worried about /bin being always present.
How about: test -d "$file" || continue
> +
> + exercise_rm_r_root --preserve-root "$file" \
> + && fail=1
> +
> + sed "s,FILE,$file," exp_same > exp2 || framework_failure_
> + sed "s,FILE,$file," exp_dot > exp_dot2 || framework_failure_
> +
> + # Check against the "refusing to remove '.' or '..'" diagnostic.
> + compare exp_dot2 err \
> + && continue
> +
> + compare /dev/null out || fail=1
> + compare exp2 err || fail=1
> + test -f x && fail=1
> +
> + # Do nothing more if this test failed.
> + test $fail = 1 && { cat out; cat err; Exit $fail; }
> +done
> +
> +#-------------------------------------------------------------------------------
> +# Until now, it was all just fun.
> +# Now exercise the --no-preserve-root option with which rm(1) should enter
> +# the intercepted unlinkat() system call.
> +# As the interception code terminates the process immediately via _exit(0),
> +# the exit status should be 0.
> +# Use the option --interactive=never to bypass the following prompt:
> +# "rm: descend into write-protected directory '/'?"
> +exercise_rm_r_root --interactive=never --no-preserve-root '/' \
> + || fail=1
/me doubles checks unlinkat() is the only function that should be called,
and the previous checks guarantee this... OK :)
thanks!
Pádraig.
p.s. something that might be worth looking at in future,
would be to use runcon to restrict the process using selinux
if in selinux enforcing mode.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Thu, 28 Nov 2013 00:15:02 GMT)
Full text and
rfc822 format available.
Message #197 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/27/2013 04:14 AM, Pádraig Brady wrote:
> On 11/26/2013 11:08 PM, Bernhard Voelker wrote:
>> +#-------------------------------------------------------------------------------
>> +# Exercise "rm -r /" without and with the --preserve-root option.
>> +# Also exercise the synonyms '///' and '////' which would normally go into
>> +# the 'synonyms' test category below; but due to the way gnulib's fts_open()
>> +# performs trimming of trailing slashes ("///" -> "/"), the error diagnostic
>> +# is the same as for a bare "/", see the ROOT_DEV_INO_WARN macro.
>> +# Expect a non-Zero exit status.
>> +for opts in '/' '--preserve-root /' '///' '////'; do
>
> This relies on that fts behavior, which I'm a bit wary about coupling to.
> I'd be inclined to lump all the synonyms together and just
> be less stringent in the error message we compare there.
Good point, done.
While at it, I separated the "refusing to remove '.' or '..'" cases
from the "it is dangerous to operate recursively on '/'" cases.
>> + exercise_rm_r_root $opts \
>> + && fail=1
>
> So this call has 2s to work or we fail the test.
> That seems too short from experience of running many tests
> in parallel on slow/loaded systems.
> This is at odds of course with trying to get rm not run
> for too long. As I said I wouldn't bother with that constraint,
> and I can't think how to avoid false failures doing that.
The whole test with 21 test cases lasts .5 - .8 seconds here on
a 3-4 year-old PC - measured via "time make tests/rm/r-root.log".
I'd vote for being conservative for now, i.e. stay with "timeout 2",
and increase the timeout if we see false positives.
>> +for file in \
>> + 'rootlink/' \
>> + 'rootlink2/' \
>> + 'rootlink3/' \
>> + '//' \
>> + '//.' \
>> + '/./' \
>> + '/../' \
>> + '/.././' \
>> + '/bin/..' ; do
>
> I'm still worried about /bin being always present.
> How about: test -d "$file" || continue
Good idea, added.
That reminded me that on some systems /usr/bin and /bin
may be the same, and switched to testing '/etc/..'.
> /me doubles checks unlinkat() is the only function that should be called,
> and the previous checks guarantee this... OK :)
Not quite: it only checked for a directory.
I added a check for a file.
> p.s. something that might be worth looking at in future,
> would be to use runcon to restrict the process using selinux
> if in selinux enforcing mode.
I've never used selinux (knowingly), so I don't know much about
it. But feel free to add it. ;-)
Thanks for the review!
Have a nice day,
Berny
[tests-rm-r-root.patch (text/x-patch, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 29 Nov 2013 01:44:02 GMT)
Full text and
rfc822 format available.
Message #200 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/28/2013 12:14 AM, Bernhard Voelker wrote:
> On 11/27/2013 04:14 AM, Pádraig Brady wrote:
>> On 11/26/2013 11:08 PM, Bernhard Voelker wrote:
>>> +#-------------------------------------------------------------------------------
>>> +# Exercise "rm -r /" without and with the --preserve-root option.
>>> +# Also exercise the synonyms '///' and '////' which would normally go into
>>> +# the 'synonyms' test category below; but due to the way gnulib's fts_open()
>>> +# performs trimming of trailing slashes ("///" -> "/"), the error diagnostic
>>> +# is the same as for a bare "/", see the ROOT_DEV_INO_WARN macro.
>>> +# Expect a non-Zero exit status.
>>> +for opts in '/' '--preserve-root /' '///' '////'; do
>>
>> This relies on that fts behavior, which I'm a bit wary about coupling to.
>> I'd be inclined to lump all the synonyms together and just
>> be less stringent in the error message we compare there.
>
> Good point, done.
> While at it, I separated the "refusing to remove '.' or '..'" cases
> from the "it is dangerous to operate recursively on '/'" cases.
>
>>> + exercise_rm_r_root $opts \
>>> + && fail=1
>>
>> So this call has 2s to work or we fail the test.
>> That seems too short from experience of running many tests
>> in parallel on slow/loaded systems.
>> This is at odds of course with trying to get rm not run
>> for too long. As I said I wouldn't bother with that constraint,
>> and I can't think how to avoid false failures doing that.
>
> The whole test with 21 test cases lasts .5 - .8 seconds here on
> a 3-4 year-old PC - measured via "time make tests/rm/r-root.log".
> I'd vote for being conservative for now, i.e. stay with "timeout 2",
> and increase the timeout if we see false positives.
>
>>> +for file in \
>>> + 'rootlink/' \
>>> + 'rootlink2/' \
>>> + 'rootlink3/' \
>>> + '//' \
>>> + '//.' \
>>> + '/./' \
>>> + '/../' \
>>> + '/.././' \
>>> + '/bin/..' ; do
>>
>> I'm still worried about /bin being always present.
>> How about: test -d "$file" || continue
>
> Good idea, added.
> That reminded me that on some systems /usr/bin and /bin
> may be the same, and switched to testing '/etc/..'.
>
>> /me doubles checks unlinkat() is the only function that should be called,
>> and the previous checks guarantee this... OK :)
>
> Not quite: it only checked for a directory.
> I added a check for a file.
>
>> p.s. something that might be worth looking at in future,
>> would be to use runcon to restrict the process using selinux
>> if in selinux enforcing mode.
>
> I've never used selinux (knowingly), so I don't know much about
> it. But feel free to add it. ;-)
>
> Thanks for the review!
>
> Have a nice day,
> Berny
>
It all looks good apart from the `timeout 2` race.
Unlikely but not something we could release on a bazillion
build hosts doing `make check`.
So as a compromise how about disabling the test by default by adding:
expensive_
I.E. it would only be developers that would be running this,
who might be less likely to hit the issue, or at least
more likely to recognize it and not report false positives.
thanks!
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 29 Nov 2013 07:12:01 GMT)
Full text and
rfc822 format available.
Message #203 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/29/2013 02:43 AM, Pádraig Brady wrote:
> It all looks good [...]
Thanks for the review(s)!
> [...] apart from the `timeout 2` race.
> Unlikely but not something we could release on a bazillion
> build hosts doing `make check`.
> So as a compromise how about disabling the test by default by adding:
>
> expensive_
>
> I.E. it would only be developers that would be running this,
> who might be less likely to hit the issue, or at least
> more likely to recognize it and not report false positives.
I agree.
I found a precedent case in tests/misc/sort-spinlock-abuse.sh.
I'll add the following before pushing.
Thanks again & have a nice day,
Berny
diff --git a/tests/rm/r-root.sh b/tests/rm/r-root.sh
index aa52bc4..9656274 100755
--- a/tests/rm/r-root.sh
+++ b/tests/rm/r-root.sh
@@ -32,6 +32,14 @@ skip_if_root_
# LD_PRELOAD environment variable. This requires shared libraries to work.
require_gcc_shared_
+# This isn't terribly expensive, but it must not be run under heavy load.
+# The reason is the conservative 'timeout' setting below to limit possible
+# damage in the worst case which yields a race under heavy load.
+# Marking this test as "expensive" therefore is a compromise, i.e., adding
+# this test to the list ensures it still gets _some_ (albeit minimal)
+# coverage while not causing false-positive failures in day to day runs.
+expensive_
+
cat > k.c <<'EOF' || framework_failure_
#include <stdio.h>
#include <stdlib.h>
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Fri, 29 Nov 2013 19:58:01 GMT)
Full text and
rfc822 format available.
Message #206 received at 15926 <at> debbugs.gnu.org (full text, mbox):
Eric Blake wrote:
> Bernhard Voelker wrote:
> > Bob Proulx wrote:
> > +# Exercise "rm -rf /" without the --preserve-root and --no-preserve-root option.
> > +# Expect a non-Zero exist status.
> > +exercise_rm_rf_root \
> > + && fail=1
>
> Maybe you should favor 'rm -r /' rather than 'rm -rf /'. That way, even
> if all the failsafes are bypassed, you at least have better logging of
> what started to go wrong rather than silence.
I still think this is a very scary test and isn't worth the return on
investment. It is the kind of thing that makes me think I could never
recommend building coreutils anywhere but in a throwaway chroot.
Because the risk of a failure is just so very extremely high. That
would be a shame.
Bob
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 30 Nov 2013 01:49:01 GMT)
Full text and
rfc822 format available.
Message #209 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/29/2013 07:57 PM, Bob Proulx wrote:
> Eric Blake wrote:
>> Bernhard Voelker wrote:
>>> Bob Proulx wrote:
>>> +# Exercise "rm -rf /" without the --preserve-root and --no-preserve-root option.
>>> +# Expect a non-Zero exist status.
>>> +exercise_rm_rf_root \
>>> + && fail=1
>>
>> Maybe you should favor 'rm -r /' rather than 'rm -rf /'. That way, even
>> if all the failsafes are bypassed, you at least have better logging of
>> what started to go wrong rather than silence.
>
> I still think this is a very scary test and isn't worth the return on
> investment. It is the kind of thing that makes me think I could never
> recommend building coreutils anywhere but in a throwaway chroot.
> Because the risk of a failure is just so very extremely high. That
> would be a shame.
To summarize, it,
only runs with: make EXPENSIVE=yes check,
only runs as non root,
ensures file & dir removal bypass work in a safe context first
Do you still think it's too dangerous?
Pádraig.
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 30 Nov 2013 17:58:02 GMT)
Full text and
rfc822 format available.
Message #212 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/29/2013 06:48 PM, Pádraig Brady wrote:
> To summarize, it,
> only runs with: make EXPENSIVE=yes check,
> only runs as non root,
> ensures file & dir removal bypass work in a safe context first
>
> Do you still think it's too dangerous?
I think we've done a great job at writing a very robust test that exits
early if not all preconditions of safety can be met (the most important
of which is that using rm under LD_PRELOAD on a safe file leaves the
file untouched, proving that we correctly avoided the right system call)
- I see no issue with including the test by default, nor any reason to
scare users into only running the test in a chroot jail. I actually
think the timeout to limit to 2 seconds is a bit of overkill, but it's
fine to be over-conservative and leave it in.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 30 Nov 2013 21:34:02 GMT)
Full text and
rfc822 format available.
Message #215 received at 15926 <at> debbugs.gnu.org (full text, mbox):
Pádraig Brady wrote:
> Bob Proulx wrote:
> > I still think this is a very scary test and isn't worth the return on
> > investment. It is the kind of thing that makes me think I could never
> > recommend building coreutils anywhere but in a throwaway chroot.
> > Because the risk of a failure is just so very extremely high. That
> > would be a shame.
>
> To summarize, it,
> only runs with: make EXPENSIVE=yes check,
> only runs as non root,
> ensures file & dir removal bypass work in a safe context first
Plus I see a timeout too. The timeout is a good safety net. However
it does create some possibility for a false positive fail if the
system is slow. But a possible FP is a minor thing.
I wasn't quite convinced that this is skipped if LD_PRELOAD isn't
supported by the system. However I don't doubt it. I just wasn't
able to trace through the logic by inspection quickly enough.
> Do you still think it's too dangerous?
I can't say that it isn't still scary. There is an intentional
execution of an 'rm -r /'! But looking at it I think you guys have
done a lot of work to make it as safe as possible. I still think that
was way too much investment for the potential return. But having done
it I can't say not to include it. Since it only runs as non-root then
an accident there really isn't any worse than any other rm accident
that might happen. But that is today in the current incarnation.
But for example what happens if the preferred unlink command ever
changes from unlinkat() to something new about ten years in the
future? Can't say it won't change since we have already seen it
change. If the code is changed in the future and the tests run then
what prevents the pitfall at that point in the future?
I really don't want to be a rainy day but I worry about these things.
Bob
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Sat, 30 Nov 2013 21:49:02 GMT)
Full text and
rfc822 format available.
Message #218 received at 15926 <at> debbugs.gnu.org (full text, mbox):
On 11/30/2013 10:33 PM, Bob Proulx wrote:
> [...] I still think that
> was way too much investment for the potential return.
I wouldn't say that: 20 of the 21 runs of "rm -r /" prove that
'rm' skips the '/' argument and various synonyms, as required
by POSIX (and common sense). I think it's important to ensure
that this skipping works.
> But for example what happens if the preferred unlink command ever
> changes from unlinkat() to something new about ten years in the
> future?
At the start, the test proves that intercepting unlinkat() via
LD_PRELOAD works and that the arguments (once for a file, once for
a directory) still exist afterward.
> I really don't want to be a rainy day but I worry about these things.
That's basically a good thing. This is really a delicate test,
so I'd like to encourage as much people as possible to have a
look at it. Please feel free to double- (or more) read the code.
Have a nice day,
Berny
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Mon, 02 Dec 2013 15:49:01 GMT)
Full text and
rfc822 format available.
Message #221 received at 15926 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 11/30/2013 02:33 PM, Bob Proulx wrote:
> But for example what happens if the preferred unlink command ever
> changes from unlinkat() to something new about ten years in the
> future? Can't say it won't change since we have already seen it
> change.
Indeed, we've already moved from unlink()/rmdir() to unlinkat() as our
preferred syscall(s).
But that's part of the sanity checks of the test - if the LD_PRELOAD
test runs, and the file is deleted, then the code base has switched away
from using unlinkat() as its deletion mechanism, and the test will be
skipped until someone updates it to override the new preferred syscall
instead. The only way we attempt 'rm -r /' is if we FIRST prove that we
have safely prevented 'rm -r file dir' from deleting sub-trees in the
(safe) current working directory, so that we know that we are also
preventing any attempt to actually remove / and only end up testing the
other effects (such as the appropriate POSIX behavior of refusing to
remove / by default).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[signature.asc (application/pgp-signature, attachment)]
Information forwarded
to
bug-coreutils <at> gnu.org:
bug#15926; Package
coreutils.
(Wed, 10 Oct 2018 16:30:02 GMT)
Full text and
rfc822 format available.
Message #224 received at 15926 <at> debbugs.gnu.org (full text, mbox):
close 15926
stop
(triaging old bugs)
The original topic of using remove(2) call in rm(1)
was decided to be undesirable.
Starting at around the 33rd message [1] the thread diverges into bugs in
"rm -rf ." and similar problems (which are resolved by the 221st
message [2].
[1] https://bugs.gnu.org/15926#33
[2] https://bugs.gnu.org/15926#221
I'm therefore closing this item (it is already marked as "not a bug",
relating to the original request).
We typically write "discussion can continue by replying to this thread",
but in this case I think it should not be encouraged.
If there are further issues with "rm", please do start a new thread
(if it's a bug - to bug-coreutils <at> gnu.org ; if a feature request - to
coreutils <at> gnu.org ).
regards,
- assaf
bug closed, send any further explanations to
15926 <at> debbugs.gnu.org and Linda Walsh <coreutils <at> tlinx.org>
Request was from
Assaf Gordon <assafgordon <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Wed, 10 Oct 2018 16:30:03 GMT)
Full text and
rfc822 format available.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Thu, 08 Nov 2018 12:24:06 GMT)
Full text and
rfc822 format available.
This bug report was last modified 5 years and 191 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.