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

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

[Thomas Huth]

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
 
 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];
     }
 }
-- 
1.8.3.1

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

[Paolo Bonzini]

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];
>      }
>  }
> 

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

[Thomas Huth]

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

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

[Paolo Bonzini]

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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3129 bytes --]

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];
>      }
>  }

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Paolo Bonzini]

----- 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