Re: [PATCH] hw/i386/pc: Fix crash that occurs when introspecting TYPE_PC_MACHINE machines

[Thomas Huth]

On 29/01/2025 08.11, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
>
> On 17/1/25 20:21, Thomas Huth wrote:
> > QEMU currently crashes when you try to inspect the machines based on
> > TYPE_PC_MACHINE for their properties:
> >
> >   $ echo '{ "execute": "qmp_capabilities" }
> >           { "execute": "qom-list-properties","arguments":
> >                        { "typename": "pc-q35-10.0-machine"}}' \
> >     | ./qemu-system-x86_64 -M pc -qmp stdio
> >   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 9},
> >    "package": "v9.2.0-1070-g87e115c122-dirty"}, "capabilities": ["oob"]}}
> >   {"return": {}}
> >   Segmentation fault (core dumped)
> >
> > This happens because TYPE_PC_MACHINE machines add a machine_init-
> > done_notifier in their instance_init function - but instance_init
> > of machines are not only called for machines that are realized,
> > but also for machines that are introspected, so in this case the
> > listener is added for a q35 machine that is never realized. But
> > since there is already a running pc machine, the listener function
> > is triggered immediately, causing a crash since it was not for the
> > right machine it was meant for.
> >
> > Such listener functions must never be installed from an instance_init
> > function. Let's do it from pc_basic_device_init() instead - this
> > function is called from the MachineClass->init() function instead,
> > i.e. guaranteed to be only called once in the lifetime of a QEMU
> > process.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2779
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   hw/i386/pc.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b46975c8a4..85b8a76455 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1241,6 +1241,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> >       /* Super I/O */
> >       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
> >                       pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
> > +
> > +    pcms->machine_done.notify = pc_machine_done;
>
> I could accept if we rename:
>
>    pc_machine_done() -> pc_basic_device_init_done[_notifier]()

Not sure whether that's such a good idea: The notifier is called when the 
machine init has been done, not when the device init has been done. It just 
happens to be instantiated from a function called pc_basic_device_init() 
now, but that does not really mean that the notifier function should get a 
similar name, should it?

 Thomas


> > +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
> >   }
> >   void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
> > @@ -1714,9 +1717,6 @@ static void pc_machine_initfn(Object *obj)
> >       if (pcmc->pci_enabled) {
> >           cxl_machine_init(obj, &pcms->cxl_devices_state);
> >       }
> > -
> > -    pcms->machine_done.notify = pc_machine_done;
> > -    qemu_add_machine_init_done_notifier(&pcms->machine_done);
> >   }
> >   static void pc_machine_reset(MachineState *machine, ResetType type)