| Message ID | 1425478186-18894-1-git-send-email-ild@inbox.ru |
|---|---|
| State | New |
| Headers | show |
Stumbled over this while throwing away old mail. Andreas, what do you think? Ildar Isaev <ild@inbox.ru> 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 <ild@inbox.ru> > --- > 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_t offset; > uint8_t bitnr; > uint8_t qtype; > int64_t defval;
On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> 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 <peter.maydell@linaro.org> 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
Peter Maydell <peter.maydell@linaro.org> writes: > On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> 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 <peter.maydell@linaro.org> > > 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?
Am 11.11.2015 um 09:54 schrieb Markus Armbruster: > Peter Maydell <peter.maydell@linaro.org> writes: >> On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> 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 <peter.maydell@linaro.org> >> >> 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
On 11/12/2015 12:41 PM, Andreas Färber wrote: > Am 11.11.2015 um 09:54 schrieb Markus Armbruster: >> Peter Maydell <peter.maydell@linaro.org> writes: >>> On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> 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 <peter.maydell@linaro.org> >>> >>> 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?
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
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
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_t offset; uint8_t bitnr; uint8_t qtype; int64_t defval;
'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 <ild@inbox.ru> --- include/hw/qdev-core.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)