[Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[john.liuli]

From: Li Liu <john.liuli@huawei.com>

Eeay to reproduce, just try "qemu -monitor stdio -nographic"
and type "quit", then the terminal will be crashed.

There are two pathes try to call tcgetattr of stdio in vl.c:

1) Monitor_parse(optarg, "readline");
   .....
   qemu_opts_foreach(qemu_find_opts("chardev"),
                     chardev_init_func, NULL, 1) != 0)

2) if (default_serial)
   add_device_config(DEV_SERIAL, "stdio");
   ....
   if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)

Both of them will trigger qemu_chr_open_stdio which will disable
ECHO attributes. First one has updated the attributes of stdio
by calling qemu_chr_fe_set_echo(chr, false). And the tty
attributes has been saved in oldtty. Then the second path will
redo such actions, and the oldtty is overlapped. So till "quit",
term_exit can't recove the correct attributes.

Signed-off-by: Li Liu <john.liuli@huawei.com>
---
 qemu-char.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d4f327a..941eb3e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1017,6 +1017,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
 /* init terminal so that we can grab keys */
 static struct termios oldtty;
 static int old_fd0_flags;
+static bool stdio_is_ready;
 static bool stdio_allow_signal;
 
 static void term_exit(void)
@@ -1060,10 +1061,15 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
         error_report("cannot use stdio with -daemonize");
         return NULL;
     }
-    old_fd0_flags = fcntl(0, F_GETFL);
-    tcgetattr (0, &oldtty);
-    qemu_set_nonblock(0);
-    atexit(term_exit);
+
+    if (!stdio_is_ready) {
+        stdio_is_ready = true;
+
+        old_fd0_flags = fcntl(0, F_GETFL);
+        tcgetattr(0, &oldtty);
+        qemu_set_nonblock(0);
+        atexit(term_exit);
+    }
 
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Markus Armbruster]

"john.liuli" <john.liuli@huawei.com> writes:

> From: Li Liu <john.liuli@huawei.com>
>
> Eeay to reproduce, just try "qemu -monitor stdio -nographic"
> and type "quit", then the terminal will be crashed.
>
> There are two pathes try to call tcgetattr of stdio in vl.c:
>
> 1) Monitor_parse(optarg, "readline");
>    .....
>    qemu_opts_foreach(qemu_find_opts("chardev"),
>                      chardev_init_func, NULL, 1) != 0)
>
> 2) if (default_serial)
>    add_device_config(DEV_SERIAL, "stdio");
>    ....
>    if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>
> Both of them will trigger qemu_chr_open_stdio which will disable
> ECHO attributes. First one has updated the attributes of stdio
> by calling qemu_chr_fe_set_echo(chr, false). And the tty
> attributes has been saved in oldtty. Then the second path will
> redo such actions, and the oldtty is overlapped. So till "quit",
> term_exit can't recove the correct attributes.
>
> Signed-off-by: Li Liu <john.liuli@huawei.com>

Yes, failure to restore tty settings is a bug.

But is having multiple character devices use the same terminal valid?
If no, can we catch and reject the attempt?

[...]

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Li Liu]

On 2014/8/27 14:44, Markus Armbruster wrote:
> "john.liuli" <john.liuli@huawei.com> writes:
> 
>> From: Li Liu <john.liuli@huawei.com>
>>
>> Eeay to reproduce, just try "qemu -monitor stdio -nographic"
>> and type "quit", then the terminal will be crashed.
>>
>> There are two pathes try to call tcgetattr of stdio in vl.c:
>>
>> 1) Monitor_parse(optarg, "readline");
>>    .....
>>    qemu_opts_foreach(qemu_find_opts("chardev"),
>>                      chardev_init_func, NULL, 1) != 0)
>>
>> 2) if (default_serial)
>>    add_device_config(DEV_SERIAL, "stdio");
>>    ....
>>    if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>
>> Both of them will trigger qemu_chr_open_stdio which will disable
>> ECHO attributes. First one has updated the attributes of stdio
>> by calling qemu_chr_fe_set_echo(chr, false). And the tty
>> attributes has been saved in oldtty. Then the second path will
>> redo such actions, and the oldtty is overlapped. So till "quit",
>> term_exit can't recove the correct attributes.
>>
>> Signed-off-by: Li Liu <john.liuli@huawei.com>
> 
> Yes, failure to restore tty settings is a bug.
> 
> But is having multiple character devices use the same terminal valid?

