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

[Pádraig Brady]

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.