Re: [Qemu-devel] [PATCH v5 2/5] Add Error **errp for xen_host_pci_device_get()
On 01/16/2016 12:41 AM, Eric Blake wrote: On 01/14/2016 08:11 PM, Cao jin wrote: buf[rc] = 0; -rc = qemu_strtoul(buf, , base, ); -if (!rc) { -*pvalue = value; +rc = qemu_strtoul(buf, , base, (unsigned long *)pvalue); Ouch. Casting unsigned int * to unsigned long * and then dereferencing it is bogus (you end up having qemu_strtoul() write beyond bounds on platforms where long is larger than int). Yes, I considered this issue a little. Because the current condition is: the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So I guess even if on x86_64(length of int != long), it won`t break things. So, compared with following, which style do you prefer? Maybe: rc = qemu_strtoul(buf, , base, ); if (rc) { assert(value < UINT_MAX); *pvalue = value; } else { report error ... } And maybe some of it should even be done as part of the conversion to qemu_strtoul() in 1/5. Thanks for the example, will give v6 soon. -- Yours Sincerely, Cao jin
Re: [Qemu-devel] [PATCH v5 2/5] Add Error **errp for xen_host_pci_device_get()
On 01/14/2016 08:11 PM, Cao jin wrote: >>> buf[rc] = 0; >>> -rc = qemu_strtoul(buf, , base, ); >>> -if (!rc) { >>> -*pvalue = value; >>> +rc = qemu_strtoul(buf, , base, (unsigned long *)pvalue); >> >> Ouch. Casting unsigned int * to unsigned long * and then dereferencing >> it is bogus (you end up having qemu_strtoul() write beyond bounds on >> platforms where long is larger than int). > > Yes, I considered this issue a little. Because the current condition is: > the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So > I guess even if on x86_64(length of int != long), it won`t break things. > So, compared with following, which style do you prefer? Maybe: rc = qemu_strtoul(buf, , base, ); if (rc) { assert(value < UINT_MAX); *pvalue = value; } else { report error ... } And maybe some of it should even be done as part of the conversion to qemu_strtoul() in 1/5. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 2/5] Add Error **errp for xen_host_pci_device_get()
On 01/13/2016 05:51 AM, Cao jin wrote: > To catch the error msg. Also modify the caller > > Signed-off-by: Cao jin> --- > hw/xen/xen-host-pci-device.c | 142 > +-- > hw/xen/xen-host-pci-device.h | 5 +- > hw/xen/xen_pt.c | 13 ++-- > 3 files changed, 80 insertions(+), 80 deletions(-) > > diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c > index 351b61a..3e22de8 100644 > --- a/hw/xen/xen-host-pci-device.c > +++ b/hw/xen/xen-host-pci-device.c > @@ -31,25 +31,20 @@ > #define IORESOURCE_PREFETCH 0x1000 /* No side effects */ > #define IORESOURCE_MEM_64 0x0010 > > -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d, > - const char *name, char *buf, ssize_t size) > +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d, > +const char *name, char *buf, ssize_t > size) Changing xen_host_pci_sysfs_path() to return void, by assert()ing on caller error, is not mentioned in the commit message; and if I were doing the series, I probably would have done it as a separate commit. > /* This size should be enough to read a long from a file */ > #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22 > -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, > - unsigned int *pvalue, int base) > +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, > + unsigned int *pvalue, int base, Error > **errp) > { > buf[rc] = 0; > -rc = qemu_strtoul(buf, , base, ); > -if (!rc) { > -*pvalue = value; > +rc = qemu_strtoul(buf, , base, (unsigned long *)pvalue); Ouch. Casting unsigned int * to unsigned long * and then dereferencing it is bogus (you end up having qemu_strtoul() write beyond bounds on platforms where long is larger than int). You'll need to revert this part of the patch, and stick with *pvalue = value (and possibly even add a bounds check that value <= UINT_MAX). Otherwise looks okay. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v5 2/5] Add Error **errp for xen_host_pci_device_get()
On 01/15/2016 06:29 AM, Eric Blake wrote: On 01/13/2016 05:51 AM, Cao jin wrote: To catch the error msg. Also modify the caller Signed-off-by: Cao jin--- hw/xen/xen-host-pci-device.c | 142 +-- hw/xen/xen-host-pci-device.h | 5 +- hw/xen/xen_pt.c | 13 ++-- 3 files changed, 80 insertions(+), 80 deletions(-) diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c index 351b61a..3e22de8 100644 --- a/hw/xen/xen-host-pci-device.c +++ b/hw/xen/xen-host-pci-device.c @@ -31,25 +31,20 @@ #define IORESOURCE_PREFETCH 0x1000 /* No side effects */ #define IORESOURCE_MEM_64 0x0010 -static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d, - const char *name, char *buf, ssize_t size) +static void xen_host_pci_sysfs_path(const XenHostPCIDevice *d, +const char *name, char *buf, ssize_t size) Changing xen_host_pci_sysfs_path() to return void, by assert()ing on caller error, is not mentioned in the commit message; and if I were doing the series, I probably would have done it as a separate commit. Thanks for the suggestion, will split it out. /* This size should be enough to read a long from a file */ #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22 -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, - unsigned int *pvalue, int base) +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name, + unsigned int *pvalue, int base, Error **errp) { buf[rc] = 0; -rc = qemu_strtoul(buf, , base, ); -if (!rc) { -*pvalue = value; +rc = qemu_strtoul(buf, , base, (unsigned long *)pvalue); Ouch. Casting unsigned int * to unsigned long * and then dereferencing it is bogus (you end up having qemu_strtoul() write beyond bounds on platforms where long is larger than int). Yes, I considered this issue a little. Because the current condition is: the value it want to get won`t exceed 4 byte (vendor/device ID, etc). So I guess even if on x86_64(length of int != long), it won`t break things. So, compared with following, which style do you prefer? You'll need to revert this part of the patch, and stick with *pvalue = value (and possibly even add a bounds check that value <= UINT_MAX). Otherwise looks okay. -- Yours Sincerely, Cao jin