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

[Pádraig Brady]

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.