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@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