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

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

[Thomas Huth]

The linker on OpenBSD complains:

 ld: warning: dirtyrate.c:447 (../src/migration/dirtyrate.c:447)(...):
 warning: strcpy() is almost always misused, please use strlcpy()

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));
 }
 
 static void free_ramblock_dirty_info(struct RamblockDirtyInfo *infos, int count)
-- 
2.47.0

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

[Daniel P. Berrangé]

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....

> 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.


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 :|

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

[Peter Xu]

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

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

[Thomas Huth]

On 16/10/2024 18.22, 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....

There are only a fistful of other warnings. I guess most of the spots are 
turned into inlined code by the compiler, so the linker never sees those 
other occurrences.

>> 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 alternatively check the return value of g_strlcpy() ? ... but that 
wouldn't work if pstrcpy() if we switch to that function instead.

I don't mind either way - Peter, Fabiano, Hyman, what's your opinion here?

  Thomas

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

[Yong Huang]

[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]

On Thu, Oct 17, 2024 at 1:40 PM Thomas Huth <thuth@redhat.com> wrote:

> On 16/10/2024 18.22, 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....
>
> There are only a fistful of other warnings. I guess most of the spots are
> turned into inlined code by the compiler, so the linker never sees those
> other occurrences.
>
> >> 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 alternatively check the return value of g_strlcpy() ? ... but that
> wouldn't work if pstrcpy() if we switch to that function instead.
>
> I don't mind either way - Peter, Fabiano, Hyman, what's your opinion here?
>
>   Thomas
>
>
Since RamblockDirtyInfo is only used in dirtyrate.c,  I'm inclined to just
check the return value of g_strlcpy to avoid DoS attack, and fix this
warning case by case.

Thanks,

Yong

-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 4538 bytes --]

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

[Peter Maydell]

On Thu, 17 Oct 2024 at 06:41, Thomas Huth <thuth@redhat.com> wrote:
>
> On 16/10/2024 18.22, 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....
>
> There are only a fistful of other warnings. I guess most of the spots are
> turned into inlined code by the compiler, so the linker never sees those
> other occurrences.

From my vm-build-openbsd buildlog:

$ < /tmp/par0L_eB.par grep 'almost always misused'| wc -l
305

(I've had these in my "ignore this type of warning" filter
for grepping the build logs for years.)

-- PMM