I'm not sure. But I have found such comments in vl.c
"According to documentation and historically, -nographic redirects
serial port, parallel port and monitor to stdio"

Best regards
Li.

> If no, can we catch and reject the attempt?
> 
> [...]
> 
> 

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Li Liu]

Ping, any more comments? Thanks.

On 2014/8/27 15:40, Li Liu wrote:
> 
> 
> On 2014/8/27 14:44, Markus Armbruster wrote:
>> "john.liuli" <john.liuli@huawei.com> writes:
>>
>>> From: Li Liu <john.liuli@huawei.com>
>>>
>>> Eeay to reproduce, just try "qemu -monitor stdio -nographic"
>>> and type "quit", then the terminal will be crashed.
>>>
>>> There are two pathes try to call tcgetattr of stdio in vl.c:
>>>
>>> 1) Monitor_parse(optarg, "readline");
>>>    .....
>>>    qemu_opts_foreach(qemu_find_opts("chardev"),
>>>                      chardev_init_func, NULL, 1) != 0)
>>>
>>> 2) if (default_serial)
>>>    add_device_config(DEV_SERIAL, "stdio");
>>>    ....
>>>    if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>>
>>> Both of them will trigger qemu_chr_open_stdio which will disable
>>> ECHO attributes. First one has updated the attributes of stdio
>>> by calling qemu_chr_fe_set_echo(chr, false). And the tty
>>> attributes has been saved in oldtty. Then the second path will
>>> redo such actions, and the oldtty is overlapped. So till "quit",
>>> term_exit can't recove the correct attributes.
>>>
>>> Signed-off-by: Li Liu <john.liuli@huawei.com>
>>
>> Yes, failure to restore tty settings is a bug.
>>
>> But is having multiple character devices use the same terminal valid?
> 
> I'm not sure. But I have found such comments in vl.c
> "According to documentation and historically, -nographic redirects
> serial port, parallel port and monitor to stdio"
> 
> Best regards
> Li.
> 
>> If no, can we catch and reject the attempt?
>>
>> [...]
>>
>>
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Markus Armbruster]

Li Liu <john.liuli@huawei.com> writes:

> Ping, any more comments? Thanks.

I'd like to hear Gerd's opinion (cc'ed).

> On 2014/8/27 15:40, Li Liu wrote:
>> 
>> 
>> On 2014/8/27 14:44, Markus Armbruster wrote:
>>> "john.liuli" <john.liuli@huawei.com> writes:
>>>
>>>> From: Li Liu <john.liuli@huawei.com>
>>>>
>>>> Eeay to reproduce, just try "qemu -monitor stdio -nographic"
>>>> and type "quit", then the terminal will be crashed.
>>>>
>>>> There are two pathes try to call tcgetattr of stdio in vl.c:
>>>>
>>>> 1) Monitor_parse(optarg, "readline");
>>>>    .....
>>>>    qemu_opts_foreach(qemu_find_opts("chardev"),
>>>>                      chardev_init_func, NULL, 1) != 0)
>>>>
>>>> 2) if (default_serial)
>>>>    add_device_config(DEV_SERIAL, "stdio");
>>>>    ....
>>>>    if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
>>>>
>>>> Both of them will trigger qemu_chr_open_stdio which will disable
>>>> ECHO attributes. First one has updated the attributes of stdio
>>>> by calling qemu_chr_fe_set_echo(chr, false). And the tty
>>>> attributes has been saved in oldtty. Then the second path will
>>>> redo such actions, and the oldtty is overlapped. So till "quit",
>>>> term_exit can't recove the correct attributes.
>>>>
>>>> Signed-off-by: Li Liu <john.liuli@huawei.com>
>>>
>>> Yes, failure to restore tty settings is a bug.
>>>
>>> But is having multiple character devices use the same terminal valid?
>> 
>> I'm not sure. But I have found such comments in vl.c
>> "According to documentation and historically, -nographic redirects
>> serial port, parallel port and monitor to stdio"
>> 
>> Best regards
>> Li.
>> 
>>> If no, can we catch and reject the attempt?
>>>
>>> [...]

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Gerd Hoffmann]

