bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#15926: RFE: unlink command already uses 'unlink' call; make 'rm' use


From: Pádraig Brady
Subject: bug#15926: RFE: unlink command already uses 'unlink' call; make 'rm' use 'remove' call
Date: Mon, 25 Nov 2013 01:10:27 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2

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 <address@hidden>
>>> 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.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]