[Qemu-devel] [PATCH 0/2] pci devaddr parsing cleanups

[Qemu-devel] [PATCH 0/2] pci devaddr parsing cleanups

[Michael S. Tsirkin]

Michael S. Tsirkin (2):
  pci: don't export an internal function
  pci: rewrite devaddr parsing

 hw/pci.c |   83 +++++++++++++++++++++++++++++--------------------------------
 hw/pci.h |    2 -
 2 files changed, 39 insertions(+), 46 deletions(-)

-- 
1.7.9.111.gf3fb0

[Qemu-devel] [PATCH 1/2] pci: don't export an internal function

[Michael S. Tsirkin]

Make an internal function, pci_parse_devaddr,
static.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |    2 +-
 hw/pci.h |    2 --
 2 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index f580bb5..5827c0e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -478,7 +478,7 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
  * Parse [[<domain>:]<bus>:]<slot>, return -1 on error if funcp == NULL
  *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
  */
-int pci_parse_devaddr(const char *addr, int *domp, int *busp,
+static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
                       unsigned int *slotp, unsigned int *funcp)
 {
     const char *p;
diff --git a/hw/pci.h b/hw/pci.h
index 355c905..fd40b5e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -311,8 +311,6 @@ PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
 int pci_qdev_find_device(const char *id, PCIDevice **pdev);
 PCIBus *pci_get_bus_devfn(int *devfnp, const char *devaddr);
 
-int pci_parse_devaddr(const char *addr, int *domp, int *busp,
-                      unsigned int *slotp, unsigned int *funcp);
 int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
                      unsigned *slotp);
 
-- 
1.7.9.111.gf3fb0

[Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

Use scanf instead of manual string scanning.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c |   81 +++++++++++++++++++++++++++++---------------------------------
 1 files changed, 38 insertions(+), 43 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 5827c0e..a8c0b69 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -479,61 +479,56 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
  *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
  */
 static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
-                      unsigned int *slotp, unsigned int *funcp)
-{
-    const char *p;
-    char *e;
-    unsigned long val;
-    unsigned long dom = 0, bus = 0;
-    unsigned int slot = 0;
-    unsigned int func = 0;
-
-    p = addr;
-    val = strtoul(p, &e, 16);
-    if (e == p)
-	return -1;
-    if (*e == ':') {
-	bus = val;
-	p = e + 1;
-	val = strtoul(p, &e, 16);
-	if (e == p)
-	    return -1;
-	if (*e == ':') {
-	    dom = bus;
-	    bus = val;
-	    p = e + 1;
-	    val = strtoul(p, &e, 16);
-	    if (e == p)
-		return -1;
-	}
-    }
-
-    slot = val;
-
-    if (funcp != NULL) {
-        if (*e != '.')
-            return -1;
+                             unsigned int *slotp, unsigned int *funcp)
+{
+    unsigned dom, bus, slot, func;
+    int n = -1;
+
+    /* Parse [[<domain>:]<bus>:]<slot> */
+    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
+    if (n == -1) {
+        dom = 0;
+        sscanf(addr, "%x:%x%n", &bus, &slot, &n);
+        if (n == -1) {
+            bus = 0;
+            sscanf(addr, "%x%n", &slot, &n);
+            if (n == -1) {
+                return -1;
+            }
+        }
+    }
 
-        p = e + 1;
-        val = strtoul(p, &e, 16);
-        if (e == p)
+    /* Parse the optional .func */
+    addr += n;
+    if (!*addr) {
+        func = 0;
+    } else {
+        sscanf(addr, ".%x%n", &func, &n);
+        if (n == -1) {
             return -1;
+        }
+        addr += n;
+    }
 
-        func = val;
+    /* Anything left? Malformed input. */
+    if (*addr) {
+        return -1;
     }
 
-    /* if funcp == NULL func is 0 */
-    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
-	return -1;
+    if (funcp == NULL) {
+        func = 0;
+    }
 
-    if (*e)
+    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
 	return -1;
+    }
 
     *domp = dom;
     *busp = bus;
     *slotp = slot;
-    if (funcp != NULL)
+    if (funcp != NULL) {
         *funcp = func;
+    }
     return 0;
 }
 
