[Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[James Hogan]

Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an
infinite recursion occurs when putting the monitor on a pty (-monitor
pty) and connecting a terminal to the slave port.

This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to
qemu_chr_be_generic_open(). This event is captured by monitor_event()
which prints a welcome message to the character device. The flush of
that welcome message retriggers another open event in pty_chr_state()
because it checks s->connected, but only sets it to 1 after calling
qemu_chr_be_generic_open().

I've fixed this by setting s->connected = 1 before the call to
qemu_chr_be_generic_open() instead of after, so that the recursive
pty_chr_state() doesn't call it again.

An example snippet of repeating backtrace:
 ...
 #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288
 #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322
 #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n",
     ap=0x7f432be0) at qemu/monitor.c:339
 #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n")
     at qemu/monitor.c:347
 #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699
 #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108
 #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113
 #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145
 #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121
 #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
     at qemu/qemu-char.c:1063
 #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
     at qemu/qemu-char.c:118
 ...

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this
patch is acceptable it probably makes sense for it to follow the other
patch into stable too.

 qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 16f3ad7..1be1cf6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected)
             s->timer_tag = 0;
         }
         if (!s->connected) {
-            qemu_chr_be_generic_open(chr);
             s->connected = 1;
+            qemu_chr_be_generic_open(chr);
             s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
         }
     }
-- 
1.8.1.2

Re: [Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[Michael Roth]

Quoting James Hogan (2013-08-08 06:09:38)
> Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an
> infinite recursion occurs when putting the monitor on a pty (-monitor
> pty) and connecting a terminal to the slave port.
> 
> This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to
> qemu_chr_be_generic_open(). This event is captured by monitor_event()
> which prints a welcome message to the character device. The flush of
> that welcome message retriggers another open event in pty_chr_state()
> because it checks s->connected, but only sets it to 1 after calling
> qemu_chr_be_generic_open().
> 
> I've fixed this by setting s->connected = 1 before the call to
> qemu_chr_be_generic_open() instead of after, so that the recursive
> pty_chr_state() doesn't call it again.
> 
> An example snippet of repeating backtrace:
>  ...
>  #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288
>  #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322
>  #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n",
>      ap=0x7f432be0) at qemu/monitor.c:339
>  #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n")
>      at qemu/monitor.c:347
>  #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699
>  #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108
>  #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113
>  #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145
>  #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121
>  #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
>      at qemu/qemu-char.c:1063
>  #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
>      at qemu/qemu-char.c:118
>  ...
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>

Thanks for the catch!

I reproduced the bug and can confirm the this fixes it.

Prior to bd5c51e, the code relied on deferring CHR_EVENT_OPENED via BH to
ensure setting s->connected = 1 was visible by the time the event was
processed, so this patch simply makes that ordering explicit.

Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
> Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this
> patch is acceptable it probably makes sense for it to follow the other
> patch into stable too.
> 
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 16f3ad7..1be1cf6 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected)
>              s->timer_tag = 0;
>          }
>          if (!s->connected) {
> -            qemu_chr_be_generic_open(chr);
>              s->connected = 1;
> +            qemu_chr_be_generic_open(chr);
>              s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
>          }
>      }
> -- 
> 1.8.1.2

Re: [Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[Michael Roth]

Quoting Michael Roth (2013-08-08 09:03:08)
> Quoting James Hogan (2013-08-08 06:09:38)
> > Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an
> > infinite recursion occurs when putting the monitor on a pty (-monitor
> > pty) and connecting a terminal to the slave port.
> > 
> > This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to
> > qemu_chr_be_generic_open(). This event is captured by monitor_event()
> > which prints a welcome message to the character device. The flush of
> > that welcome message retriggers another open event in pty_chr_state()
> > because it checks s->connected, but only sets it to 1 after calling
> > qemu_chr_be_generic_open().
> > 
> > I've fixed this by setting s->connected = 1 before the call to
> > qemu_chr_be_generic_open() instead of after, so that the recursive
> > pty_chr_state() doesn't call it again.
> > 
> > An example snippet of repeating backtrace:
> >  ...
> >  #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288
> >  #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322
> >  #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n",
> >      ap=0x7f432be0) at qemu/monitor.c:339
> >  #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n")
> >      at qemu/monitor.c:347
> >  #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699
> >  #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108
> >  #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113
> >  #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145
> >  #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121
> >  #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
> >      at qemu/qemu-char.c:1063
> >  #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
> >      at qemu/qemu-char.c:118
> >  ...
> > 
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> 
> Thanks for the catch!
> 
> I reproduced the bug and can confirm the this fixes it.
> 
> Prior to bd5c51e, the code relied on deferring CHR_EVENT_OPENED via BH to
> ensure setting s->connected = 1 was visible by the time the event was
> processed, so this patch simply makes that ordering explicit.
> 
> Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> > ---
> > Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this
> > patch is acceptable it probably makes sense for it to follow the other
> > patch into stable too.

