Re: [RFC PATCH] libvduse: Do not truncate terminating NUL character with strncpy()

[Markus Armbruster <armbru@redhat.com>]

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!