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