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

[Peter Maydell]

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.

thanks
-- PMM