Re: [Qemu-devel] [PATCH 5/6] hw/misc: add a TMP42{1, 2, 3} device model

[Cédric Le Goater]

On 06/02/2017 12:51 PM, Peter Maydell wrote:
> On 15 May 2017 at 06:51, Cédric Le Goater <address@hidden> wrote:
>> Largely inspired by the TMP105 temperature sensor, here is a model for
>> the TMP42{1,2,3} temperature sensors.
>>
>> Specs can be found here :
>>
>>         http://www.ti.com/lit/gpn/tmp421
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> This turns out to segfault on OSX and BSD...
> 
>> +static void tmp421_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> +    TMP421Class *sc = TMP421_CLASS(klass);
>> +
>> +    k->init = tmp421_init;
>> +    k->event = tmp421_event;
>> +    k->recv = tmp421_rx;
>> +    k->send = tmp421_tx;
>> +    dc->vmsd = &vmstate_tmp421;
>> +    sc->dev = (DeviceInfo *) data;
> 
> ...because this write to sc->dev is off the end of a
> malloced block. You can see this with valgrind on Linux:
> 
> $ valgrind ./build/x86/aarch64-softmmu/qemu-system-aarch64
> ==14009== Memcheck, a memory error detector
> [...]
> ==14009== Invalid write of size 8
> ==14009==    at 0x67D3D9: tmp421_class_init (tmp421.c:374)
> ==14009==    by 0x80CD51: type_initialize (object.c:334)
> ==14009==    by 0x80CABC: type_initialize (object.c:286)
> ==14009==    by 0x80DF49: object_class_foreach_tramp (object.c:805)
> ==14009==    by 0x1B7AE33F: g_hash_table_foreach (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
> ==14009==    by 0x80E028: object_class_foreach (object.c:827)
> ==14009==    by 0x80E1F7: object_class_get_list (object.c:881)
> ==14009==    by 0x5500A8: find_default_machine (vl.c:1488)
> ==14009==    by 0x554039: select_machine (vl.c:2745)
> ==14009==    by 0x55730E: main (vl.c:4091)
> ==14009==  Address 0x2b4e7cd0 is 0 bytes after a block of size 224 alloc'd
> ==14009==    at 0x4C2FB55: calloc (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==14009==    by 0x1B7C4770: g_malloc0 (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
> ==14009==    by 0x80CA8A: type_initialize (object.c:282)
> ==14009==    by 0x80CABC: type_initialize (object.c:286)
> ==14009==    by 0x80DF49: object_class_foreach_tramp (object.c:805)
> ==14009==    by 0x1B7AE33F: g_hash_table_foreach (in
> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2)
> ==14009==    by 0x80E028: object_class_foreach (object.c:827)
> ==14009==    by 0x80E1F7: object_class_get_list (object.c:881)
> ==14009==    by 0x5500A8: find_default_machine (vl.c:1488)
> ==14009==    by 0x554039: select_machine (vl.c:2745)
> ==14009==    by 0x55730E: main (vl.c:4091)
> ==14009==
> 
>> +}
>> +
>> +static const TypeInfo tmp421_info = {
>> +    .name          = TYPE_TMP421,
>> +    .parent        = TYPE_I2C_SLAVE,
>> +    .instance_size = sizeof(TMP421State),
>> +    .instance_init = tmp421_initfn,
>> +    .class_init    = tmp421_class_init,
> 
> ...which is because this TypeInfo doesn't set the
> .class_size field, so only sizeof(I2CSlaveClass) is
> allocated, not sizeof(TMP421Class).
> 
> (http://wiki.qemu.org/Documentation/QOMConventions
> lists this as one of the things you need to do for a class
> that has a class struct).
> 
>> +};
>> +
>> +static void tmp421_register_types(void)
>> +{
>> +    int i;
>> +
>> +    type_register_static(&tmp421_info);
>> +    for (i = 0; i < ARRAY_SIZE(devices); ++i) {
>> +        TypeInfo ti = {
>> +            .name       = devices[i].name,
>> +            .parent     = TYPE_TMP421,
>> +            .class_init = tmp421_class_init,
>> +            .class_data = (void *) &devices[i],
> 
> This TypeInfo is a bit odd too, looking more closely at it.
> It defines a type that's a subclass of TYPE_TMP421, but which
> has a class_init method that's the same function as TYPE_TMP421's
> class_init method. That means we call it twice for the same
> class, which doesn't seem right.
> 
>> +        };
>> +        type_register(&ti);
>> +    }
>> +}
>> +
>> +type_init(tmp421_register_types)
>> --
>> 2.7.4
> 
> I'm going to drop this patch and the next one from my arm
> queue, but leave 1-4 in.

OK. I should be able to fix it before 2.10. 

I would also like to spend some time on the rx8900 model Alastair 
wrote. It would give an RTC to the romulus machine. 

Thanks for the tests and analysis. 

C.