diff mbox series

[v3,45/72] libmultipath: fix -Wsign-compare warnings with snprintf()

Message ID 20191107092651.10975-2-martin.wilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipath-tools: cleanup and warning enablement | expand

Commit Message

Martin Wilck Nov. 7, 2019, 9:27 a.m. UTC 
From: Martin Wilck <mwilck@suse.com>

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@suse.com>
---
 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(-)

Comments

Benjamin Marzinski Nov. 13, 2019, 10:07 p.m. UTC | #1 
On Thu, Nov 07, 2019 at 09:27:41AM +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> 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@suse.com>
> ---
>  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@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

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;
 	}