[systemd-devel] [PATCH 2/4] Allow systemd-tmpfiles to set the file/directory attributes

[Goffredo Baroncelli]

Hi Lennart,

On 2015-04-08 20:36, Lennart Poettering wrote:
>> +
>> > +static int path_set_attrib(Item *item, const char *path) {
>> > +        _cleanup_close_ int fd = -1;
>> > +        int r;
>> > +        unsigned f;
>> > +        struct stat st;
>> > +
>> > +        /* do nothing */
>> > +        if (item->attrib_mask == 0 || !item->attrib_set)
>> > +                return 0;
>> > +        /*
>> > +         * It is OK to ignore an lstat() error, because the error
>> > +         * will be catch by the open() below anyway
>> > +         */
>> > +        if (lstat(path, &st) == 0 &&
>> > +            !S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
>> > +                return 0;
>> > +        }
> Why do we need this check? What happens if we apply this onto
> a device node, or socket, or whatever? 
> 
> I really don#t like this seperate lstat() + open(). If it all it
> should be open() + fstat(), to avoid TTOCTOU issues...

I am checking your changes; you modified the code above in :

        fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
        if (fd < 0)
                return log_error_errno(errno, "Cannot open '%s': %m", path);

        if (fstat(fd, &st) < 0)
                return log_error_errno(errno, "Cannot stat '%s': %m", path);

        /* Issuing the file attribute ioctls on device nodes is not
         * safe, as that will be delivered to the drivers, not the
         * file system containing the device node. */
        if (!S_ISREG(st.st_mode) && !S_ISDIR(st.st_mode)) {
                log_error("Setting file flags is only supported on regular files and directories, cannot set on '%s'.", path);
                return -EINVAL;
        }


However the original code catch also the case where the file is a soft-link.
The same check is performed also by chattr(1); I suggest to leave the original
behavior, changing

        fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC);
in
	fd = open(path, O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_NOFOLLOW);

and checking if the errno is ELOOP. In this case a further check is performed to 
verify if the file is a link or the error is due to a too many symbolic link.
Then an appropriate message error is printed.

What do you think ?

Goffredo

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5