hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------ 1 file changed, 27 insertions(+), 34 deletions(-)
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;
+ 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
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 :|
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 >
© 2016 - 2024 Red Hat, Inc.