-- 
1.7.9.111.gf3fb0

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[malc]

On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:

> Use scanf instead of manual string scanning.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pci.c |   81 +++++++++++++++++++++++++++++---------------------------------
>  1 files changed, 38 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 5827c0e..a8c0b69 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -479,61 +479,56 @@ static void pci_set_default_subsystem_id(PCIDevice *pci_dev)
>   *       [[<domain>:]<bus>:]<slot>.<func>, return -1 on error
>   */
>  static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> -                      unsigned int *slotp, unsigned int *funcp)
> -{
> -    const char *p;
> -    char *e;
> -    unsigned long val;
> -    unsigned long dom = 0, bus = 0;
> -    unsigned int slot = 0;
> -    unsigned int func = 0;
> -
> -    p = addr;
> -    val = strtoul(p, &e, 16);
> -    if (e == p)
> -	return -1;
> -    if (*e == ':') {
> -	bus = val;
> -	p = e + 1;
> -	val = strtoul(p, &e, 16);
> -	if (e == p)
> -	    return -1;
> -	if (*e == ':') {
> -	    dom = bus;
> -	    bus = val;
> -	    p = e + 1;
> -	    val = strtoul(p, &e, 16);
> -	    if (e == p)
> -		return -1;
> -	}
> -    }
> -
> -    slot = val;
> -
> -    if (funcp != NULL) {
> -        if (*e != '.')
> -            return -1;
> +                             unsigned int *slotp, unsigned int *funcp)
> +{
> +    unsigned dom, bus, slot, func;
> +    int n = -1;
> +
> +    /* Parse [[<domain>:]<bus>:]<slot> */
> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);

sscanf can fail.

> +    if (n == -1) {
> +        dom = 0;
> +        sscanf(addr, "%x:%x%n", &bus, &slot, &n);
> +        if (n == -1) {
> +            bus = 0;
> +            sscanf(addr, "%x%n", &slot, &n);
> +            if (n == -1) {
> +                return -1;
> +            }
> +        }
> +    }
>  
> -        p = e + 1;
> -        val = strtoul(p, &e, 16);
> -        if (e == p)
> +    /* Parse the optional .func */
> +    addr += n;
> +    if (!*addr) {
> +        func = 0;
> +    } else {
> +        sscanf(addr, ".%x%n", &func, &n);
> +        if (n == -1) {
>              return -1;
> +        }
> +        addr += n;
> +    }
>  
> -        func = val;
> +    /* Anything left? Malformed input. */
> +    if (*addr) {
> +        return -1;
>      }
>  
> -    /* if funcp == NULL func is 0 */
> -    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
> -	return -1;
> +    if (funcp == NULL) {
> +        func = 0;
> +    }
>  
> -    if (*e)
> +    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) {
>  	return -1;
> +    }
>  
>      *domp = dom;
>      *busp = bus;
>      *slotp = slot;
> -    if (funcp != NULL)
> +    if (funcp != NULL) {
>          *funcp = func;
> +    }
>      return 0;
>  }
>  
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On 02/16/2012 12:23 PM, malc wrote:
> On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
> 
>> Use scanf instead of manual string scanning.
>>
>> +
>> +    /* Parse [[<domain>:]<bus>:]<slot> */
>> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> 
> sscanf can fail.

Worse, the *scanf family has undefined behavior on integer overflow.  If
addr contains "100000000000000:0:0", there's no telling whether it will
be diagnosed as a parse error, or silently accepted and truncated, in
which case, there's no telling what dom will contain.

I cringe any time I see someone using scanf to parse numbers from
arbitrary user input; I barely tolerate it for parsing things generated
by the kernel, but even there, I won't ever use scanf myself.  Same goes
for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
into integers.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[malc]

On Thu, 16 Feb 2012, Eric Blake wrote:

