| Message ID | 20201013102252.3562860-1-philmd@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] hw/core/qdev-properties-system: Rewrite set_pci_host_devaddr using GLib | expand |
Ping for 5.2 as this is a bugfix. On 10/13/20 12:22 PM, 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> > --- > v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > --- > 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; > + g_auto(GStrv) 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 = { >
Cc'ing PCI developers (rc2 is scheduled for tomorrow). On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote: > Ping for 5.2 as this is a bugfix. > > On 10/13/20 12:22 PM, 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> >> --- >> v2: Free g_strsplit() with g_auto(GStrv) (Daniel) >> --- >> 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; >> + g_auto(GStrv) 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 = { >> >
ping??? On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote: > Cc'ing PCI developers (rc2 is scheduled for tomorrow). > > On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote: >> Ping for 5.2 as this is a bugfix. >> >> On 10/13/20 12:22 PM, 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> >>> --- >>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel) >>> --- >>> 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; >>> + g_auto(GStrv) 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 = { >>> >> >
On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote: > ping??? > > On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote: >> Cc'ing PCI developers (rc2 is scheduled for tomorrow). >> >> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote: >>> Ping for 5.2 as this is a bugfix. >>> >>> On 10/13/20 12:22 PM, 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(). As there is few interest in fixing the issue with this patch, let me remind the 2 other approaches: Klaus Herman: https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html Geoffrey McRae: https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html That said, I'm done with this issue for 5.2. Regards, Phil. >>>> 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> >>>> --- >>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel) >>>> --- >>>> 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; >>>> + g_auto(GStrv) 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 = { >>>> >>> >> >
On Fri, Nov 20, 2020 at 12:00:56PM +0100, Philippe Mathieu-Daudé wrote: > On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote: > > ping??? > > > > On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote: > >> Cc'ing PCI developers (rc2 is scheduled for tomorrow). > >> > >> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote: > >>> Ping for 5.2 as this is a bugfix. > >>> > >>> On 10/13/20 12:22 PM, 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(). > > As there is few interest in fixing the issue with this patch, > let me remind the 2 other approaches: > > Klaus Herman: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html > Geoffrey McRae: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html > > That said, I'm done with this issue for 5.2. > > Regards, > > Phil. What happens if we just revert bccb20c49df1bd683248a366021973901c11982f? This is what I'm inclined to do for 5.2 ... > >>>> 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> > >>>> --- > >>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > >>>> --- > >>>> 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; > >>>> + g_auto(GStrv) 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 = { > >>>> > >>> > >> > >
On Fri, Nov 20, 2020 at 1:55 PM Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Nov 20, 2020 at 12:00:56PM +0100, Philippe Mathieu-Daudé wrote: > > On 11/17/20 12:16 PM, Philippe Mathieu-Daudé wrote: > > > ping??? > > > > > > On 11/9/20 3:16 PM, Philippe Mathieu-Daudé wrote: > > >> Cc'ing PCI developers (rc2 is scheduled for tomorrow). > > >> > > >> On 11/7/20 9:59 AM, Philippe Mathieu-Daudé wrote: > > >>> Ping for 5.2 as this is a bugfix. > > >>> > > >>> On 10/13/20 12:22 PM, 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(). > > > > As there is few interest in fixing the issue with this patch, > > let me remind the 2 other approaches: > > > > Klaus Herman: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg750101.html > > Geoffrey McRae: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg758182.html > > > > That said, I'm done with this issue for 5.2. > > > > Regards, > > > > Phil. > > What happens if we just revert bccb20c49df1bd683248a366021973901c11982f? > > This is what I'm inclined to do for 5.2 ... Clever, fine by me. > > >>>> 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> > > >>>> --- > > >>>> v2: Free g_strsplit() with g_auto(GStrv) (Daniel) > > >>>> --- > > >>>> 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..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; > > >>>> + g_auto(GStrv) 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 = { > > >>>> > > >>> > > >> > > > >
diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 49bdd125814..36d4fd8b22a 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_auto(GStrv) col_s0 = NULL; + g_auto(GStrv) 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 = {
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> --- v2: Free g_strsplit() with g_auto(GStrv) (Daniel) --- hw/core/qdev-properties-system.c | 61 ++++++++++++++------------------ 1 file changed, 27 insertions(+), 34 deletions(-)