Re: [PATCH] migration/dirtyrate: Silence warning about strcpy() on OpenBSD

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 16, 2024 at 05:22:11PM +0100, Daniel P. Berrangé wrote:
> On Wed, Oct 16, 2024 at 06:07:12PM +0200, Thomas Huth wrote:
> > The linker on OpenBSD complains:
> > 
> >  ld: warning: dirtyrate.c:447 (../src/migration/dirtyrate.c:447)(...):
> >  warning: strcpy() is almost always misused, please use strlcpy()
> 
> Is that the only place it complains ?  We use 'strcpy' in almost
> 100 places across the codebase....

I remember we switched some places to pstrcpy(), which seems superior,
where I saw tha g_strlcpy() is mostly strlcpy() and the man page says:

      strlcpy(3bsd) and strlcat(3bsd) are similar, but have important
      performance problems; see BUGS.

      ...

      BUGS

      ...

      strlcpy(3) and strlcat(3) need to read the entire src string, even
      if the destination buffer is small.  This makes them vulnerable to
      Denial of Service (DoS) attacks if an attacker can control the
      length of the src string.  And if not, they're still unnecessarily
      slow.

> 
> > It's currently not a real problem in this case since both arrays
> > have the same size (256 bytes). But just in case somebody changes
> > the size of the source array in the future, let's better play safe
> > and use g_strlcpy() here instead.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  migration/dirtyrate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > index 233acb0855..090c76e934 100644
> > --- a/migration/dirtyrate.c
> > +++ b/migration/dirtyrate.c
> > @@ -444,7 +444,7 @@ static void get_ramblock_dirty_info(RAMBlock *block,
> >      info->ramblock_pages = qemu_ram_get_used_length(block) >>
> >                             qemu_target_page_bits();
> >      info->ramblock_addr = qemu_ram_get_host_addr(block);
> > -    strcpy(info->idstr, qemu_ram_get_idstr(block));
> > +    g_strlcpy(info->idstr, qemu_ram_get_idstr(block), sizeof(info->idstr));
> >  }
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> Is it worth also adding
> 
>   G_STATIC_ASSERT(sizeof((struct RamblockDirtyInfo){}.idstr) ==
>                   sizeof((struct RAMBlock){}.idstr));
> 
> at the top of this file, since both of these fields are expected to
> be the same size by this code, to avoid truncation.

Or we could define a size macro for RAMBlock.idstr and use it everywhere
where it's fundamentally the same thing.

> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

-- 
Peter Xu