| Message ID | 1478768797-26401-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/11/2016 10:06, 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. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1639322 > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > 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 This change causes a change in the migration protocol. Paolo > > 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]; > } > } >
On 10.11.2016 15:41, Paolo Bonzini wrote: > > > On 10/11/2016 10:06, 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. >> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1639322 >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> 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 > > This change causes a change in the migration protocol. Bummer! You're right, the buffer is listed in the vmstate_spapr_vty description ... so please ignore this patch, I've got to think about a better solution here... Thomas
On 10/11/2016 18:41, Thomas Huth wrote: > On 10.11.2016 15:41, Paolo Bonzini wrote: >> >> >> On 10/11/2016 10:06, 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. >>> >>> Buglink: https://bugs.launchpad.net/qemu/+bug/1639322 >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> 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 >> >> This change causes a change in the migration protocol. > > Bummer! You're right, the buffer is listed in the vmstate_spapr_vty > description ... so please ignore this patch, I've got to think about a > better solution here... Well, the assert change is still good and should be in 2.8. Paolo
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 <thuth@redhat.com> > --- > 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]; > } > }
On 11.11.2016 00:01, David Gibson wrote: > 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. Right. The real bug seems to be on the front end side: gd_vc_in() in ui/gtk.c calls qemu_chr_be_write() without checking the qemu_chr_be_can_write() first (which internally calls the can_receive() function of the backend). So we likely need to fix gd_vc_in() to use qemu_chr_be_can_write() instead ... but I'm currently puzzled whether that can be done at all, since this seems to be a callback function - and we likely can not simply loop/yield here, can we? > 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. Right, it should return the amount of free space instead. > If you use a serial port with the same GTK interface, do you get the > same problem with pastes of >16 characters being truncated? Yes, I tried the coldfire image from http://wiki.qemu.org/download/coldfire-test-0.1.tar.bz2 and that even only accepts 1 byte when doing the copy-n-paste and drops everything else. So copy-n-paste is currently completely broken there, too. Thomas >> Buglink: https://bugs.launchpad.net/qemu/+bug/1639322
----- Original Message ----- > From: "Thomas Huth" <thuth@redhat.com> > To: "David Gibson" <david@gibson.dropbear.id.au> > Cc: qemu-ppc@nongnu.org, "Alexander Graf" <agraf@suse.de>, qemu-devel@nongnu.org, pbonzini@redhat.com, "Gerd > Hoffmann" <kraxel@redhat.com> > Sent: Friday, November 11, 2016 9:13:00 AM > Subject: Re: [PATCH] spapr-vty: Fix bad assert() statement > > On 11.11.2016 00:01, David Gibson wrote: > > 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. > > Right. The real bug seems to be on the front end side: > > gd_vc_in() in ui/gtk.c calls qemu_chr_be_write() without checking the > qemu_chr_be_can_write() first (which internally calls the can_receive() > function of the backend). > > So we likely need to fix gd_vc_in() to use qemu_chr_be_can_write() > instead ... but I'm currently puzzled whether that can be done at all, > since this seems to be a callback function - and we likely can not > simply loop/yield here, can we? You'd have to use a buffer in the GTK+ backend, in order to have working cut and paste. But at least using qemu_chr_be_can_write, and limiting the size of the written data to the return value, would fix the assertion. > > 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. > > Right, it should return the amount of free space instead. Yes. Paolo
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]; } }
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. Buglink: https://bugs.launchpad.net/qemu/+bug/1639322 Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/char/spapr_vty.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)