On Fr, 2014-09-05 at 11:04 +0200, Markus Armbruster wrote:
> Li Liu <john.liuli@huawei.com> writes:
> 
> > Ping, any more comments? Thanks.
> 
> I'd like to hear Gerd's opinion (cc'ed).
> 
> >>> But is having multiple character devices use the same terminal valid?

No (guess we should catch that case in stdio init).

Beside the tty initialization and cleanup you also have the problem that
both users are racing for input.  Well, maybe not in the qemu case as it
is the same process and it very well might be that it polls the two
chardevs in a well defined order, so one of them gets all input and the
other gets nothing.  With two processes reading from the terminal (try
'cat | less') it is actually random though.

> >> I'm not sure. But I have found such comments in vl.c
> >> "According to documentation and historically, -nographic redirects
> >> serial port, parallel port and monitor to stdio"

In that case mux chardev is used (that is the piece which handles the
input switching between serial and monitor via 'Ctrl-A c').  There is
one stdio instance, and one mux instance, the mux is chained to stdio,
and mux allows multiple backends to connect.

You can construct it on the command line this way:

qemu -nographic -nodefaults \
   -chardev stdio,mux=on,id=terminal \
   -serial chardev:terminal \
   -monitor chardev:terminal

[ serial is default, so no output here, unless you boot a guest
  with serial console configured ]

[ Hit 'Ctrl-A h' now ]

C-a h    print this help
C-a x    exit emulator
C-a s    save disk data back to file (if -snapshot)
C-a t    toggle console timestamps
C-a b    send break (magic sysrq)
C-a c    switch between console and monitor
C-a C-a  sends C-a

[ Hit 'Ctrl-A c' now ]

QEMU 2.1.50 monitor - type 'help' for more information
(qemu) info chardev
terminal: filename=mux
terminal-base: filename=stdio
(qemu) 

HTH,
  Gerd

Re: [Qemu-devel] [PATCH] qemu-char: fix terminal crash when using "-monitor stdio -nographic"

[Li Liu]

On 2014/9/5 17:31, Gerd Hoffmann wrote:
> On Fr, 2014-09-05 at 11:04 +0200, Markus Armbruster wrote:
>> Li Liu <john.liuli@huawei.com> writes:
>>
>>> Ping, any more comments? Thanks.
>>
>> I'd like to hear Gerd's opinion (cc'ed).
>>
>>>>> But is having multiple character devices use the same terminal valid?
> 
> No (guess we should catch that case in stdio init).
> 
> Beside the tty initialization and cleanup you also have the problem that
> both users are racing for input.  Well, maybe not in the qemu case as it
> is the same process and it very well might be that it polls the two
> chardevs in a well defined order, so one of them gets all input and the
> other gets nothing.  With two processes reading from the terminal (try
> 'cat | less') it is actually random though.
> 
>>>> I'm not sure. But I have found such comments in vl.c
>>>> "According to documentation and historically, -nographic redirects
>>>> serial port, parallel port and monitor to stdio"
> 
> In that case mux chardev is used (that is the piece which handles the
> input switching between serial and monitor via 'Ctrl-A c').  There is
> one stdio instance, and one mux instance, the mux is chained to stdio,
> and mux allows multiple backends to connect.
> 
> You can construct it on the command line this way:
> 
> qemu -nographic -nodefaults \
>    -chardev stdio,mux=on,id=terminal \
>    -serial chardev:terminal \
>    -monitor chardev:terminal
> 
> [ serial is default, so no output here, unless you boot a guest
>   with serial console configured ]
> 
> [ Hit 'Ctrl-A h' now ]
> 
> C-a h    print this help
> C-a x    exit emulator
> C-a s    save disk data back to file (if -snapshot)
> C-a t    toggle console timestamps
> C-a b    send break (magic sysrq)
> C-a c    switch between console and monitor
> C-a C-a  sends C-a
> 
> [ Hit 'Ctrl-A c' now ]
> 
> QEMU 2.1.50 monitor - type 'help' for more information
> (qemu) info chardev
> terminal: filename=mux
> terminal-base: filename=stdio
> (qemu) 
> 
> HTH,
>   Gerd
> 

Appreciate your detailed answer. Thank you very much.

Li.

> 
> 
> .
>