diff mbox series

hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib

Message ID 20201013100315.3554421-1-philmd@redhat.com
State New
Headers show
Series hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib | expand

Commit Message

Philippe Mathieu-Daudé Oct. 13, 2020, 10:03 a.m. UTC 
set_pci_host_devaddr() is hard to follow, thus bug-prone.
We indeed introduced a bug in commit bccb20c49df, as the
same line might be used to parse a bus (up to 0xff) or a
slot (up to 0x1f). Instead of making things worst, rewrite
using g_strsplit().

Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")
Reported-by: Klaus Herman <kherman@inbox.lv>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

Comments

Daniel P. Berrangé Oct. 13, 2020, 10:07 a.m. UTC | #1 
On Tue, Oct 13, 2020 at 12:03:15PM +0200, Philippe Mathieu-Daudé wrote:
> set_pci_host_devaddr() is hard to follow, thus bug-prone.

> We indeed introduced a bug in commit bccb20c49df, as the

> same line might be used to parse a bus (up to 0xff) or a

> slot (up to 0x1f). Instead of making things worst, rewrite

> using g_strsplit().

> 

> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")

> Reported-by: Klaus Herman <kherman@inbox.lv>

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---

>  hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------

>  1 file changed, 27 insertions(+), 34 deletions(-)

> 

> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c

> index 49bdd125814..e6e081efd58 100644

> --- a/hw/core/qdev-properties-system.c

> +++ b/hw/core/qdev-properties-system.c

> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,

>      DeviceState *dev = DEVICE(obj);

>      Property *prop = opaque;

>      PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);

> -    char *str, *p;

> -    const char *e;

> +    g_autofree char *str = NULL;

> +    g_autofree char **col_s0 = NULL;

> +    g_autofree char **dot_s = NULL;


These free the array, but not the array elements.

You need to use

   g_auto(GStrv) col_s0 = NULL

GStrv is a typedef for char **, that exists solely so that there is
a typename against which g_auto can be used.

> +    char **col_s;

>      unsigned long val;

> -    unsigned long dom = 0, bus = 0;

> -    unsigned int slot = 0, func = 0;

>  

>      if (dev->realized) {

>          qdev_prop_set_after_realize(dev, name, errp);

> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,

>          return;

>      }

>  

> -    p = str;

> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {

> +    col_s = col_s0 = g_strsplit(str, ":", 3);

> +    if (!col_s || !col_s[0] || !col_s[1]) {

>          goto inval;

>      }

> -    if (*e != ':') {

> -        goto inval;

> -    }

> -    bus = val;

>  

> -    p = (char *)e + 1;

> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {

> -        goto inval;

> -    }

> -    if (*e == ':') {

> -        dom = bus;

> -        bus = val;

> -        p = (char *)e + 1;

> -        if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {

> +    /* domain */

> +    if (col_s[2]) {

> +        if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {

>              goto inval;

>          }

> +        addr->domain = val;

> +        col_s++;

> +    } else {

> +        addr->domain = 0;

>      }

> -    slot = val;

>  

> -    if (*e != '.') {

> +    /* bus */

> +    if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {

>          goto inval;

>      }

> -    p = (char *)e + 1;

> -    if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {

> -        goto inval;

> -    }

> -    func = val;

> +    addr->bus = val;

>  

> -    if (bus > 0xff) {

> +    /* <slot>.<func> */

> +    dot_s = g_strsplit(col_s[1], ".", 2);

> +    if (!dot_s || !dot_s[0] || !dot_s[1]) {

>          goto inval;

>      }

>  

> -    if (*e) {

> +    /* slot */

> +    if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {

>          goto inval;

>      }

> +    addr->slot = val;

>  

> -    addr->domain = dom;

> -    addr->bus = bus;

> -    addr->slot = slot;

> -    addr->function = func;

> +    /* func */

> +    if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {

> +        goto inval;

> +    }

> +    addr->function = val;

>  

> -    g_free(str);

>      return;

>  

>  inval:

>      error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);

> -    g_free(str);

>  }

>  

>  const PropertyInfo qdev_prop_pci_host_devaddr = {

> -- 

> 2.26.2

> 


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 :|
Philippe Mathieu-Daudé Oct. 13, 2020, 10:18 a.m. UTC | #2 
On 10/13/20 12:07 PM, Daniel P. Berrangé wrote:
> On Tue, Oct 13, 2020 at 12:03:15PM +0200, Philippe Mathieu-Daudé wrote:

>> set_pci_host_devaddr() is hard to follow, thus bug-prone.

>> We indeed introduced a bug in commit bccb20c49df, as the

>> same line might be used to parse a bus (up to 0xff) or a

>> slot (up to 0x1f). Instead of making things worst, rewrite

>> using g_strsplit().

>>

>> Fixes: bccb20c49df ("Use qemu_strtoul() in set_pci_host_devaddr()")

>> Reported-by: Klaus Herman <kherman@inbox.lv>

>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>> ---

>>   hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------

>>   1 file changed, 27 insertions(+), 34 deletions(-)

>>

>> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c

>> index 49bdd125814..e6e081efd58 100644

>> --- a/hw/core/qdev-properties-system.c

>> +++ b/hw/core/qdev-properties-system.c

>> @@ -878,11 +878,11 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,

>>       DeviceState *dev = DEVICE(obj);

>>       Property *prop = opaque;

>>       PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);

>> -    char *str, *p;

>> -    const char *e;

>> +    g_autofree char *str = NULL;

>> +    g_autofree char **col_s0 = NULL;

>> +    g_autofree char **dot_s = NULL;

> 

> These free the array, but not the array elements.

> 

> You need to use

> 

>     g_auto(GStrv) col_s0 = NULL

> 

> GStrv is a typedef for char **, that exists solely so that there is

> a typename against which g_auto can be used.


Ah I have been wondering how that part works.

Thanks, I'll fix.

> 

>> +    char **col_s;

>>       unsigned long val;

>> -    unsigned long dom = 0, bus = 0;

>> -    unsigned int slot = 0, func = 0;

>>   

>>       if (dev->realized) {

>>           qdev_prop_set_after_realize(dev, name, errp);

>> @@ -893,57 +893,50 @@ static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,

>>           return;

>>       }

>>   

>> -    p = str;

>> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {

>> +    col_s = col_s0 = g_strsplit(str, ":", 3);

>> +    if (!col_s || !col_s[0] || !col_s[1]) {

>>           goto inval;

>>       }

>> -    if (*e != ':') {

>> -        goto inval;

>> -    }

>> -    bus = val;

>>   

>> -    p = (char *)e + 1;

>> -    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {

>> -        goto inval;

>> -    }

>> -    if (*e == ':') {

>> -        dom = bus;

>> -        bus = val;

>> -        p = (char *)e + 1;

>> -        if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {

>> +    /* domain */

>> +    if (col_s[2]) {

>> +        if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {

>>               goto inval;

>>           }

>> +        addr->domain = val;

>> +        col_s++;

>> +    } else {

>> +        addr->domain = 0;

>>       }

>> -    slot = val;

>>   

>> -    if (*e != '.') {

>> +    /* bus */

>> +    if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {

>>           goto inval;

>>       }

>> -    p = (char *)e + 1;

>> -    if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {

>> -        goto inval;

>> -    }

>> -    func = val;

>> +    addr->bus = val;

>>   

>> -    if (bus > 0xff) {

>> +    /* <slot>.<func> */

>> +    dot_s = g_strsplit(col_s[1], ".", 2);

>> +    if (!dot_s || !dot_s[0] || !dot_s[1]) {

>>           goto inval;

>>       }

>>   

>> -    if (*e) {

>> +    /* slot */

>> +    if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {

>>           goto inval;

>>       }

>> +    addr->slot = val;

>>   

>> -    addr->domain = dom;

>> -    addr->bus = bus;

>> -    addr->slot = slot;

>> -    addr->function = func;

>> +    /* func */

>> +    if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {

>> +        goto inval;

>> +    }

>> +    addr->function = val;

>>   

>> -    g_free(str);

>>       return;

>>   

>>   inval:

>>       error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);

>> -    g_free(str);

>>   }

>>   

>>   const PropertyInfo qdev_prop_pci_host_devaddr = {

>> -- 

>> 2.26.2

>>

> 

> Regards,

> Daniel

>
diff mbox series

Patch

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 49bdd125814..e6e081efd58 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -878,11 +878,11 @@  static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
     PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
-    char *str, *p;
-    const char *e;
+    g_autofree char *str = NULL;
+    g_autofree char **col_s0 = NULL;
+    g_autofree char **dot_s = NULL;
+    char **col_s;
     unsigned long val;
-    unsigned long dom = 0, bus = 0;
-    unsigned int slot = 0, func = 0;
 
     if (dev->realized) {
         qdev_prop_set_after_realize(dev, name, errp);
@@ -893,57 +893,50 @@  static void set_pci_host_devaddr(Object *obj, Visitor *v, const char *name,
         return;
     }
 
-    p = str;
-    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0xffff || e == p) {
+    col_s = col_s0 = g_strsplit(str, ":", 3);
+    if (!col_s || !col_s[0] || !col_s[1]) {
         goto inval;
     }
-    if (*e != ':') {
-        goto inval;
-    }
-    bus = val;
 
-    p = (char *)e + 1;
-    if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
-        goto inval;
-    }
-    if (*e == ':') {
-        dom = bus;
-        bus = val;
-        p = (char *)e + 1;
-        if (qemu_strtoul(p, &e, 16, &val) < 0 || val > 0x1f || e == p) {
+    /* domain */
+    if (col_s[2]) {
+        if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xffff) {
             goto inval;
         }
+        addr->domain = val;
+        col_s++;
+    } else {
+        addr->domain = 0;
     }
-    slot = val;
 
-    if (*e != '.') {
+    /* bus */
+    if (qemu_strtoul(col_s[0], NULL, 16, &val) < 0 || val > 0xff) {
         goto inval;
     }
-    p = (char *)e + 1;
-    if (qemu_strtoul(p, &e, 10, &val) < 0 || val > 7 || e == p) {
-        goto inval;
-    }
-    func = val;
+    addr->bus = val;
 
-    if (bus > 0xff) {
+    /* <slot>.<func> */
+    dot_s = g_strsplit(col_s[1], ".", 2);
+    if (!dot_s || !dot_s[0] || !dot_s[1]) {
         goto inval;
     }
 
-    if (*e) {
+    /* slot */
+    if (qemu_strtoul(dot_s[0], NULL, 16, &val) < 0 || val > 0x1f) {
         goto inval;
     }
+    addr->slot = val;
 
-    addr->domain = dom;
-    addr->bus = bus;
-    addr->slot = slot;
-    addr->function = func;
+    /* func */
+    if (qemu_strtoul(dot_s[1], NULL, 10, &val) < 0 || val > 7) {
+        goto inval;
+    }
+    addr->function = val;
 
-    g_free(str);
     return;
 
 inval:
     error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
-    g_free(str);
 }
 
 const PropertyInfo qdev_prop_pci_host_devaddr = {