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

[Qemu-devel] [PATCH v2 for-2.8] 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.

Buglink: https://bugs.launchpad.net/qemu/+bug/1639322
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Do not increase the buffer size since this breaks migration.
   (We should figure out a better solution for the size of the
    buffer once 2.8 is released)

 hw/char/spapr_vty.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
index 31822fe..06b9b39 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"
@@ -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 v2 for-2.8] spapr-vty: Fix bad assert() statement

[David Gibson]

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

On Thu, Nov 10, 2016 at 09:16:01PM +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.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1639322
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Do not increase the buffer size since this breaks migration.
>    (We should figure out a better solution for the size of the
>     buffer once 2.8 is released)

Applied to ppc-for-2.8. since actually hitting the assert is clearly
bad.  I still think this smells like a bug on the other side of the
chardev, feeding in more characters than the can_receive() hook says
can be supplied.

> 
>  hw/char/spapr_vty.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/char/spapr_vty.c b/hw/char/spapr_vty.c
> index 31822fe..06b9b39 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"
> @@ -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 --]