Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
On 11/13/2015 01:36 PM, Andreas Färber wrote: > Am 13.11.2015 um 19:32 schrieb John Snow: >> On 11/12/2015 12:41 PM, Andreas Färber wrote: >>> [...] Testing >>> got stuck in ahci though, investigating. >>> >>> Thanks, >>> Andreas >>> >> >> Did you ever reproduce this, or does it seem to just be a race? > > Once I updated to a later git commit I was no longer able to reproduce > that hang to date. So it could either be a hard-to-reproduce race, or > some merge yesterday made it go away. > > Cheers, > Andreas > FWIW, on Fedora 22, I can't get my 64 or 32 bit build of the i386 target to reproduce the behavior after a couple dozen runs. I was using the same commit you pointed to, so it seems like a race is likely. I'll keep my eye open. --js
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
Am 13.11.2015 um 19:32 schrieb John Snow: > On 11/12/2015 12:41 PM, Andreas Färber wrote: >> [...] Testing >> got stuck in ahci though, investigating. >> >> Thanks, >> Andreas >> > > Did you ever reproduce this, or does it seem to just be a race? Once I updated to a later git commit I was no longer able to reproduce that hang to date. So it could either be a hard-to-reproduce race, or some merge yesterday made it go away. Cheers, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
On 11/12/2015 12:41 PM, Andreas Färber wrote: > Am 11.11.2015 um 09:54 schrieb Markus Armbruster: >> Peter Maydell writes: >>> On 25 August 2015 at 15:17, Markus Armbruster wrote: Stumbled over this while throwing away old mail. Andreas, what do you think? >>> >>> Seems right to me -- I suspect the original properties code was >>> written with the assumption that the property field would be >>> inside the device struct (and so offsets are small). The array >>> properties code breaks that assumption by allocating a separate >>> lump of memory with the properties in it; so now there's no >>> guarantee that the two pointers being subtracted will be >>> within 4G of each other. >>> >>> Reviewed-by: Peter Maydell >>> >>> Arguably for consistency the 'arrayoffset' struct member should >>> also be a ptrdiff_t, though our current uses of it are such >>> that it'll always be within int range. >> >> Andreas? > > Found it archived. I honestly don't think it's necessary in practice to > have 64-bit offsets on 64-bit host, but it builds okay, queued. Testing > got stuck in ahci though, investigating. > > Thanks, > Andreas > Did you ever reproduce this, or does it seem to just be a race?
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
Am 11.11.2015 um 09:54 schrieb Markus Armbruster: > Peter Maydell writes: >> On 25 August 2015 at 15:17, Markus Armbruster wrote: >>> Stumbled over this while throwing away old mail. Andreas, what do you >>> think? >> >> Seems right to me -- I suspect the original properties code was >> written with the assumption that the property field would be >> inside the device struct (and so offsets are small). The array >> properties code breaks that assumption by allocating a separate >> lump of memory with the properties in it; so now there's no >> guarantee that the two pointers being subtracted will be >> within 4G of each other. >> >> Reviewed-by: Peter Maydell >> >> Arguably for consistency the 'arrayoffset' struct member should >> also be a ptrdiff_t, though our current uses of it are such >> that it'll always be within int range. > > Andreas? Found it archived. I honestly don't think it's necessary in practice to have 64-bit offsets on 64-bit host, but it builds okay, queued. Testing got stuck in ahci though, investigating. Thanks, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
Peter Maydell writes: > On 25 August 2015 at 15:17, Markus Armbruster wrote: >> Stumbled over this while throwing away old mail. Andreas, what do you >> think? > > Seems right to me -- I suspect the original properties code was > written with the assumption that the property field would be > inside the device struct (and so offsets are small). The array > properties code breaks that assumption by allocating a separate > lump of memory with the properties in it; so now there's no > guarantee that the two pointers being subtracted will be > within 4G of each other. > > Reviewed-by: Peter Maydell > > Arguably for consistency the 'arrayoffset' struct member should > also be a ptrdiff_t, though our current uses of it are such > that it'll always be within int range. Andreas?
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
On 25 August 2015 at 15:17, Markus Armbruster wrote: > Stumbled over this while throwing away old mail. Andreas, what do you > think? Seems right to me -- I suspect the original properties code was written with the assumption that the property field would be inside the device struct (and so offsets are small). The array properties code breaks that assumption by allocating a separate lump of memory with the properties in it; so now there's no guarantee that the two pointers being subtracted will be within 4G of each other. Reviewed-by: Peter Maydell Arguably for consistency the 'arrayoffset' struct member should also be a ptrdiff_t, though our current uses of it are such that it'll always be within int range. -- PMM
Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
Stumbled over this while throwing away old mail. Andreas, what do you
think?
Ildar Isaev writes:
> 'offset' field in struct Property is calculated as a diff between two
> pointers (hw/core/qdev-properties.c:802)
>
> arrayprop->prop.offset = eltptr - (void *)dev;
>
> If offset is declared as int, this subtraction can cause type overflow
> thus leading to the fall of the subsequent assert
> (hw/core/qdev-properties.c:803)
>
> assert(qdev_get_prop_ptr(dev, &arrayprop->prop) == eltptr);
>
> So ptrdiff_t should be used instead
>
> Signed-off-by: Ildar Isaev
> ---
> include/hw/qdev-core.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4e673f9..f0e2a73 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -224,7 +224,7 @@ struct BusState {
> struct Property {
> const char *name;
> PropertyInfo *info;
> -int offset;
> +ptrdiff_toffset;
> uint8_t bitnr;
> uint8_t qtype;
> int64_t defval;
[Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow
'offset' field in struct Property is calculated as a diff between two pointers
(hw/core/qdev-properties.c:802)
arrayprop->prop.offset = eltptr - (void *)dev;
If offset is declared as int, this subtraction can cause type overflow
thus leading to the fall of the subsequent assert
(hw/core/qdev-properties.c:803)
assert(qdev_get_prop_ptr(dev, &arrayprop->prop) == eltptr);
So ptrdiff_t should be used instead
Signed-off-by: Ildar Isaev
---
include/hw/qdev-core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..f0e2a73 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -224,7 +224,7 @@ struct BusState {
struct Property {
const char *name;
PropertyInfo *info;
-int offset;
+ptrdiff_toffset;
uint8_t bitnr;
uint8_t qtype;
int64_t defval;
--
1.9.3
