Re: [PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Volker Rümelin]

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_qemu@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.

With best regards,
Volker

>
>> +        error_setg(errp, "Initializing audio stream failed");
>> +        return;
>> +    }
>> +
>>       s->shift = 1;
>>       s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
>>       s->mixbuf = g_malloc0(s->samples << s->shift);
>
>
> ATB,
>
> Mark.
>