[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoi
From: |
Pan Nengyuan |
Subject: |
Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks |
Date: |
Fri, 21 Feb 2020 11:37:53 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 2/21/2020 1:56 AM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 03:22, <address@hidden> wrote:
>>
>> From: Pan Nengyuan <address@hidden>
>>
>> There are some memleaks when we call 'device_list_properties'. This patch
>> move timer_new from init into realize to fix it.
>> Meanwhile, do the null check in mos6522_reset() to avoid null deref if we
>> move timer_new into realize().
>>
>> Reported-by: Euler Robot <address@hidden>
>> Signed-off-by: Pan Nengyuan <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>
>
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 19e154b870..980eda7599 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -465,11 +465,15 @@ static void mos6522_reset(DeviceState *dev)
>> s->timers[0].frequency = s->frequency;
>> s->timers[0].latch = 0xffff;
>> set_counter(s, &s->timers[0], 0xffff);
>> - timer_del(s->timers[0].timer);
>> + if (s->timers[0].timer) {
>> + timer_del(s->timers[0].timer);
>> + }
>>
>> s->timers[1].frequency = s->frequency;
>> s->timers[1].latch = 0xffff;
>> - timer_del(s->timers[1].timer);
>> + if (s->timers[1].timer) {
>> + timer_del(s->timers[1].timer);
>> + }
>> }
>
> What code path calls a device 'reset' method on a device
> that has not yet been realized ? I wasn't expecting that
> to be valid...
I got the follow null-deref case on m68k If I move timer_new into realize():
#0 0x55cbb0d3e9f9 in timer_del /mnt/sdb/qemu-new/qemu/util/qemu-timer.c:429
#1 0x55cbb04f3abe in mos6522_reset
/mnt/sdb/qemu-new/qemu/hw/misc/mos6522.c:468
#2 0x55cbb02b5fd5 in mos6522_q800_via2_reset
/mnt/sdb/qemu-new/qemu/hw/misc/mac_via.c:1098
#3 0x55cbb047b926 in device_transitional_reset
/mnt/sdb/qemu-new/qemu/hw/core/qdev.c:1136
#4 0x55cbb0491a71 in resettable_phase_hold
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:182
#5 0x55cbb048700e in bus_reset_child_foreach
/mnt/sdb/qemu-new/qemu/hw/core/bus.c:94
#6 0x55cbb0490f66 in resettable_child_foreach
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:96
#7 0x55cbb0491896 in resettable_phase_hold
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:173
#8 0x55cbb0490c06 in resettable_assert_reset
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:60
#9 0x55cbb0490aec in resettable_reset
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:45
#10 0x55cbb0492668 in resettable_cold_reset_fn
/mnt/sdb/qemu-new/qemu/hw/core/resettable.c:269
#11 0x55cbb0494a04 in qemu_devices_reset
/mnt/sdb/qemu-new/qemu/hw/core/reset.c:69
#12 0x55cbb03ab91d in qemu_system_reset /mnt/sdb/qemu-new/qemu/vl.c:1412
#13 0x55cbb03bfe04 in main /mnt/sdb/qemu-new/qemu/vl.c:4403
mos6522_init was called in mac_via_realize as follow, but mos6522_realize was
not called at all.
So maybe we shouldn't move it into realize or add realize step in this code
path?
#0 0x0000555555789e40 in mos6522_init (obj=0x555557537b00) at
/mnt/sdb/qemu-new/qemu/hw/misc/mos6522.c:476
#1 0x000055555581b6c3 in object_init_with_type (obj=0x555557537b00,
ti=0x55555617c2b0) at /mnt/sdb/qemu-new/qemu/qom/object.c:372
#2 0x000055555581cc80 in object_initialize_with_type
(data=data@entry=0x555557537b00, size=1504, type=0x55555617c2b0) at
/mnt/sdb/qemu-new/qemu/qom/object.c:516
#3 0x000055555581cd1f in object_initialize
(data=data@entry=0x555557537b00, size=<optimized out>, typename=<optimized
out>) at /mnt/sdb/qemu-new/qemu/qom/object.c:529
#4 0x000055555581e387 in object_initialize_childv
(parentobj=0x555557537510, propname=0x555555a3c673 "via1",
childobj=0x555557537b00, size=<optimized out>, type=<optimized out>,
errp=0x55555613b338 <error_abort>, vargs=0x7fffffffdb30)
at /mnt/sdb/qemu-new/qemu/qom/object.c:552
#5 0x000055555581e4d3 in object_initialize_child
(parentobj=<optimized out>, propname=<optimized out>,
childobj=childobj@entry=0x555557537b00, size=<optimized out>, type=<optimized
out>, errp=<optimized out>) at /mnt/sdb/qemu-new/qemu/qom/object.c:539
#6 0x000055555577ba88 in sysbus_init_child_obj (parent=<optimized out>,
childname=<optimized out>, child=0x555557537b00, childsize=<optimized out>,
childtype=<optimized out>)
at /mnt/sdb/qemu-new/qemu/hw/core/sysbus.c:352
#7 0x000055555570d301 in mac_via_realize (dev=0x555557537510,
errp=0x7fffffffdce0) at /mnt/sdb/qemu-new/qemu/hw/misc/mac_via.c:876
#8 0x0000555555774444 in device_set_realized (obj=0x555557537510,
value=<optimized out>, errp=0x7fffffffddd0) at
/mnt/sdb/qemu-new/qemu/hw/core/qdev.c:891
#9 0x000055555581b266 in property_set_bool (obj=0x555557537510,
v=<optimized out>, name=<optimized out>, opaque=0x555556165f50,
errp=0x7fffffffddd0) at /mnt/sdb/qemu-new/qemu/qom/object.c:2238
#10 0x000055555581feee in object_property_set_qobject (obj=0x555557537510,
value=<optimized out>, name=0x555555a5fa67 "realized", errp=0x7fffffffddd0) at
/mnt/sdb/qemu-new/qemu/qom/qom-qobject.c:26
#11 0x000055555581d60f in object_property_set_bool (obj=0x555557537510,
value=<optimized out>, name=0x555555a5fa67 "realized", errp=0x7fffffffddd0) at
/mnt/sdb/qemu-new/qemu/qom/object.c:1390
#12 0x0000555555773381 in qdev_init_nofail (dev=dev@entry=0x555557537510)
at /mnt/sdb/qemu-new/qemu/hw/core/qdev.c:418
#13 0x0000555555711fcd in q800_init (machine=<optimized out>) at
/mnt/sdb/qemu-new/qemu/hw/m68k/q800.c:230
#14 0x0000555555686dfb in main (argc=<optimized out>, argv=<optimized out>,
envp=<optimized out>) at /mnt/sdb/qemu-new/qemu/vl.c:4308
And I have another quesion, how to distinguish whether the realize() will be
called or not.
Thanks.
>
> thanks
> -- PMM
> .
>