| Message ID | 9009578ee10a50d994b2e10aa2840d73765f5968.1370577272.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 07, 2013 at 02:02:43PM +1000, peter.crosthwaite@xilinx.com wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue > where QEMU would segfault if you have an unattached Cadence UART. > > Fix by guarding the flush-on-reset logic on there being a qemu_chr > attachment. > > Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Tested-by: Soren Brinkmann <soren.brinkmann@xilinx.com> Thanks, Sören
On 7 June 2013 05:02, <peter.crosthwaite@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue > where QEMU would segfault if you have an unattached Cadence UART. > > Fix by guarding the flush-on-reset logic on there being a qemu_chr > attachment. > > Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > > hw/char/cadence_uart.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c > index c2a7834..29827d2 100644 > --- a/hw/char/cadence_uart.c > +++ b/hw/char/cadence_uart.c > @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s) > { > s->rx_wpos = 0; > s->rx_count = 0; > - qemu_chr_accept_input(s->chr); > + if (s->chr) { > + qemu_chr_accept_input(s->chr); > + } There seem to be a lot of references to s->chr in this device: is this really the only one which needs a guard against NULL? thanks -- PMM
Hi Peter, On Wed, Jun 26, 2013 at 3:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 June 2013 05:02, <peter.crosthwaite@xilinx.com> wrote: >> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> >> commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue >> where QEMU would segfault if you have an unattached Cadence UART. >> >> Fix by guarding the flush-on-reset logic on there being a qemu_chr >> attachment. >> >> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> >> hw/char/cadence_uart.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c >> index c2a7834..29827d2 100644 >> --- a/hw/char/cadence_uart.c >> +++ b/hw/char/cadence_uart.c >> @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s) >> { >> s->rx_wpos = 0; >> s->rx_count = 0; >> - qemu_chr_accept_input(s->chr); >> + if (s->chr) { >> + qemu_chr_accept_input(s->chr); >> + } > > There seem to be a lot of references to s->chr in this device: > is this really the only one which needs a guard against NULL? > Actually no, We have had some corner cases where other dereferences have segfaulted on us too. On my todo list to sweep and fix globally. But this is unique and a much bigger problem than the rest, as it is in the reset path. So even if the uart is present in system and unused it segafaults. Regards, Peter > thanks > -- PMM >
On 7 June 2013 05:02, <peter.crosthwaite@xilinx.com> wrote: > From: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue > where QEMU would segfault if you have an unattached Cadence UART. > > Fix by guarding the flush-on-reset logic on there being a qemu_chr > attachment. > > Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Applied to arm-devs.next. thanks -- PMM
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c index c2a7834..29827d2 100644 --- a/hw/char/cadence_uart.c +++ b/hw/char/cadence_uart.c @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s) { s->rx_wpos = 0; s->rx_count = 0; - qemu_chr_accept_input(s->chr); + if (s->chr) { + qemu_chr_accept_input(s->chr); + } s->r[R_SR] |= UART_SR_INTR_REMPTY; s->r[R_SR] &= ~UART_SR_INTR_RFUL;