1. Patchew
  2. QEMU
  3. View series

[PATCH] qga: Fix ubsan warning

Thomas Huth posted 1 patch 3 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250728173016.314460-1-thuth@redhat.com
Maintainers: Michael Roth <michael.roth@amd.com>, Kostiantyn Kostiuk <kkostiuk@redhat.com>
There is a newer version of this series
qga/commands-linux.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] qga: Fix ubsan warning
Posted by Thomas Huth 3 months, 2 weeks ago 
From: Thomas Huth <thuth@redhat.com>

When compiling QEMU with --enable-ubsan there is a undefined behavior
warning when running "make check":

 .../qga/commands-linux.c:452:15: runtime error: applying non-zero offset 5 to null pointer
 #0 0x55ea7b89450c in build_guest_fsinfo_for_pci_dev ..../qga/commands-linux.c:452:15

Add a check to avoid incrementing the NULL pointer here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qga/commands-linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 9e8a934b9a6..caf7c3ca22b 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -449,7 +449,9 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
         has_ata = true;
     } else {
         p = strstr(syspath, "/host");
-        q = p + 5;
+        if (p) {
+            q = p + 5;
+        }
     }
     if (p && sscanf(q, "%u", &host) == 1) {
         has_host = true;
-- 
2.50.1
Re: [PATCH] qga: Fix ubsan warning
Posted by Daniel P. Berrangé 3 months, 2 weeks ago 
On Mon, Jul 28, 2025 at 07:30:16PM +0200, Thomas Huth wrote:
> From: Thomas Huth <thuth@redhat.com>
> 
> When compiling QEMU with --enable-ubsan there is a undefined behavior
> warning when running "make check":
> 
>  .../qga/commands-linux.c:452:15: runtime error: applying non-zero offset 5 to null pointer
>  #0 0x55ea7b89450c in build_guest_fsinfo_for_pci_dev ..../qga/commands-linux.c:452:15
> 
> Add a check to avoid incrementing the NULL pointer here.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  qga/commands-linux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 9e8a934b9a6..caf7c3ca22b 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -449,7 +449,9 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
>          has_ata = true;
>      } else {
>          p = strstr(syspath, "/host");
> -        q = p + 5;
> +        if (p) {
> +            q = p + 5;
> +        }
>      }
>      if (p && sscanf(q, "%u", &host) == 1) {

q is always non-NULL if p is non-NULL, so this is safe, but I would be more
happy with this changing to 'q && sscanf' to eliminate the indirection.


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] qga: Fix ubsan warning
Posted by Thomas Huth 3 months, 2 weeks ago 
On 28/07/2025 19.53, Daniel P. Berrangé wrote:
> On Mon, Jul 28, 2025 at 07:30:16PM +0200, Thomas Huth wrote:
>> From: Thomas Huth <thuth@redhat.com>
>>
>> When compiling QEMU with --enable-ubsan there is a undefined behavior
>> warning when running "make check":
>>
>>   .../qga/commands-linux.c:452:15: runtime error: applying non-zero offset 5 to null pointer
>>   #0 0x55ea7b89450c in build_guest_fsinfo_for_pci_dev ..../qga/commands-linux.c:452:15
>>
>> Add a check to avoid incrementing the NULL pointer here.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   qga/commands-linux.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
>> index 9e8a934b9a6..caf7c3ca22b 100644
>> --- a/qga/commands-linux.c
>> +++ b/qga/commands-linux.c
>> @@ -449,7 +449,9 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
>>           has_ata = true;
>>       } else {
>>           p = strstr(syspath, "/host");
>> -        q = p + 5;
>> +        if (p) {
>> +            q = p + 5;
>> +        }
>>       }
>>       if (p && sscanf(q, "%u", &host) == 1) {
> 
> q is always non-NULL if p is non-NULL, so this is safe, but I would be more
> happy with this changing to 'q && sscanf' to eliminate the indirection.

If we agree to do a bigger change here, I'd rather drop the "q" pointer 
completely and use a new integer variable instead, something like:

     int offset;
     ...
     p = strstr(syspath, "/ata");
     if (p) {
         offset = 4;
         has_ata = true;
     } else {
         offset = 5;
         p = strstr(syspath, "/host");
     }
     if (p && sscanf(p + offset, "%u", &host) == 1) {
         ...
     }

WDYT?

   Thomas
Re: [PATCH] qga: Fix ubsan warning
Posted by Kostiantyn Kostiuk 3 months, 2 weeks ago 
LGTM

On Tue, Jul 29, 2025 at 9:02 AM Thomas Huth <thuth@redhat.com> wrote:

> On 28/07/2025 19.53, Daniel P. Berrangé wrote:
> > On Mon, Jul 28, 2025 at 07:30:16PM +0200, Thomas Huth wrote:
> >> From: Thomas Huth <thuth@redhat.com>
> >>
> >> When compiling QEMU with --enable-ubsan there is a undefined behavior
> >> warning when running "make check":
> >>
> >>   .../qga/commands-linux.c:452:15: runtime error: applying non-zero
> offset 5 to null pointer
> >>   #0 0x55ea7b89450c in build_guest_fsinfo_for_pci_dev
> ..../qga/commands-linux.c:452:15
> >>
> >> Add a check to avoid incrementing the NULL pointer here.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   qga/commands-linux.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> >> index 9e8a934b9a6..caf7c3ca22b 100644
> >> --- a/qga/commands-linux.c
> >> +++ b/qga/commands-linux.c
> >> @@ -449,7 +449,9 @@ static bool build_guest_fsinfo_for_pci_dev(char
> const *syspath,
> >>           has_ata = true;
> >>       } else {
> >>           p = strstr(syspath, "/host");
> >> -        q = p + 5;
> >> +        if (p) {
> >> +            q = p + 5;
> >> +        }
> >>       }
> >>       if (p && sscanf(q, "%u", &host) == 1) {
> >
> > q is always non-NULL if p is non-NULL, so this is safe, but I would be
> more
> > happy with this changing to 'q && sscanf' to eliminate the indirection.
>
> If we agree to do a bigger change here, I'd rather drop the "q" pointer
> completely and use a new integer variable instead, something like:
>
>      int offset;
>      ...
>      p = strstr(syspath, "/ata");
>      if (p) {
>          offset = 4;
>          has_ata = true;
>      } else {
>          offset = 5;
>          p = strstr(syspath, "/host");
>      }
>      if (p && sscanf(p + offset, "%u", &host) == 1) {
>          ...
>      }
>
> WDYT?
>
>    Thomas
>
>
Re: [PATCH] qga: Fix ubsan warning
Posted by Daniel P. Berrangé 3 months, 2 weeks ago 
On Tue, Jul 29, 2025 at 08:02:25AM +0200, Thomas Huth wrote:
> On 28/07/2025 19.53, Daniel P. Berrangé wrote:
> > On Mon, Jul 28, 2025 at 07:30:16PM +0200, Thomas Huth wrote:
> > > From: Thomas Huth <thuth@redhat.com>
> > > 
> > > When compiling QEMU with --enable-ubsan there is a undefined behavior
> > > warning when running "make check":
> > > 
> > >   .../qga/commands-linux.c:452:15: runtime error: applying non-zero offset 5 to null pointer
> > >   #0 0x55ea7b89450c in build_guest_fsinfo_for_pci_dev ..../qga/commands-linux.c:452:15
> > > 
> > > Add a check to avoid incrementing the NULL pointer here.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   qga/commands-linux.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> > > index 9e8a934b9a6..caf7c3ca22b 100644
> > > --- a/qga/commands-linux.c
> > > +++ b/qga/commands-linux.c
> > > @@ -449,7 +449,9 @@ static bool build_guest_fsinfo_for_pci_dev(char const *syspath,
> > >           has_ata = true;
> > >       } else {
> > >           p = strstr(syspath, "/host");
> > > -        q = p + 5;
> > > +        if (p) {
> > > +            q = p + 5;
> > > +        }
> > >       }
> > >       if (p && sscanf(q, "%u", &host) == 1) {
> > 
> > q is always non-NULL if p is non-NULL, so this is safe, but I would be more
> > happy with this changing to 'q && sscanf' to eliminate the indirection.
> 
> If we agree to do a bigger change here, I'd rather drop the "q" pointer
> completely and use a new integer variable instead, something like:
> 
>     int offset;
>     ...
>     p = strstr(syspath, "/ata");
>     if (p) {
>         offset = 4;
>         has_ata = true;
>     } else {
>         offset = 5;
>         p = strstr(syspath, "/host");
>     }
>     if (p && sscanf(p + offset, "%u", &host) == 1) {
>         ...
>     }
> 
> WDYT?

Works for me.


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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.