| Message ID | 20241016160712.962407-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | migration/dirtyrate: Silence warning about strcpy() on OpenBSD | expand |
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
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 :| >
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
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
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
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)
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(-)