| Message ID | 20191012212703.12989-46-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 |
On 2019-10-12 14:28, Martin Wilck wrote: > - if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >= bufsiz) > + if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) > + >= (int)bufsiz) > return 0; Please don't insert casts like this. I think enabling -Wsign-compare is wrong because it makes the source code ugly. Thank, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hello Bart, On Sat, 2019-10-12 at 15:59 -0700, Bart Van Assche wrote: > On 2019-10-12 14:28, Martin Wilck wrote: > > - if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >= > > bufsiz) > > + if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) > > + >= (int)bufsiz) > > return 0; > > Please don't insert casts like this. I think enabling -Wsign-compare > is > wrong because it makes the source code ugly. The casts make it explicit which signedness is intended, which is a good thing IMO, better than the compiler trying to figure it out using implicit type conversion. Enabling the warning will help avoid subtle programming errors in the future, by forcing us to think twice about signedness. Considering that, isn't this ugliness - which I don't dispute - a relatively minor issue? In this particular case, we're dealing with a historically-caused property of snprintf(), as you are probably aware (https://stackoverflow.com/questions/45740276/why-does-printf-return-an-int-instead-of-a-size-t-in-c), so I'd argue the ugliness is forced upon us, sort of. We can hide the ugliness in a macro if you prefer. Actually, we have safe_snprintf() already. We just need to use it in all places where this kind of comparison is made. Would that be acceptable to you? Thanks, Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2019-10-22 09:07, Martin Wilck wrote: > Hello Bart, > > On Sat, 2019-10-12 at 15:59 -0700, Bart Van Assche wrote: >> On 2019-10-12 14:28, Martin Wilck wrote: >>> - if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >= >>> bufsiz) >>> + if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >>> + >= (int)bufsiz) >>> return 0; >> >> Please don't insert casts like this. I think enabling -Wsign-compare >> is >> wrong because it makes the source code ugly. > > The casts make it explicit which signedness is intended, which is a > good thing IMO, better than the compiler trying to figure it out > using implicit type conversion. Enabling the warning will help avoid > subtle programming errors in the future, by forcing us to think twice > about signedness. Considering that, isn't this ugliness - which I don't > dispute - a relatively minor issue? > > In this particular case, we're dealing with a historically-caused > property of snprintf(), as you are probably aware > (https://stackoverflow.com/questions/45740276/why-does-printf-return-an-int-instead-of-a-size-t-in-c), > so I'd argue the ugliness is forced upon us, sort of. > > We can hide the ugliness in a macro if you prefer. Actually, we have > safe_snprintf() already. We just need to use it in all places where > this kind of comparison is made. Would that be acceptable to you? Hi Martin, Have you considered to use asprintf() instead of snprintf() and a check whether the output buffer overflows? I think the former is a more elegant solution. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Tue, 2019-10-22 at 09:47 -0700, Bart Van Assche wrote: > On 2019-10-22 09:07, Martin Wilck wrote: > > > > In this particular case, we're dealing with a historically-caused > > property of snprintf(), as you are probably aware > > ( > > https://stackoverflow.com/questions/45740276/why-does-printf-return-an-int-instead-of-a-size-t-in-c > > ), > > so I'd argue the ugliness is forced upon us, sort of. > > > > We can hide the ugliness in a macro if you prefer. Actually, we > > have > > safe_snprintf() already. We just need to use it in all places where > > this kind of comparison is made. Would that be acceptable to you? > > Hi Martin, > > Have you considered to use asprintf() instead of snprintf() and a > check > whether the output buffer overflows? I think the former is a more > elegant solution. Most uses of snprintf() are in libmultipath printing code, where items are printed sequentially into a big buffer, advancing the buffer pointer on the way. asprintf() doesn't match that use case well, AFAICS. But in some other places, switching to asprintf would certainly make sense. Anyway, I'd like to do that in a separate patch set if you don't mind; this one is big enough already. Thanks, Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 2019-10-22 13:34, Martin Wilck wrote: > Most uses of snprintf() are in libmultipath printing code, where items > are printed sequentially into a big buffer, advancing the buffer > pointer on the way. asprintf() doesn't match that use case well, > AFAICS. But in some other places, switching to asprintf would certainly > make sense. Anyway, I'd like to do that in a separate patch set if you > don't mind; this one is big enough already. Hi Martin, For this patch, have you considered to change the type of the 'bufsiz' argument of format_partname() from size_t into int or unsigned int? I do not expect that the output string will ever exceed 65535 characters. As you probably know the C standard guarantees that there are at least 16 bits in an int. I'd like to reiterate that introducing -Wsign-compare seems dubious to me. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hello Bart, On Tue, 2019-10-22 at 14:32 -0700, Bart Van Assche wrote: > I'd like to reiterate that introducing -Wsign-compare seems dubious > to me. While I disagree with this general statement, I now realized that my approach to use (int) casts in this specific patch was indeed wrong, because it prevents detection of negative snprintf() return values. I'm going to rework this patch. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c index 3aa4988d..7e599e07 100644 --- a/kpartx/devmapper.c +++ b/kpartx/devmapper.c @@ -107,7 +107,8 @@ 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 (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) + >= (int)bufsiz) return 0; strip_slash(buf); return 1; diff --git a/kpartx/kpartx.h b/kpartx/kpartx.h index 52920e43..3ec13dbc 100644 --- a/kpartx/kpartx.h +++ b/kpartx/kpartx.h @@ -17,7 +17,7 @@ #define unlikely(x) __builtin_expect(!!(x), 0) #define safe_sprintf(var, format, args...) \ - snprintf(var, sizeof(var), format, ##args) >= sizeof(var) + snprintf(var, sizeof(var), format, ##args) >= (int)sizeof(var) #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..17569b36 100644 --- a/libmultipath/foreign/nvme.c +++ b/libmultipath/foreign/nvme.c @@ -592,7 +592,7 @@ static void test_ana_support(struct nvme_map *map, struct udev_device *ctl) dev_t = udev_device_get_sysattr_value(ctl, "dev"); if (snprintf(sys_path, sizeof(sys_path), "/dev/char/%s", dev_t) - >= sizeof(sys_path)) + >= (int)sizeof(sys_path)) return; fd = open(sys_path, O_RDONLY); @@ -664,7 +664,7 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map) struct udev_device *ctrl, *udev; if (snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s", fn) - >= sizeof(pathbuf) - n) + >= (int)(sizeof(pathbuf) - n)) 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..eb1f03e1 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 >= (int)sizeof(pathbuf)) { condlog(1, "%s: pathname overflow", __func__); return false; } @@ -329,7 +329,7 @@ bool sysfs_is_multipathed(const struct path *pp) if (snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s/dm/uuid", di[i]->d_name) - >= sizeof(pathbuf) - n) + >= (int)(sizeof(pathbuf) - n)) continue; fd = open(pathbuf, O_RDONLY); diff --git a/libmultipath/util.c b/libmultipath/util.c index ccc0de29..4657e7db 100644 --- a/libmultipath/util.c +++ b/libmultipath/util.c @@ -213,7 +213,8 @@ int devt2devname(char *devname, int devname_len, char *devt) if ((major == tmpmaj) && (minor == tmpmin)) { if (snprintf(block_path, sizeof(block_path), - "/sys/block/%s", dev) >= sizeof(block_path)) { + "/sys/block/%s", dev) + >= (int)sizeof(block_path)) { 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..cfc3b7a9 100644 --- a/libmultipath/util.h +++ b/libmultipath/util.h @@ -29,9 +29,9 @@ 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) + snprintf((var), sizeof(var), (format), ##args) >= (int)sizeof(var) #define safe_snprintf(var, size, format, args...) \ - snprintf(var, size, format, ##args) >= size + snprintf(var, size, format, ##args) >= (int)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..57c2707b 100644 --- a/libmultipath/wwids.c +++ b/libmultipath/wwids.c @@ -394,7 +394,7 @@ static int _failed_wwid_op(const char *wwid, bool rw, int r = -1; if (snprintf(path, sizeof(path), "%s/%s", shm_dir, wwid) - >= sizeof(path)) { + >= (int)sizeof(path)) { condlog(1, "%s: path name overflow", __func__); return -1; } diff --git a/multipath/main.c b/multipath/main.c index c2ef8c7b..8d518585 100644 --- a/multipath/main.c +++ b/multipath/main.c @@ -424,7 +424,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)) { + >= (int)sizeof(path)) { condlog(1, "%s: path name overflow", __func__); return FIND_MULTIPATHS_ERROR; }