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.
ATB,
Mark.
