Re: [PATCH v2 2/2] hw: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan <pannengyuan@huawei.com>]

On 2/21/2020 1:56 AM, Peter Maydell wrote:
> On Mon, 17 Feb 2020 at 03:22, <pannengyuan@huawei.com> wrote:
>>
>> From: Pan Nengyuan <pannengyuan@huawei.com>
>>
>> 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 <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 
>> 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
> .
>