Re: [Qemu-devel] [PATCH] spapr-vty: Fix bad assert() statement

[David Gibson]

On Thu, Nov 10, 2016 at 10:06:37AM +0100, Thomas Huth wrote:
> When using the serial console in the GTK interface of QEMU (and
> QEMU has been compiled with CONFIG_VTE), it is possible to trigger
> the assert() statement in vty_receive() in spapr_vty.c by pasting
> a chunk of text with length > 16 into the QEMU window.
> Most of the other serial backends seem to simply drop characters
> that they can not handle, so I think we should also do the same in
> spapr-vty to fix this issue. And since it is quite ugly when pasted
> text is chopped after 16 bytes, we also increase the size of the
> input buffer here so that we can at least handle a couple of text
> lines.

Adding Paolo to CC, as qemu-char maintainer.

So, vastly increasing the buffer like this doesn't seem right - the
plain 16550 serial driver doesn't maintain a buffer bigger than its
small internal FIFO (32 characters IIRC) - or a single byte if the
FIFO is disabled.

I thought the other side of the char driver was supposed to call
can_receive() first and not deliver more bytes than we can handle -
hence the assert.

That said, I think vty_can_receive() has a bug - looking at other
drivers, I think it's supposed to return the amount of buffer space
available, but we're just returning 0 or 1.  Although AFAICT that
should still work, just inefficiently.

If you use a serial port with the same GTK interface, do you get the
same problem with pastes of >16 characters being truncated?

> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1639322
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  hw/char/spapr_vty.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 31822fe..bee6c34 100644
> --- a/hw/char/spapr_vty.c
> +++ b/hw/char/spapr_vty.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> @@ -7,7 +8,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_vio.h"
>  
> -#define VTERM_BUFSIZE   16
> +#define VTERM_BUFSIZE   2048
>  
>  typedef struct VIOsPAPRVTYDevice {
>      VIOsPAPRDevice sdev;
> @@ -37,7 +38,15 @@ static void vty_receive(void *opaque, const uint8_t *buf, 
> int size)
>          qemu_irq_pulse(spapr_vio_qirq(&dev->sdev));
>      }
>      for (i = 0; i < size; i++) {
> -        assert((dev->in - dev->out) < VTERM_BUFSIZE);
> +        if (dev->in - dev->out >= VTERM_BUFSIZE) {
> +            static bool reported;
> +            if (!reported) {
> +                error_report("VTY input buffer exhausted - characters 
> dropped."
> +                             " (input size = %i)", size);
> +                reported = true;
> +            }
> +            break;
> +        }
>          dev->buf[dev->in++ % VTERM_BUFSIZE] = buf[i];
>      }
>  }

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature