On Thu, Nov 07, 2019 at 09:27:41AM +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> snprintf() returns int, but the size argument "n" is size_t.
> Use safe_snprintf() to avoid -Wsign-compare warnings. At the same
> time, improve these macros to check for errors in snprintf(), too.
>
> Note: there are more uses of snprintf() in our code that may
> need review, too. For now, I'm fixing only those that were causing
> warnings.
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> kpartx/devmapper.c | 3 ++-
> kpartx/kpartx.h | 11 ++++++++++-
> libmultipath/foreign/nvme.c | 6 ++----
> libmultipath/sysfs.c | 7 +++----
> libmultipath/util.c | 3 +--
> libmultipath/util.h | 11 +++++++++--
> libmultipath/wwids.c | 3 +--
> multipath/main.c | 3 +--
> 8 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
> index 3aa4988d..b100b5ef 100644
> --- a/kpartx/devmapper.c
> +++ b/kpartx/devmapper.c
> @@ -10,6 +10,7 @@
> #include <errno.h>
> #include <sys/sysmacros.h>
> #include "devmapper.h"
> +#include "kpartx.h"
>
> #define _UUID_PREFIX "part"
> #define UUID_PREFIX _UUID_PREFIX "%d-"
> @@ -107,7 +108,7 @@ strip_slash (char * device)
> static int format_partname(char *buf, size_t bufsiz,
> const char *mapname, const char *delim, int part)
> {
> - if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >= bufsiz)
> + if (safe_snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part))
> return 0;
> strip_slash(buf);
> return 1;
> diff --git a/kpartx/kpartx.h b/kpartx/kpartx.h
> index 52920e43..8a2db046 100644
> --- a/kpartx/kpartx.h
> +++ b/kpartx/kpartx.h
> @@ -16,8 +16,17 @@
> #define likely(x) __builtin_expect(!!(x), 1)
> #define unlikely(x) __builtin_expect(!!(x), 0)
>
> +#define safe_snprintf(var, size, format, args...) \
> + ({ \
> + size_t __size = size; \
> + int __ret; \
> + \
> + __ret = snprintf(var, __size, format, ##args); \
> + __ret < 0 || (size_t)__ret >= __size; \
> + })
> +
> #define safe_sprintf(var, format, args...) \
> - snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
> + safe_snprintf(var, sizeof(var), format, ##args)
>
> #ifndef BLKSSZGET
> #define BLKSSZGET _IO(0x12,104) /* get block device sector size */
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index e8ca516c..09cdddf0 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -591,8 +591,7 @@ static void test_ana_support(struct nvme_map *map, struct udev_device *ctl)
> return;
>
> dev_t = udev_device_get_sysattr_value(ctl, "dev");
> - if (snprintf(sys_path, sizeof(sys_path), "/dev/char/%s", dev_t)
> - >= sizeof(sys_path))
> + if (safe_sprintf(sys_path, "/dev/char/%s", dev_t))
> return;
>
> fd = open(sys_path, O_RDONLY);
> @@ -663,8 +662,7 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
> char *fn = di[i]->d_name;
> struct udev_device *ctrl, *udev;
>
> - if (snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s", fn)
> - >= sizeof(pathbuf) - n)
> + if (safe_snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s", fn))
> continue;
> if (realpath(pathbuf, realbuf) == NULL) {
> condlog(3, "%s: %s: realpath: %s", __func__, THIS,
> diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> index 923b529b..62ec2ed7 100644
> --- a/libmultipath/sysfs.c
> +++ b/libmultipath/sysfs.c
> @@ -306,7 +306,7 @@ bool sysfs_is_multipathed(const struct path *pp)
> n = snprintf(pathbuf, sizeof(pathbuf), "/sys/block/%s/holders",
> pp->dev);
>
> - if (n >= sizeof(pathbuf)) {
> + if (n < 0 || (size_t)n >= sizeof(pathbuf)) {
> condlog(1, "%s: pathname overflow", __func__);
> return false;
> }
> @@ -327,9 +327,8 @@ bool sysfs_is_multipathed(const struct path *pp)
> int nr;
> char uuid[6];
>
> - if (snprintf(pathbuf + n, sizeof(pathbuf) - n,
> - "/%s/dm/uuid", di[i]->d_name)
> - >= sizeof(pathbuf) - n)
> + if (safe_snprintf(pathbuf + n, sizeof(pathbuf) - n,
> + "/%s/dm/uuid", di[i]->d_name))
> continue;
>
> fd = open(pathbuf, O_RDONLY);
> diff --git a/libmultipath/util.c b/libmultipath/util.c
> index ccc0de29..51c38c87 100644
> --- a/libmultipath/util.c
> +++ b/libmultipath/util.c
> @@ -212,8 +212,7 @@ int devt2devname(char *devname, int devname_len, char *devt)
> continue;
>
> if ((major == tmpmaj) && (minor == tmpmin)) {
> - if (snprintf(block_path, sizeof(block_path),
> - "/sys/block/%s", dev) >= sizeof(block_path)) {
> + if (safe_sprintf(block_path, "/sys/block/%s", dev)) {
> condlog(0, "device name %s is too long", dev);
> fclose(fd);
> return 1;
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index 913ab7c2..56bd78c7 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -29,9 +29,16 @@ void set_max_fds(rlim_t max_fds);
> #define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))
>
> #define safe_sprintf(var, format, args...) \
> - snprintf(var, sizeof(var), format, ##args) >= sizeof(var)
> + safe_snprintf(var, sizeof(var), format, ##args)
> +
> #define safe_snprintf(var, size, format, args...) \
> - snprintf(var, size, format, ##args) >= size
> + ({ \
> + size_t __size = size; \
> + int __ret; \
> + \
> + __ret = snprintf(var, __size, format, ##args); \
> + __ret < 0 || (size_t)__ret >= __size; \
> + })
>
> #define pthread_cleanup_push_cast(f, arg) \
> pthread_cleanup_push(((void (*)(void *))&f), (arg))
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index 291db8f5..28a2150d 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -393,8 +393,7 @@ static int _failed_wwid_op(const char *wwid, bool rw,
> long lockfd;
> int r = -1;
>
> - if (snprintf(path, sizeof(path), "%s/%s", shm_dir, wwid)
> - >= sizeof(path)) {
> + if (safe_sprintf(path, "%s/%s", shm_dir, wwid)) {
> condlog(1, "%s: path name overflow", __func__);
> return -1;
> }
> diff --git a/multipath/main.c b/multipath/main.c
> index c2ef8c7b..c553437b 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -423,8 +423,7 @@ static int find_multipaths_check_timeout(const struct path *pp, long tmo,
>
> clock_gettime(CLOCK_REALTIME, &now);
>
> - if (snprintf(path, sizeof(path), "%s/%s", shm_find_mp_dir, pp->dev_t)
> - >= sizeof(path)) {
> + if (safe_sprintf(path, "%s/%s", shm_find_mp_dir, pp->dev_t)) {
> condlog(1, "%s: path name overflow", __func__);
> return FIND_MULTIPATHS_ERROR;
> }
> --
> 2.23.0
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