Agreed, CC'ing stable.

> > 
> >  qemu-char.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 16f3ad7..1be1cf6 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected)
> >              s->timer_tag = 0;
> >          }
> >          if (!s->connected) {
> > -            qemu_chr_be_generic_open(chr);
> >              s->connected = 1;
> > +            qemu_chr_be_generic_open(chr);
> >              s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
> >          }
> >      }
> > -- 
> > 1.8.1.2

Re: [Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[Michael Roth]

Quoting James Hogan (2013-08-08 06:09:38)
> Since commit bd5c51e (qemu-char: don't issue CHR_EVENT_OPEN in a BH), an
> infinite recursion occurs when putting the monitor on a pty (-monitor
> pty) and connecting a terminal to the slave port.
> 
> This is because of the qemu_chr_be_event(s, CHR_EVENT_OPENED) added to
> qemu_chr_be_generic_open(). This event is captured by monitor_event()
> which prints a welcome message to the character device. The flush of
> that welcome message retriggers another open event in pty_chr_state()
> because it checks s->connected, but only sets it to 1 after calling
> qemu_chr_be_generic_open().
> 
> I've fixed this by setting s->connected = 1 before the call to
> qemu_chr_be_generic_open() instead of after, so that the recursive
> pty_chr_state() doesn't call it again.
> 
> An example snippet of repeating backtrace:
>  ...
>  #107486 0x007aec58 in monitor_flush (mon=0xf418b0) at qemu/monitor.c:288
>  #107487 0x007aee7c in monitor_puts (mon=0xf418b0, str=0x1176d07 "") at qemu/monitor.c:322
>  #107488 0x007aef20 in monitor_vprintf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n",
>      ap=0x7f432be0) at qemu/monitor.c:339
>  #107489 0x007aefac in monitor_printf (mon=0xf418b0, fmt=0x8d4820 "QEMU %s monitor - type 'help' for more information\n")
>      at qemu/monitor.c:347
>  #107490 0x007ba4bc in monitor_event (opaque=0xf418b0, event=2) at qemu/monitor.c:4699
>  #107491 0x00684c28 in qemu_chr_be_event (s=0xf37788, event=2) at qemu/qemu-char.c:108
>  #107492 0x00684c70 in qemu_chr_be_generic_open (s=0xf37788) at qemu/qemu-char.c:113
>  #107493 0x006880a4 in pty_chr_state (chr=0xf37788, connected=1) at qemu/qemu-char.c:1145
>  #107494 0x00687fa4 in pty_chr_update_read_handler (chr=0xf37788) at qemu/qemu-char.c:1121
>  #107495 0x00687c9c in pty_chr_write (chr=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
>      at qemu/qemu-char.c:1063
>  #107496 0x00684cc4 in qemu_chr_fe_write (s=0xf37788, buf=0x70b3c008 <Address 0x70b3c008 out of bounds>, len=538720)
>      at qemu/qemu-char.c:118
>  ...
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
> Note the commit I mentioned Cc's gnu-stable@nongnu.org. Assuming this
> patch is acceptable it probably makes sense for it to follow the other
> patch into stable too.

Ping. Looking to pull this in for stable.

> 
>  qemu-char.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 16f3ad7..1be1cf6 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1142,8 +1142,8 @@ static void pty_chr_state(CharDriverState *chr, int connected)
>              s->timer_tag = 0;
>          }
>          if (!s->connected) {
> -            qemu_chr_be_generic_open(chr);
>              s->connected = 1;
> +            qemu_chr_be_generic_open(chr);
>              s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
>          }
>      }
> -- 
> 1.8.1.2

Re: [Qemu-devel] [PATCH] qemu-char: fix infinite recursion connecting to monitor pty

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori