[Toybox] rm -rf doesn't chmod high enough

[Rob Landley]

On 09/23/14 10:17, Isaac Dunham wrote:
> On Mon, Sep 22, 2014 at 08:19:21AM -0500, Rob Landley wrote:
>> On 09/20/14 08:22, Felix Janda wrote:
>>> Isaac Dunham wrote:
>>> [..]
>>>> A trickier bug is that rm -r dir will skip dir, saying 
>>>> "rm: dir: is a directory"
>>>> I guess the fix involves rm -r calling rmdir instead of unlink.
>>>
>>> unlinkat() also removes directories when given AT_REMOVEDIR.
>>> The following patch makes rm pass the tests and it still asks
>>> me when I try to remove a readonly dir.
>>>
>>> -Felix
>>>
>>> diff -r 434c4ae19f05 toys/posix/rm.c
>>> --- a/toys/posix/rm.c	Thu Sep 18 18:07:58 2014 -0500
>>> +++ b/toys/posix/rm.c	Sat Sep 20 15:19:09 2014 +0200
>>> @@ -50,7 +50,6 @@
>>>      // Handle chmod 000 directories when -f
>>>      if (faccessat(fd, try->name, R_OK, AT_SYMLINK_NOFOLLOW)) {
>>>        if (toys.optflags & FLAG_f) wfchmodat(fd, try->name, 0700);
>>> -      else goto skip;
>>>      }
>>>      if (!try->again) return DIRTREE_COMEAGAIN;
>>>      using = AT_REMOVEDIR;
>>
>> I'm confused: rm is currently passing for me? "scripts/test.sh rm"
>> passes, and "mkdir blah; rm -r blah" also works for me...
>>
>> How do I reproduce the failure? (Try on fedora maybe?)
> 
> Both "rm -r" tests fail on Alpine Linux (musl) for me.
> rm -rf passes.

Ah. That I can reproduce.

So the problem turns out to be that faccessat() in musl returns error
EINVAL whenever you feed AT_SYMLINK_NOFOLLOW into the flags field, which
the man page says you're allowed to do.

I'm pretty sure this is a musl bug, not a toybox bug. The code as-is
works on uClibc and glibc for a _reason_.

> And the mount test hangs in an infinite loop for me.
> Per strace, it's not doing any syscalls.

There's a mount test? Oh right, contribution I still haven't reviewed.

the reason i hadn't written a mount test yet was the "running tests as
root" issue I mentioned earlier, I need some kind of test harness using
qemu or similar. (It's on the todo list.)

Almost certainly the failure is related to the /etc/fstab "user" logic,
which I still haven't gotten around to properly testing. I tested the
"mount as root" paths, but just enough to get aboriginal working so I
could cut a release. I don't think I've ever properly tested remount yet...

> Workaround-hit it with a hammer:
> while killall mount; do sleep 1; done
> 
> (I'll be poking at that, if I get the time...)

I've been writing a new ping implementation. (I've gotten a couple ping
contributions but they always do ping and ping6 as two separate
implementations, tend to suck in 4-clause BSD code, and on top of the
normal cleanup it's just faster to do a new one. Not _fast_, just faster. :)

> Thanks,
> Isaac Dunham

Rob

 1411618785.0