[systemd-devel] [PATCH] udev: fix TOCTOU when creating a directory

[Ronny Chevalier]

2014-11-16 19:39 GMT+01:00 David Herrmann <dh.herrmann at gmail.com>:
> Hi
>
> On Sun, Nov 16, 2014 at 7:34 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>> Hi
>>
>> On Sun, Nov 9, 2014 at 3:42 PM, Ronny Chevalier
>> <chevalier.ronny at gmail.com> wrote:
>>> CID#979416
>>> ---
>>>  src/udev/collect/collect.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/udev/collect/collect.c b/src/udev/collect/collect.c
>>> index dc849bd..6cb10fe 100644
>>> --- a/src/udev/collect/collect.c
>>> +++ b/src/udev/collect/collect.c
>>> @@ -86,12 +86,13 @@ static void usage(void)
>>>   */
>>>  static int prepare(char *dir, char *filename)
>>>  {
>>> -        struct stat statbuf;
>>>          char buf[512];
>>>          int fd;
>>> +        int r;
>>>
>>> -        if (stat(dir, &statbuf) < 0)
>>> -                mkdir(dir, 0700);
>>> +        r = mkdir(dir, 0700);
>>> +        if (r < 0 && errno != EEXIST)
>>> +                return -errno;
>>>
>>>          snprintf(buf, sizeof(buf), "%s/%s", dir, filename);
>>
>> So the race you describe is if the directory is removed after we
>> stat() it, but before we use it somewhere down in the code. Applying
>> your patch avoids the stat(), but it still fails if the dir is removed
>> after your mkdir(). So this doesn't fix anything, does it?
Right

>>
>> The code is definitely nicer than before, so I guess I'll apply it,
>> anyway. But I don't see how it would fix anything, but silence a
>> coverity warning. Or am I missing something? Feel free to prove me
>> wrong ;)
>
> One more addition: your code avoids an additional syscall, so yeah,
> it's nicer. So I applied it now!
It was the purpose of this patch. The stat() call was useless and we
did not check if the mkdir() succeeded.
But you are right, it does not fix any TOCTOU, I think I named it this
way because of the coverity report.

So you can change the commit message if you want :)

>
> Thanks
> David