Am 13.05.25 um 10:17 schrieb Mark Cave-Ayland:
> On 13/05/2025 07:14, Volker Rümelin wrote:
>
>> Am 11.05.25 um 13:52 schrieb Mark Cave-Ayland:
>>> On 11/05/2025 08:38, Volker Rümelin wrote:
>>>
>>>> AUD_open_out() may fail and return NULL. This may then lead to
>>>> a segmentation fault in memset() below. The memset() behaviour
>>>> is undefined if the pointer to the destination object is a null
>>>> pointer.
>>>>
>>>> Add the missing error handling code.
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_q...@t-online.de>
>>>> ---
>>>> hw/audio/asc.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
>>>> index 18382ccf6a..03dee0fcc7 100644
>>>> --- a/hw/audio/asc.c
>>>> +++ b/hw/audio/asc.c
>>>> @@ -12,6 +12,7 @@
>>>> #include "qemu/osdep.h"
>>>> #include "qemu/timer.h"
>>>> +#include "qapi/error.h"
>>>> #include "hw/sysbus.h"
>>>> #include "hw/irq.h"
>>>> #include "audio/audio.h"
>>>> @@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error
>>>> **errp)
>>>> s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s,
>>>> asc_out_cb,
>>>> &as);
>>>> + if (!s->voice) {
>>>> + asc_unrealize(dev);
>>>
>>> I don't think it is ever right for a device to unrealize itself. If
>>> the aim is to handle cleanup, then since s->mixbuf and s->silentbuf
>>> are yet to be allocated by this point then I think you can simply call
>>> error_setg() and return.
>>
>> Hi Mark,
>>
>> yes my aim was to handle cleanup. When calling asc_unrealize() I don't
>> have to think about which steps are necessary. In this case I would have
>> to call AUD_remove_card(&s->card). Therefore, I would like to keep my
>> patch version.
>
> Hi Volker,
>
> I can see why you want to call a single routine to handle tidy-up,
> however it is still expected that the various qdev device callbacks
> are only called internally by the qdev APIs. For example when
> debugging lifecycle issues you typically put breakpoints on
> init/realize/unrealize/finalize to figure out why something is not
> working as expected.
>
> A common example of this is reset in that a chip can be reset by qdev,
> but also by writing to a reset register via MMIO: in this case both
> the Resettable interface and the MMIO access simply call the same
> internal reset function.
>
> You could have a separate cleanup function and call it for the error
> case here, but since the qdev device callbacks are called in a strict
> order, you can guarantee that s->mixbuf and s->silentbuf will be unset
> at this point. So it's probably easiest just to call
> AUD_remove_card(&s->card) before the return and that guarantees that
> you won't interfere with any internal qdev management logic.
Ok, thanks for the explanation. I will send a version 2 patch series.
With best regards,
Volker