> On 02/16/2012 12:23 PM, malc wrote:
> > On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
> > 
> >> Use scanf instead of manual string scanning.
> >>
> >> +
> >> +    /* Parse [[<domain>:]<bus>:]<slot> */
> >> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> > 
> > sscanf can fail.
> 
> Worse, the *scanf family has undefined behavior on integer overflow.  If
> addr contains "100000000000000:0:0", there's no telling whether it will
> be diagnosed as a parse error, or silently accepted and truncated, in
> which case, there's no telling what dom will contain.
> 
> I cringe any time I see someone using scanf to parse numbers from
> arbitrary user input; I barely tolerate it for parsing things generated
> by the kernel, but even there, I won't ever use scanf myself.  Same goes
> for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
> into integers.

To whomever wanted to apply this - don't.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

On Thu, Feb 16, 2012 at 11:23:40PM +0400, malc wrote:
> > +{
> > +    unsigned dom, bus, slot, func;
> > +    int n = -1;
> > +
> > +    /* Parse [[<domain>:]<bus>:]<slot> */
> > +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> 
> sscanf can fail.
> 
> > +    if (n == -1) {


If it does n will stay -1.

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[malc]

On Fri, 17 Feb 2012, Michael S. Tsirkin wrote:

> On Thu, Feb 16, 2012 at 11:23:40PM +0400, malc wrote:
> > > +{
> > > +    unsigned dom, bus, slot, func;
> > > +    int n = -1;
> > > +
> > > +    /* Parse [[<domain>:]<bus>:]<slot> */
> > > +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> > 
> > sscanf can fail.
> > 
> > > +    if (n == -1) {
> 
> 
> If it does n will stay -1.
> 

You are right. That does not, however, invalidate the points made
by Eric Blake.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

On Thu, Feb 16, 2012 at 01:39:02PM -0700, Eric Blake wrote:
> On 02/16/2012 12:23 PM, malc wrote:
> > On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
> > 
> >> Use scanf instead of manual string scanning.
> >>
> >> +
> >> +    /* Parse [[<domain>:]<bus>:]<slot> */
> >> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> > 
> > sscanf can fail.
> 
> Worse, the *scanf family has undefined behavior on integer overflow.  If
> addr contains "100000000000000:0:0", there's no telling whether it will
> be diagnosed as a parse error, or silently accepted and truncated, in
> which case, there's no telling what dom will contain.
> 
> I cringe any time I see someone using scanf to parse numbers from
> arbitrary user input; I barely tolerate it for parsing things generated
> by the kernel, but even there, I won't ever use scanf myself.
> Same goes
> for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
> into integers.

Seems easy to fix: I'll just set maximum field width of 8.
Any other issues?


> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

On Fri, Feb 17, 2012 at 01:50:41PM +0400, malc wrote:
> On Fri, 17 Feb 2012, Michael S. Tsirkin wrote:
> 
> > On Thu, Feb 16, 2012 at 11:23:40PM +0400, malc wrote:
> > > > +{
> > > > +    unsigned dom, bus, slot, func;
> > > > +    int n = -1;
> > > > +
> > > > +    /* Parse [[<domain>:]<bus>:]<slot> */
> > > > +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> > > 
> > > sscanf can fail.
> > > 
> > > > +    if (n == -1) {
> > 
> > 
> > If it does n will stay -1.
> > 
> 
> You are right. That does not, however, invalidate the points made
> by Eric Blake.


Yes. But I think
+    sscanf(addr, "%8x:%8x:%8x%n", &dom, &bus, &slot, &n);

addresses his points.


> -- 
> mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Feb 16, 2012 at 01:39:02PM -0700, Eric Blake wrote:
>> On 02/16/2012 12:23 PM, malc wrote:
>> > On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
>> > 
>> >> Use scanf instead of manual string scanning.
>> >>
>> >> +
>> >> +    /* Parse [[<domain>:]<bus>:]<slot> */
>> >> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
>> > 
>> > sscanf can fail.
>> 
>> Worse, the *scanf family has undefined behavior on integer overflow.  If
>> addr contains "100000000000000:0:0", there's no telling whether it will
>> be diagnosed as a parse error, or silently accepted and truncated, in
>> which case, there's no telling what dom will contain.
>> 
>> I cringe any time I see someone using scanf to parse numbers from
>> arbitrary user input; I barely tolerate it for parsing things generated
>> by the kernel, but even there, I won't ever use scanf myself.
>> Same goes
>> for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
>> into integers.
>
> Seems easy to fix: I'll just set maximum field width of 8.

Nope.  Functional change: "000000001.2" is no longer accepted.

> Any other issues?

Yes.

1. More functional changes, e.g.

   * "1" is no longer rejected when funcp != NULL

   Probably more.  I'd be particularly wary of sscanf()'s appetite for
   space.

2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)

Why bother?

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

On Fri, Feb 17, 2012 at 01:35:02PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 16, 2012 at 01:39:02PM -0700, Eric Blake wrote:
> >> On 02/16/2012 12:23 PM, malc wrote:
> >> > On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
> >> > 
> >> >> Use scanf instead of manual string scanning.
> >> >>
> >> >> +
> >> >> +    /* Parse [[<domain>:]<bus>:]<slot> */
> >> >> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> >> > 
> >> > sscanf can fail.
> >> 
> >> Worse, the *scanf family has undefined behavior on integer overflow.  If
> >> addr contains "100000000000000:0:0", there's no telling whether it will
> >> be diagnosed as a parse error, or silently accepted and truncated, in
> >> which case, there's no telling what dom will contain.
> >> 
> >> I cringe any time I see someone using scanf to parse numbers from
> >> arbitrary user input; I barely tolerate it for parsing things generated
> >> by the kernel, but even there, I won't ever use scanf myself.
> >> Same goes
> >> for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
> >> into integers.
> >
> > Seems easy to fix: I'll just set maximum field width of 8.
> 
> Nope.  Functional change: "000000001.2" is no longer accepted.

Hmm. Why would anyone do this?
But I get the hint I think. So I think what I will do is,
use sscanf to validate, detect the format and tokenize input,
then strtoul to read in integers.



> > Any other issues?
> 
> Yes.
> 
> 1. More functional changes, e.g.
> 
>    * "1" is no longer rejected when funcp != NULL

I think it's a bug. Why would we want to reject it?

>    Probably more.  I'd be particularly wary of sscanf()'s appetite for
>    space.

We pass in a pre-parsed string. How can it have spaces?

> 2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)
> 
> Why bother?

I need to extend the functionality and manual parsing
makes it very fragile.

-- 
MST

Re: [Qemu-devel] [PATCH 2/2] pci: rewrite devaddr parsing

[Michael S. Tsirkin]

On Fri, Feb 17, 2012 at 01:35:02PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Thu, Feb 16, 2012 at 01:39:02PM -0700, Eric Blake wrote:
> >> On 02/16/2012 12:23 PM, malc wrote:
> >> > On Thu, 16 Feb 2012, Michael S. Tsirkin wrote:
> >> > 
> >> >> Use scanf instead of manual string scanning.
> >> >>
> >> >> +
> >> >> +    /* Parse [[<domain>:]<bus>:]<slot> */
> >> >> +    sscanf(addr, "%x:%x:%x%n", &dom, &bus, &slot, &n);
> >> > 
> >> > sscanf can fail.
> >> 
> >> Worse, the *scanf family has undefined behavior on integer overflow.  If
> >> addr contains "100000000000000:0:0", there's no telling whether it will
> >> be diagnosed as a parse error, or silently accepted and truncated, in
> >> which case, there's no telling what dom will contain.
> >> 
> >> I cringe any time I see someone using scanf to parse numbers from
> >> arbitrary user input; I barely tolerate it for parsing things generated
> >> by the kernel, but even there, I won't ever use scanf myself.
> >> Same goes
> >> for atoi.  _Only_ strtol and friends can robustly parse arbitrary input
> >> into integers.
> >
> > Seems easy to fix: I'll just set maximum field width of 8.
> 
> Nope.  Functional change: "000000001.2" is no longer accepted.

OK, there's an easy way to fix with
%*[0]%8x

> > Any other issues?
> 
> Yes.
> 
> 1. More functional changes, e.g.
> 
>    * "1" is no longer rejected when funcp != NULL
> 
>    Probably more.  I'd be particularly wary of sscanf()'s appetite for
>    space.

BTW strtoul that we currently use skips whitespace
silently too. We probably should validate input with
%*[0-9a-f.:] beforehand to address this.

> 2. Diffstat: 1 files changed, 38 insertions(+), 43 deletions(-)
> 
> Why bother?