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

[Yongji Xie <xieyongji@bytedance.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