| Message ID | 20220919192306.52729-1-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] libvduse: Do not truncate terminating NUL character with strncpy() | expand |
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > GCC 8 added a -Wstringop-truncation warning: > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > bug 81117 is specifically intended to highlight likely unintended > uses of the strncpy function that truncate the terminating NUL > character from the source string. > > Here the next line indeed unconditionally zeroes the last byte, so > we can call strncpy() on the buffer size less the last byte. Actually, the buffer is all zero to begin with, so we could do this even without the next line's assignment. > This > fixes when using gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0: > > [42/666] Compiling C object subprojects/libvduse/libvduse.a.p/libvduse.c.o > FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o > cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p -Isubprojects/libvduse -I../../subprojects/libvduse [...] -o subprojects/libvduse/libvduse.a.p/libvduse.c.o -c ../../subprojects/libvduse/libvduse.c > In file included from /usr/include/string.h:495, > from ../../subprojects/libvduse/libvduse.c:24: > In function ‘strncpy’, > inlined from ‘vduse_dev_create’ at ../../subprojects/libvduse/libvduse.c:1312:5: > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 256 equals destination size [-Werror=stringop-truncation] > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > ninja: build stopped: cannot make progress due to previous errors. > > Fixes: d9cf16c0be ("libvduse: Replace strcpy() with strncpy()") > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> The subject feels a bit too alarming to me. This patch suppresses a warning, no less, no more. Behavior doesn't change. Perhaps libvduse: Avoid warning about dangerous use of strncpy() > --- > Cc: Xie Yongji <xieyongji@bytedance.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > > RFC: Any better idea? We can't use strpadcpy() because libvduse > doesn't depend on QEMU. There's no need for padding: the destination calloc'ed. So, pstrcpy() would do, but it's just as unavailable. Can we use GLib? There's g_strlcpy(). Outside this patch's scope: is silent truncation what we want? > --- > subprojects/libvduse/libvduse.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c > index 1a5981445c..e460780ce3 100644 > --- a/subprojects/libvduse/libvduse.c > +++ b/subprojects/libvduse/libvduse.c > @@ -1309,7 +1309,7 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id, > goto err_dev; > } > > - strncpy(dev_config->name, name, VDUSE_NAME_MAX); > + strncpy(dev_config->name, name, VDUSE_NAME_MAX - 1); > dev_config->name[VDUSE_NAME_MAX - 1] = '\0'; > dev_config->device_id = device_id; > dev_config->vendor_id = vendor_id; Since the buffer is known to be all zero, the padding done by strncpy() is unnecessary, and so is the assignment that follows it. I don't mind. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On Tue, Sep 20, 2022 at 7:25 PM Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > > > GCC 8 added a -Wstringop-truncation warning: > > > > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for > > bug 81117 is specifically intended to highlight likely unintended > > uses of the strncpy function that truncate the terminating NUL > > character from the source string. > > > > Here the next line indeed unconditionally zeroes the last byte, so > > we can call strncpy() on the buffer size less the last byte. > > Actually, the buffer is all zero to begin with, so we could do this even > without the next line's assignment. > Yes, I think we can remove the next line's assignment. > > This > > fixes when using gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0: > > > > [42/666] Compiling C object subprojects/libvduse/libvduse.a.p/libvduse.c.o > > FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o > > cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p -Isubprojects/libvduse -I../../subprojects/libvduse [...] -o subprojects/libvduse/libvduse.a.p/libvduse.c.o -c ../../subprojects/libvduse/libvduse.c > > In file included from /usr/include/string.h:495, > > from ../../subprojects/libvduse/libvduse.c:24: > > In function ‘strncpy’, > > inlined from ‘vduse_dev_create’ at ../../subprojects/libvduse/libvduse.c:1312:5: > > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 256 equals destination size [-Werror=stringop-truncation] > > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > cc1: all warnings being treated as errors > > ninja: build stopped: cannot make progress due to previous errors. > > > > Fixes: d9cf16c0be ("libvduse: Replace strcpy() with strncpy()") > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > The subject feels a bit too alarming to me. This patch suppresses a > warning, no less, no more. Behavior doesn't change. Perhaps > > libvduse: Avoid warning about dangerous use of strncpy() > > > --- > > Cc: Xie Yongji <xieyongji@bytedance.com> > > Cc: Markus Armbruster <armbru@redhat.com> > > Cc: Kevin Wolf <kwolf@redhat.com> > > > > RFC: Any better idea? We can't use strpadcpy() because libvduse > > doesn't depend on QEMU. > > There's no need for padding: the destination calloc'ed. So, pstrcpy() > would do, but it's just as unavailable. Can we use GLib? There's > g_strlcpy(). > > Outside this patch's scope: is silent truncation what we want? > Actually silent truncation would not happen since we called vduse_name_is_invalid() before. static inline bool vduse_name_is_invalid(const char *name) { return strlen(name) >= VDUSE_NAME_MAX || strstr(name, ".."); } Thanks, Yongji
Yongji Xie <xieyongji@bytedance.com> writes: > On Tue, Sep 20, 2022 at 7:25 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> >> > GCC 8 added a -Wstringop-truncation warning: >> > >> > The -Wstringop-truncation warning added in GCC 8.0 via r254630 for >> > bug 81117 is specifically intended to highlight likely unintended >> > uses of the strncpy function that truncate the terminating NUL >> > character from the source string. >> > >> > Here the next line indeed unconditionally zeroes the last byte, so >> > we can call strncpy() on the buffer size less the last byte. >> >> Actually, the buffer is all zero to begin with, so we could do this even >> without the next line's assignment. >> > > Yes, I think we can remove the next line's assignment. > >> > This >> > fixes when using gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0: >> > >> > [42/666] Compiling C object subprojects/libvduse/libvduse.a.p/libvduse.c.o >> > FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o >> > cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p -Isubprojects/libvduse -I../../subprojects/libvduse [...] -o subprojects/libvduse/libvduse.a.p/libvduse.c.o -c ../../subprojects/libvduse/libvduse.c >> > In file included from /usr/include/string.h:495, >> > from ../../subprojects/libvduse/libvduse.c:24: >> > In function ‘strncpy’, >> > inlined from ‘vduse_dev_create’ at ../../subprojects/libvduse/libvduse.c:1312:5: >> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 256 equals destination size [-Werror=stringop-truncation] >> > 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); >> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> > cc1: all warnings being treated as errors >> > ninja: build stopped: cannot make progress due to previous errors. >> > >> > Fixes: d9cf16c0be ("libvduse: Replace strcpy() with strncpy()") >> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> The subject feels a bit too alarming to me. This patch suppresses a >> warning, no less, no more. Behavior doesn't change. Perhaps >> >> libvduse: Avoid warning about dangerous use of strncpy() >> >> > --- >> > Cc: Xie Yongji <xieyongji@bytedance.com> >> > Cc: Markus Armbruster <armbru@redhat.com> >> > Cc: Kevin Wolf <kwolf@redhat.com> >> > >> > RFC: Any better idea? We can't use strpadcpy() because libvduse >> > doesn't depend on QEMU. >> >> There's no need for padding: the destination calloc'ed. So, pstrcpy() >> would do, but it's just as unavailable. Can we use GLib? There's >> g_strlcpy(). >> >> Outside this patch's scope: is silent truncation what we want? >> > > Actually silent truncation would not happen since we called > vduse_name_is_invalid() before. > > static inline bool vduse_name_is_invalid(const char *name) > { > return strlen(name) >= VDUSE_NAME_MAX || strstr(name, ".."); > } Ah, so even strcpy() would be safe (but might trigger a compiler warning). Thanks!
diff --git a/subprojects/libvduse/libvduse.c b/subprojects/libvduse/libvduse.c index 1a5981445c..e460780ce3 100644 --- a/subprojects/libvduse/libvduse.c +++ b/subprojects/libvduse/libvduse.c @@ -1309,7 +1309,7 @@ VduseDev *vduse_dev_create(const char *name, uint32_t device_id, goto err_dev; } - strncpy(dev_config->name, name, VDUSE_NAME_MAX); + strncpy(dev_config->name, name, VDUSE_NAME_MAX - 1); dev_config->name[VDUSE_NAME_MAX - 1] = '\0'; dev_config->device_id = device_id; dev_config->vendor_id = vendor_id;
GCC 8 added a -Wstringop-truncation warning: The -Wstringop-truncation warning added in GCC 8.0 via r254630 for bug 81117 is specifically intended to highlight likely unintended uses of the strncpy function that truncate the terminating NUL character from the source string. Here the next line indeed unconditionally zeroes the last byte, so we can call strncpy() on the buffer size less the last byte. This fixes when using gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0: [42/666] Compiling C object subprojects/libvduse/libvduse.a.p/libvduse.c.o FAILED: subprojects/libvduse/libvduse.a.p/libvduse.c.o cc -m64 -mcx16 -Isubprojects/libvduse/libvduse.a.p -Isubprojects/libvduse -I../../subprojects/libvduse [...] -o subprojects/libvduse/libvduse.a.p/libvduse.c.o -c ../../subprojects/libvduse/libvduse.c In file included from /usr/include/string.h:495, from ../../subprojects/libvduse/libvduse.c:24: In function ‘strncpy’, inlined from ‘vduse_dev_create’ at ../../subprojects/libvduse/libvduse.c:1312:5: /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error: ‘__builtin_strncpy’ specified bound 256 equals destination size [-Werror=stringop-truncation] 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors ninja: build stopped: cannot make progress due to previous errors. Fixes: d9cf16c0be ("libvduse: Replace strcpy() with strncpy()") Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- Cc: Xie Yongji <xieyongji@bytedance.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> RFC: Any better idea? We can't use strpadcpy() because libvduse doesn't depend on QEMU. --- subprojects/libvduse/libvduse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)