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
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
