| Message ID | 20220706143822.30182-6-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath: fixes for sysfs accessors | expand |
On Wed, Jul 06, 2022 at 04:38:13PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > udev_device_get_syspath() may return NULL; check for it, and check > for pathname overflow. Disallow a zero buffer length. The fstat() > calls were superfluous, as a read() or write() on the fd would > return the respective error codes depending on file type or permissions, > the extra system call and code complexity adds no value. > > Log levels should be moderate in sysfs.c, because it depends > on the caller whether errors getting/setting certain attributes are > fatal. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/sysfs.c | 94 ++++++++++++++++++-------------------------- > 1 file changed, 39 insertions(+), 55 deletions(-) > > diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c > index 4db911c..1f0f207 100644 > --- a/libmultipath/sysfs.c > +++ b/libmultipath/sysfs.c > @@ -47,46 +47,38 @@ > static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name, > char *value, size_t value_len, bool binary) > { > + const char *syspath; > char devpath[PATH_SIZE]; > - struct stat statbuf; > int fd; > ssize_t size = -1; > > - if (!dev || !attr_name || !value) > - return 0; > + if (!dev || !attr_name || !value || !value_len) { > + condlog(1, "%s: invalid parameters", __func__); > + return -EINVAL; > + } > > - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), > - attr_name); > + syspath = udev_device_get_syspath(dev); > + if (!syspath) { > + condlog(3, "%s: invalid udevice", __func__); > + return -EINVAL; > + } > + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { > + condlog(3, "%s: devpath overflow", __func__); > + return -EOVERFLOW; > + } > condlog(4, "open '%s'", devpath); > /* read attribute value */ > fd = open(devpath, O_RDONLY); > if (fd < 0) { > - condlog(4, "attribute '%s' can not be opened: %s", > - devpath, strerror(errno)); > + condlog(3, "%s: attribute '%s' can not be opened: %s", > + __func__, devpath, strerror(errno)); > return -errno; > } > - if (fstat(fd, &statbuf) < 0) { > - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); > - close(fd); > - return -ENXIO; > - } > - /* skip directories */ > - if (S_ISDIR(statbuf.st_mode)) { > - condlog(4, "%s is a directory", devpath); > - close(fd); > - return -EISDIR; > - } > - /* skip non-writeable files */ > - if ((statbuf.st_mode & S_IRUSR) == 0) { > - condlog(4, "%s is not readable", devpath); > - close(fd); > - return -EPERM; > - } > - > size = read(fd, value, value_len); > if (size < 0) { > - condlog(4, "read from %s failed: %s", devpath, strerror(errno)); > size = -errno; > + condlog(3, "%s: read from %s failed: %s", __func__, devpath, > + strerror(errno)); > if (!binary) > value[0] = '\0'; > } else if (!binary && size == (ssize_t)value_len) { > @@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name, > ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name, > const char * value, size_t value_len) > { > + const char *syspath; > char devpath[PATH_SIZE]; > - struct stat statbuf; > int fd; > ssize_t size = -1; > > - if (!dev || !attr_name || !value || !value_len) > - return 0; > + if (!dev || !attr_name || !value || !value_len) { > + condlog(1, "%s: invalid parameters", __func__); > + return -EINVAL; > + } > + > + syspath = udev_device_get_syspath(dev); > + if (!syspath) { > + condlog(3, "%s: invalid udevice", __func__); > + return -EINVAL; > + } > + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { > + condlog(3, "%s: devpath overflow", __func__); > + return -EOVERFLOW; > + } > > - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), > - attr_name); > condlog(4, "open '%s'", devpath); > /* write attribute value */ > fd = open(devpath, O_WRONLY); > if (fd < 0) { > - condlog(4, "attribute '%s' can not be opened: %s", > - devpath, strerror(errno)); > + condlog(2, "%s: attribute '%s' can not be opened: %s", > + __func__, devpath, strerror(errno)); You log at loglevel 2 here, but at 3 for an open() failure in __sysfs_attr_get_value(). However I notice that this gets resolved in PATCH 8/14, so it's no big deal. -Ben > return -errno; > } > - if (fstat(fd, &statbuf) != 0) { > - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); > - close(fd); > - return -errno; > - } > - > - /* skip directories */ > - if (S_ISDIR(statbuf.st_mode)) { > - condlog(4, "%s is a directory", devpath); > - close(fd); > - return -EISDIR; > - } > - > - /* skip non-writeable files */ > - if ((statbuf.st_mode & S_IWUSR) == 0) { > - condlog(4, "%s is not writeable", devpath); > - close(fd); > - return -EPERM; > - } > > size = write(fd, value, value_len); > if (size < 0) { > - condlog(4, "write to %s failed: %s", devpath, strerror(errno)); > size = -errno; > + condlog(3, "%s: write to %s failed: %s", __func__, > + devpath, strerror(errno)); > } else if (size < (ssize_t)value_len) { > - condlog(4, "tried to write %ld to %s. Wrote %ld", > - (long)value_len, devpath, (long)size); > + condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes", > + __func__, value_len, devpath, size); > size = 0; > } > > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c index 4db911c..1f0f207 100644 --- a/libmultipath/sysfs.c +++ b/libmultipath/sysfs.c @@ -47,46 +47,38 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name, char *value, size_t value_len, bool binary) { + const char *syspath; char devpath[PATH_SIZE]; - struct stat statbuf; int fd; ssize_t size = -1; - if (!dev || !attr_name || !value) - return 0; + if (!dev || !attr_name || !value || !value_len) { + condlog(1, "%s: invalid parameters", __func__); + return -EINVAL; + } - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), - attr_name); + syspath = udev_device_get_syspath(dev); + if (!syspath) { + condlog(3, "%s: invalid udevice", __func__); + return -EINVAL; + } + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { + condlog(3, "%s: devpath overflow", __func__); + return -EOVERFLOW; + } condlog(4, "open '%s'", devpath); /* read attribute value */ fd = open(devpath, O_RDONLY); if (fd < 0) { - condlog(4, "attribute '%s' can not be opened: %s", - devpath, strerror(errno)); + condlog(3, "%s: attribute '%s' can not be opened: %s", + __func__, devpath, strerror(errno)); return -errno; } - if (fstat(fd, &statbuf) < 0) { - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); - close(fd); - return -ENXIO; - } - /* skip directories */ - if (S_ISDIR(statbuf.st_mode)) { - condlog(4, "%s is a directory", devpath); - close(fd); - return -EISDIR; - } - /* skip non-writeable files */ - if ((statbuf.st_mode & S_IRUSR) == 0) { - condlog(4, "%s is not readable", devpath); - close(fd); - return -EPERM; - } - size = read(fd, value, value_len); if (size < 0) { - condlog(4, "read from %s failed: %s", devpath, strerror(errno)); size = -errno; + condlog(3, "%s: read from %s failed: %s", __func__, devpath, + strerror(errno)); if (!binary) value[0] = '\0'; } else if (!binary && size == (ssize_t)value_len) { @@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name, ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name, const char * value, size_t value_len) { + const char *syspath; char devpath[PATH_SIZE]; - struct stat statbuf; int fd; ssize_t size = -1; - if (!dev || !attr_name || !value || !value_len) - return 0; + if (!dev || !attr_name || !value || !value_len) { + condlog(1, "%s: invalid parameters", __func__); + return -EINVAL; + } + + syspath = udev_device_get_syspath(dev); + if (!syspath) { + condlog(3, "%s: invalid udevice", __func__); + return -EINVAL; + } + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { + condlog(3, "%s: devpath overflow", __func__); + return -EOVERFLOW; + } - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), - attr_name); condlog(4, "open '%s'", devpath); /* write attribute value */ fd = open(devpath, O_WRONLY); if (fd < 0) { - condlog(4, "attribute '%s' can not be opened: %s", - devpath, strerror(errno)); + condlog(2, "%s: attribute '%s' can not be opened: %s", + __func__, devpath, strerror(errno)); return -errno; } - if (fstat(fd, &statbuf) != 0) { - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); - close(fd); - return -errno; - } - - /* skip directories */ - if (S_ISDIR(statbuf.st_mode)) { - condlog(4, "%s is a directory", devpath); - close(fd); - return -EISDIR; - } - - /* skip non-writeable files */ - if ((statbuf.st_mode & S_IWUSR) == 0) { - condlog(4, "%s is not writeable", devpath); - close(fd); - return -EPERM; - } size = write(fd, value, value_len); if (size < 0) { - condlog(4, "write to %s failed: %s", devpath, strerror(errno)); size = -errno; + condlog(3, "%s: write to %s failed: %s", __func__, + devpath, strerror(errno)); } else if (size < (ssize_t)value_len) { - condlog(4, "tried to write %ld to %s. Wrote %ld", - (long)value_len, devpath, (long)size); + condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes", + __func__, value_len, devpath, size); size = 0; }