* [Qemu-devel] [PATCH 0/2] Fix use after free with mux
@ 2016-10-03 9:47 Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 1/2] char: update read handler in all cases Marc-André Lureau
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-03 9:47 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, kraxel, Marc-André Lureau
Hi,
When qemu with muxed monitor quits, it leads to invalid use after free:
valgrind qemu -chardev stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
==4306== Invalid read of size 8
==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
==4306== by 0x7E39F8: json_message_parser_destroy (json-streamer.c:134)
==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
==4306== by 0x495A98: main (vl.c:4675)
==4306== Address 0x28439e90 is 112 bytes inside a block of size 240 free'd
==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==4306== by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
==4306== by 0x495A93: main (vl.c:4674)
==4306== Block was alloc'd at
==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==4306== by 0x1E4CBE18: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.2)
==4306== by 0x344BF8: monitor_init (monitor.c:4021)
==4306== by 0x49063C: mon_init_func (vl.c:2417)
==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
==4306== by 0x4954E0: main (vl.c:4473)
...
The following two patches fix this by unregistering the muxed chr handlers.
Marc-André Lureau (2):
char: update read handler in all cases
char: use a fixed idx for child muxed chr
qemu-char.c | 24 ++++++++++++++++--------
include/sysemu/char.h | 1 +
2 files changed, 17 insertions(+), 8 deletions(-)
--
2.10.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 1/2] char: update read handler in all cases
2016-10-03 9:47 [Qemu-devel] [PATCH 0/2] Fix use after free with mux Marc-André Lureau
@ 2016-10-03 9:47 ` Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr Marc-André Lureau
2016-10-03 9:56 ` [Qemu-devel] [PATCH 0/2] Fix use after free with mux Paolo Bonzini
2 siblings, 0 replies; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-03 9:47 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, kraxel, Marc-André Lureau
In commit ac1b84dd1 (rhbz#1027181), a check was added to only update the
"read handler" when the front-end is opened, because the read callbacks
were not restored when a device is plugged. However, this seems not
correct, the handler is correctly set back on hotplug (in
virtconsole_realize) and the bug can no longer be reproduced.
Calling chr_update_read_handler() allows to fix the mux driver to stop
calling the child handlers (which may be going to be destroyed).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qemu-char.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-char.c b/qemu-char.c
index fb456ce..f7a07e6 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -468,7 +468,7 @@ void qemu_chr_add_handlers_full(CharDriverState *s,
s->chr_read = fd_read;
s->chr_event = fd_event;
s->handler_opaque = opaque;
- if (fe_open && s->chr_update_read_handler) {
+ if (s->chr_update_read_handler) {
s->chr_update_read_handler(s, context);
}
--
2.10.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-03 9:47 [Qemu-devel] [PATCH 0/2] Fix use after free with mux Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 1/2] char: update read handler in all cases Marc-André Lureau
@ 2016-10-03 9:47 ` Marc-André Lureau
2016-10-11 12:20 ` Claudio Imbrenda
2016-10-03 9:56 ` [Qemu-devel] [PATCH 0/2] Fix use after free with mux Paolo Bonzini
2 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-03 9:47 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, kraxel, Marc-André Lureau
mux_chr_update_read_handler() is adding a new mux_cnt each time
mux_chr_update_read_handler() is called, it's not possible to actually
update the "child" chr callbacks that were set previously. This may lead
to crashes if the "child" chr is destroyed:
valgrind x86_64-softmmu/qemu-system-x86_64 -chardev
stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
when quitting:
==4306== Invalid read of size 8
==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
==4306== by 0x7E39F8: json_message_parser_destroy (json-streamer.c:134)
==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
==4306== by 0x495A98: main (vl.c:4675)
==4306== Address 0x28439e90 is 112 bytes inside a block of size 240 free'd
==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==4306== by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
==4306== by 0x495A93: main (vl.c:4674)
==4306== Block was alloc'd at
==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==4306== by 0x1E4CBE18: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.2)
==4306== by 0x344BF8: monitor_init (monitor.c:4021)
==4306== by 0x49063C: mon_init_func (vl.c:2417)
==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
==4306== by 0x4954E0: main (vl.c:4473)
Instead, keep the "child" chr associated with a particular idx so its
handlers can be updated and removed to avoid the crash.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
qemu-char.c | 22 +++++++++++++++-------
include/sysemu/char.h | 1 +
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index f7a07e6..4b330ea 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -165,6 +165,7 @@ CharDriverState *qemu_chr_alloc(ChardevCommon *backend, Error **errp)
CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
qemu_mutex_init(&chr->chr_write_lock);
+ chr->mux_idx = -1;
if (backend->has_logfile) {
int flags = O_WRONLY | O_CREAT;
if (backend->has_logappend &&
@@ -738,17 +739,25 @@ static void mux_chr_update_read_handler(CharDriverState *chr,
GMainContext *context)
{
MuxDriver *d = chr->opaque;
+ int idx;
if (d->mux_cnt >= MAX_MUX) {
fprintf(stderr, "Cannot add I/O handlers, MUX array is full\n");
return;
}
- d->ext_opaque[d->mux_cnt] = chr->handler_opaque;
- d->chr_can_read[d->mux_cnt] = chr->chr_can_read;
- d->chr_read[d->mux_cnt] = chr->chr_read;
- d->chr_event[d->mux_cnt] = chr->chr_event;
+
+ if (chr->mux_idx == -1) {
+ chr->mux_idx = d->mux_cnt++;
+ }
+
+ idx = chr->mux_idx;
+ d->ext_opaque[idx] = chr->handler_opaque;
+ d->chr_can_read[idx] = chr->chr_can_read;
+ d->chr_read[idx] = chr->chr_read;
+ d->chr_event[idx] = chr->chr_event;
+
/* Fix up the real driver with mux routines */
- if (d->mux_cnt == 0) {
+ if (d->mux_cnt == 1) {
qemu_chr_add_handlers_full(d->drv, mux_chr_can_read,
mux_chr_read,
mux_chr_event,
@@ -757,8 +766,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr,
if (d->focus != -1) {
mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
}
- d->focus = d->mux_cnt;
- d->mux_cnt++;
+ d->focus = idx;
mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
}
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0d0465a..4593576 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -92,6 +92,7 @@ struct CharDriverState {
int explicit_be_open;
int avail_connections;
int is_mux;
+ int mux_idx;
guint fd_in_tag;
QemuOpts *opts;
bool replay;
--
2.10.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Fix use after free with mux
2016-10-03 9:47 [Qemu-devel] [PATCH 0/2] Fix use after free with mux Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 1/2] char: update read handler in all cases Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr Marc-André Lureau
@ 2016-10-03 9:56 ` Paolo Bonzini
2016-10-03 10:08 ` Marc-André Lureau
2 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2016-10-03 9:56 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: kraxel
On 03/10/2016 11:47, Marc-André Lureau wrote:
> Hi,
>
> When qemu with muxed monitor quits, it leads to invalid use after free:
>
> valgrind qemu -chardev stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
>
> ==4306== Invalid read of size 8
> ==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
> ==4306== by 0x7E39F8: json_message_parser_destroy (json-streamer.c:134)
> ==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
> ==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
> ==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
> ==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
> ==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
> ==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
> ==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
> ==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
> ==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
> ==4306== by 0x495A98: main (vl.c:4675)
> ==4306== Address 0x28439e90 is 112 bytes inside a block of size 240 free'd
> ==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> ==4306== by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
> ==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
> ==4306== by 0x495A93: main (vl.c:4674)
> ==4306== Block was alloc'd at
> ==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> ==4306== by 0x1E4CBE18: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.2)
> ==4306== by 0x344BF8: monitor_init (monitor.c:4021)
> ==4306== by 0x49063C: mon_init_func (vl.c:2417)
> ==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
> ==4306== by 0x4954E0: main (vl.c:4473)
> ...
>
> The following two patches fix this by unregistering the muxed chr handlers.
If I read the code right, patch 1 without patch 2 can cause the mux_cnt
to overflow the size of the d->chr_* arrays. Ok to invert the order?
Thanks,
Paolo
>
> Marc-André Lureau (2):
> char: update read handler in all cases
> char: use a fixed idx for child muxed chr
>
> qemu-char.c | 24 ++++++++++++++++--------
> include/sysemu/char.h | 1 +
> 2 files changed, 17 insertions(+), 8 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] Fix use after free with mux
2016-10-03 9:56 ` [Qemu-devel] [PATCH 0/2] Fix use after free with mux Paolo Bonzini
@ 2016-10-03 10:08 ` Marc-André Lureau
0 siblings, 0 replies; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-03 10:08 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: kraxel
Hi
On Mon, Oct 3, 2016 at 1:57 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 03/10/2016 11:47, Marc-André Lureau wrote:
> > Hi,
> >
> > When qemu with muxed monitor quits, it leads to invalid use after free:
> >
> > valgrind qemu -chardev stdio,mux=on,id=char0 -mon
> chardev=char0,mode=control,default
> >
> > ==4306== Invalid read of size 8
> > ==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
> > ==4306== by 0x7E39F8: json_message_parser_destroy
> (json-streamer.c:134)
> > ==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
> > ==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
> > ==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
> > ==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
> > ==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
> > ==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
> > ==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
> > ==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
> > ==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
> > ==4306== by 0x495A98: main (vl.c:4675)
> > ==4306== Address 0x28439e90 is 112 bytes inside a block of size 240
> free'd
> > ==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> > ==4306== by 0x1E4CBF2D: g_free (in
> /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
> > ==4306== by 0x495A93: main (vl.c:4674)
> > ==4306== Block was alloc'd at
> > ==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> > ==4306== by 0x1E4CBE18: g_malloc (in
> /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306== by 0x344BF8: monitor_init (monitor.c:4021)
> > ==4306== by 0x49063C: mon_init_func (vl.c:2417)
> > ==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
> > ==4306== by 0x4954E0: main (vl.c:4473)
> > ...
> >
> > The following two patches fix this by unregistering the muxed chr
> handlers.
>
> If I read the code right, patch 1 without patch 2 can cause the mux_cnt
> to overflow the size of the d->chr_* arrays. Ok to invert the order?
>
>
Makes sense, thanks
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-03 9:47 ` [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr Marc-André Lureau
@ 2016-10-11 12:20 ` Claudio Imbrenda
2016-10-11 14:28 ` Marc-André Lureau
0 siblings, 1 reply; 10+ messages in thread
From: Claudio Imbrenda @ 2016-10-11 12:20 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: pbonzini, kraxel
Hi,
I noticed that this patch kills the Qemu monitor for me. If I start a
text-mode guest with -nographic, then I can't switch to the monitor any
longer with Ctrl+a c . The guest works otherwise, e.g. I can login from
the console.
Tested on s390, but I think it's a more general issue, since the patch
is not arch-dependent.
On 03/10/16 11:47, Marc-André Lureau wrote:
> mux_chr_update_read_handler() is adding a new mux_cnt each time
> mux_chr_update_read_handler() is called, it's not possible to actually
> update the "child" chr callbacks that were set previously. This may lead
> to crashes if the "child" chr is destroyed:
>
> valgrind x86_64-softmmu/qemu-system-x86_64 -chardev
> stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
>
> when quitting:
>
> ==4306== Invalid read of size 8
> ==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
> ==4306== by 0x7E39F8: json_message_parser_destroy (json-streamer.c:134)
> ==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
> ==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
> ==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
> ==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
> ==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
> ==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
> ==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
> ==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
> ==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
> ==4306== by 0x495A98: main (vl.c:4675)
> ==4306== Address 0x28439e90 is 112 bytes inside a block of size 240 free'd
> ==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> ==4306== by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
> ==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
> ==4306== by 0x495A93: main (vl.c:4674)
> ==4306== Block was alloc'd at
> ==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> ==4306== by 0x1E4CBE18: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.2)
> ==4306== by 0x344BF8: monitor_init (monitor.c:4021)
> ==4306== by 0x49063C: mon_init_func (vl.c:2417)
> ==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
> ==4306== by 0x4954E0: main (vl.c:4473)
>
> Instead, keep the "child" chr associated with a particular idx so its
> handlers can be updated and removed to avoid the crash.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> qemu-char.c | 22 +++++++++++++++-------
> include/sysemu/char.h | 1 +
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index f7a07e6..4b330ea 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -165,6 +165,7 @@ CharDriverState *qemu_chr_alloc(ChardevCommon *backend, Error **errp)
> CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
> qemu_mutex_init(&chr->chr_write_lock);
>
> + chr->mux_idx = -1;
> if (backend->has_logfile) {
> int flags = O_WRONLY | O_CREAT;
> if (backend->has_logappend &&
> @@ -738,17 +739,25 @@ static void mux_chr_update_read_handler(CharDriverState *chr,
> GMainContext *context)
> {
> MuxDriver *d = chr->opaque;
> + int idx;
>
> if (d->mux_cnt >= MAX_MUX) {
> fprintf(stderr, "Cannot add I/O handlers, MUX array is full\n");
> return;
> }
> - d->ext_opaque[d->mux_cnt] = chr->handler_opaque;
> - d->chr_can_read[d->mux_cnt] = chr->chr_can_read;
> - d->chr_read[d->mux_cnt] = chr->chr_read;
> - d->chr_event[d->mux_cnt] = chr->chr_event;
> +
> + if (chr->mux_idx == -1) {
> + chr->mux_idx = d->mux_cnt++;
> + }
> +
> + idx = chr->mux_idx;
> + d->ext_opaque[idx] = chr->handler_opaque;
> + d->chr_can_read[idx] = chr->chr_can_read;
> + d->chr_read[idx] = chr->chr_read;
> + d->chr_event[idx] = chr->chr_event;
> +
> /* Fix up the real driver with mux routines */
> - if (d->mux_cnt == 0) {
> + if (d->mux_cnt == 1) {
> qemu_chr_add_handlers_full(d->drv, mux_chr_can_read,
> mux_chr_read,
> mux_chr_event,
> @@ -757,8 +766,7 @@ static void mux_chr_update_read_handler(CharDriverState *chr,
> if (d->focus != -1) {
> mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> }
> - d->focus = d->mux_cnt;
> - d->mux_cnt++;
> + d->focus = idx;
> mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
> }
>
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 0d0465a..4593576 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -92,6 +92,7 @@ struct CharDriverState {
> int explicit_be_open;
> int avail_connections;
> int is_mux;
> + int mux_idx;
> guint fd_in_tag;
> QemuOpts *opts;
> bool replay;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-11 12:20 ` Claudio Imbrenda
@ 2016-10-11 14:28 ` Marc-André Lureau
2016-10-11 16:18 ` Marc-André Lureau
0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-11 14:28 UTC (permalink / raw)
To: Claudio Imbrenda, qemu-devel; +Cc: pbonzini, kraxel
Hi
On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda <
imbrenda@linux.vnet.ibm.com> wrote:
> Hi,
>
> I noticed that this patch kills the Qemu monitor for me. If I start a
> text-mode guest with -nographic, then I can't switch to the monitor any
> longer with Ctrl+a c . The guest works otherwise, e.g. I can login from
> the console.
>
> Tested on s390, but I think it's a more general issue, since the patch
> is not arch-dependent.
>
>
> On 03/10/16 11:47, Marc-André Lureau wrote:
> > mux_chr_update_read_handler() is adding a new mux_cnt each time
> > mux_chr_update_read_handler() is called, it's not possible to actually
> > update the "child" chr callbacks that were set previously. This may lead
> > to crashes if the "child" chr is destroyed:
> >
>
My understanding was quite wrong, it was assuming that each mux user/fe was
a seperate chardev, that's not how it works.
The patch should probably be reverted until a better solution comes up. I
am looking at it, but no solution yet.
(obviously, it would be nice to have some minimal tests for mux, let see if
I get there)
> > valgrind x86_64-softmmu/qemu-system-x86_64 -chardev
> > stdio,mux=on,id=char0 -mon chardev=char0,mode=control,default
> >
> > when quitting:
> >
> > ==4306== Invalid read of size 8
> > ==4306== at 0x8061D3: json_lexer_destroy (json-lexer.c:385)
> > ==4306== by 0x7E39F8: json_message_parser_destroy
> (json-streamer.c:134)
> > ==4306== by 0x3447F6: monitor_qmp_event (monitor.c:3908)
> > ==4306== by 0x480153: mux_chr_send_event (qemu-char.c:630)
> > ==4306== by 0x480694: mux_chr_event (qemu-char.c:734)
> > ==4306== by 0x47F1E9: qemu_chr_be_event (qemu-char.c:205)
> > ==4306== by 0x481207: fd_chr_close (qemu-char.c:1114)
> > ==4306== by 0x481659: qemu_chr_close_stdio (qemu-char.c:1221)
> > ==4306== by 0x486F07: qemu_chr_free (qemu-char.c:4146)
> > ==4306== by 0x486F97: qemu_chr_delete (qemu-char.c:4154)
> > ==4306== by 0x487E66: qemu_chr_cleanup (qemu-char.c:4678)
> > ==4306== by 0x495A98: main (vl.c:4675)
> > ==4306== Address 0x28439e90 is 112 bytes inside a block of size 240
> free'd
> > ==4306== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
> > ==4306== by 0x1E4CBF2D: g_free (in /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306== by 0x344DE9: monitor_cleanup (monitor.c:4058)
> > ==4306== by 0x495A93: main (vl.c:4674)
> > ==4306== Block was alloc'd at
> > ==4306== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
> > ==4306== by 0x1E4CBE18: g_malloc (in
> /usr/lib64/libglib-2.0.so.0.4800.2)
> > ==4306== by 0x344BF8: monitor_init (monitor.c:4021)
> > ==4306== by 0x49063C: mon_init_func (vl.c:2417)
> > ==4306== by 0x7FC6DE: qemu_opts_foreach (qemu-option.c:1116)
> > ==4306== by 0x4954E0: main (vl.c:4473)
> >
> > Instead, keep the "child" chr associated with a particular idx so its
> > handlers can be updated and removed to avoid the crash.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > qemu-char.c | 22 +++++++++++++++-------
> > include/sysemu/char.h | 1 +
> > 2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index f7a07e6..4b330ea 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -165,6 +165,7 @@ CharDriverState *qemu_chr_alloc(ChardevCommon
> *backend, Error **errp)
> > CharDriverState *chr = g_malloc0(sizeof(CharDriverState));
> > qemu_mutex_init(&chr->chr_write_lock);
> >
> > + chr->mux_idx = -1;
> > if (backend->has_logfile) {
> > int flags = O_WRONLY | O_CREAT;
> > if (backend->has_logappend &&
> > @@ -738,17 +739,25 @@ static void
> mux_chr_update_read_handler(CharDriverState *chr,
> > GMainContext *context)
> > {
> > MuxDriver *d = chr->opaque;
> > + int idx;
> >
> > if (d->mux_cnt >= MAX_MUX) {
> > fprintf(stderr, "Cannot add I/O handlers, MUX array is full\n");
> > return;
> > }
> > - d->ext_opaque[d->mux_cnt] = chr->handler_opaque;
> > - d->chr_can_read[d->mux_cnt] = chr->chr_can_read;
> > - d->chr_read[d->mux_cnt] = chr->chr_read;
> > - d->chr_event[d->mux_cnt] = chr->chr_event;
> > +
> > + if (chr->mux_idx == -1) {
> > + chr->mux_idx = d->mux_cnt++;
> > + }
> > +
> > + idx = chr->mux_idx;
> > + d->ext_opaque[idx] = chr->handler_opaque;
> > + d->chr_can_read[idx] = chr->chr_can_read;
> > + d->chr_read[idx] = chr->chr_read;
> > + d->chr_event[idx] = chr->chr_event;
> > +
> > /* Fix up the real driver with mux routines */
> > - if (d->mux_cnt == 0) {
> > + if (d->mux_cnt == 1) {
> > qemu_chr_add_handlers_full(d->drv, mux_chr_can_read,
> > mux_chr_read,
> > mux_chr_event,
> > @@ -757,8 +766,7 @@ static void
> mux_chr_update_read_handler(CharDriverState *chr,
> > if (d->focus != -1) {
> > mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> > }
> > - d->focus = d->mux_cnt;
> > - d->mux_cnt++;
> > + d->focus = idx;
> > mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_IN);
> > }
> >
> > diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> > index 0d0465a..4593576 100644
> > --- a/include/sysemu/char.h
> > +++ b/include/sysemu/char.h
> > @@ -92,6 +92,7 @@ struct CharDriverState {
> > int explicit_be_open;
> > int avail_connections;
> > int is_mux;
> > + int mux_idx;
> > guint fd_in_tag;
> > QemuOpts *opts;
> > bool replay;
> >
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-11 14:28 ` Marc-André Lureau
@ 2016-10-11 16:18 ` Marc-André Lureau
2016-10-11 16:44 ` Daniel P. Berrange
0 siblings, 1 reply; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-11 16:18 UTC (permalink / raw)
To: Claudio Imbrenda, qemu-devel; +Cc: pbonzini, kraxel
Hi
On Tue, Oct 11, 2016 at 6:28 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda <
> imbrenda@linux.vnet.ibm.com> wrote:
>
> Hi,
>
> I noticed that this patch kills the Qemu monitor for me. If I start a
> text-mode guest with -nographic, then I can't switch to the monitor any
> longer with Ctrl+a c . The guest works otherwise, e.g. I can login from
> the console.
>
> Tested on s390, but I think it's a more general issue, since the patch
> is not arch-dependent.
>
>
> On 03/10/16 11:47, Marc-André Lureau wrote:
> > mux_chr_update_read_handler() is adding a new mux_cnt each time
> > mux_chr_update_read_handler() is called, it's not possible to actually
> > update the "child" chr callbacks that were set previously. This may lead
> > to crashes if the "child" chr is destroyed:
> >
>
>
> My understanding was quite wrong, it was assuming that each mux user/fe
> was a seperate chardev, that's not how it works.
>
> The patch should probably be reverted until a better solution comes up. I
> am looking at it, but no solution yet.
>
> (obviously, it would be nice to have some minimal tests for mux, let see
> if I get there)
> --
>
I am quite undecided how to fix this. qemu_chr_add_handlers users have no
associated tag: this works ok as long as there is a single user per
chardev. But it becomes problematic when there are multiple users, when the
backing chardev is a mux: the mux will just grow more fe handlers, And you
can't ever remove your fe callback which may lead to a crash. I am
surprised this problem didn't raise before.
I can imagine 2 solutions, either to associate each fe with it's opaque
pointer to lookup the corresponding mux registered callbacks (less
intrusive, yet not very clean), or to create a tag when calling
qemu_chr_add_handlers() which can be used later with a new function like
qemu_chr_remove_handlers(). This would be very intrusive, since all chr fe
will have to hold a tag in their struct and pass it accordingly.
Other thoughts?
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-11 16:18 ` Marc-André Lureau
@ 2016-10-11 16:44 ` Daniel P. Berrange
2016-10-12 9:03 ` Marc-André Lureau
0 siblings, 1 reply; 10+ messages in thread
From: Daniel P. Berrange @ 2016-10-11 16:44 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Claudio Imbrenda, qemu-devel, pbonzini, kraxel
On Tue, Oct 11, 2016 at 04:18:46PM +0000, Marc-André Lureau wrote:
> Hi
>
> On Tue, Oct 11, 2016 at 6:28 PM Marc-André Lureau <
> marcandre.lureau@gmail.com> wrote:
>
> > Hi
> >
> > On Tue, Oct 11, 2016 at 4:21 PM Claudio Imbrenda <
> > imbrenda@linux.vnet.ibm.com> wrote:
> >
> > Hi,
> >
> > I noticed that this patch kills the Qemu monitor for me. If I start a
> > text-mode guest with -nographic, then I can't switch to the monitor any
> > longer with Ctrl+a c . The guest works otherwise, e.g. I can login from
> > the console.
> >
> > Tested on s390, but I think it's a more general issue, since the patch
> > is not arch-dependent.
> >
> >
> > On 03/10/16 11:47, Marc-André Lureau wrote:
> > > mux_chr_update_read_handler() is adding a new mux_cnt each time
> > > mux_chr_update_read_handler() is called, it's not possible to actually
> > > update the "child" chr callbacks that were set previously. This may lead
> > > to crashes if the "child" chr is destroyed:
> > >
> >
> >
> > My understanding was quite wrong, it was assuming that each mux user/fe
> > was a seperate chardev, that's not how it works.
> >
> > The patch should probably be reverted until a better solution comes up. I
> > am looking at it, but no solution yet.
> >
> > (obviously, it would be nice to have some minimal tests for mux, let see
> > if I get there)
> > --
> >
>
> I am quite undecided how to fix this. qemu_chr_add_handlers users have no
> associated tag: this works ok as long as there is a single user per
> chardev. But it becomes problematic when there are multiple users, when the
> backing chardev is a mux: the mux will just grow more fe handlers, And you
> can't ever remove your fe callback which may lead to a crash. I am
> surprised this problem didn't raise before.
>
> I can imagine 2 solutions, either to associate each fe with it's opaque
> pointer to lookup the corresponding mux registered callbacks (less
> intrusive, yet not very clean), or to create a tag when calling
> qemu_chr_add_handlers() which can be used later with a new function like
> qemu_chr_remove_handlers(). This would be very intrusive, since all chr fe
> will have to hold a tag in their struct and pass it accordingly.
>
> Other thoughts?
Not sure if this is immediately helpful to your scneario or
not, but I'd like to see the qemu_chr_add_handlers method
removed long term, and everything converted to use the
qemu_chr_fe_add_watch function instead. This reverses the data
flow pattern - with chr_add_handlers the chardev code pushes
data from the backend into the frontends, but with fe_add_watch
the frontend pulls data from the backend.
To properly fix the non-blocking writes from the frontend to
the backend[1] will likely require use of qemu_chr_fe_add_watch,
and so having that function used for everything will make the
code clearer overall IMHO.
Regards,
Daniel
[1] eg the long term solution to replace this hack:
commit 90f998f5f4267a0c22e983f533d19b9de1849283
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Tue Sep 6 14:56:05 2016 +0100
char: convert qemu_chr_fe_write to qemu_chr_fe_write_all
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr
2016-10-11 16:44 ` Daniel P. Berrange
@ 2016-10-12 9:03 ` Marc-André Lureau
0 siblings, 0 replies; 10+ messages in thread
From: Marc-André Lureau @ 2016-10-12 9:03 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Claudio Imbrenda, qemu-devel, pbonzini, kraxel
Hi
On Tue, Oct 11, 2016 at 8:44 PM Daniel P. Berrange <berrange@redhat.com>
wrote:
>
> Not sure if this is immediately helpful to your scneario or
> not, but I'd like to see the qemu_chr_add_handlers method
> removed long term, and everything converted to use the
> qemu_chr_fe_add_watch function instead. This reverses the data
> flow pattern - with chr_add_handlers the chardev code pushes
> data from the backend into the frontends, but with fe_add_watch
> the frontend pulls data from the backend.
>
Very interesting, that would help, but I think that wouldn't be enough,
since we would still have the "event" handlers to properly register/track.
Furthermore, that upside-down change is not easy. I think I will simply go
with a new fe handler tag.
>
> To properly fix the non-blocking writes from the frontend to
> the backend[1] will likely require use of qemu_chr_fe_add_watch,
> and so having that function used for everything will make the
> code clearer overall IMHO.
>
> Regards,
> Daniel
>
> [1] eg the long term solution to replace this hack:
>
> commit 90f998f5f4267a0c22e983f533d19b9de1849283
> Author: Daniel P. Berrange <berrange@redhat.com>
> Date: Tue Sep 6 14:56:05 2016 +0100
>
> char: convert qemu_chr_fe_write to qemu_chr_fe_write_all
>
>
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/
> :|
> |: http://libvirt.org -o- http://virt-manager.org
> :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/
> :|
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-12 9:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-03 9:47 [Qemu-devel] [PATCH 0/2] Fix use after free with mux Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 1/2] char: update read handler in all cases Marc-André Lureau
2016-10-03 9:47 ` [Qemu-devel] [PATCH 2/2] char: use a fixed idx for child muxed chr Marc-André Lureau
2016-10-11 12:20 ` Claudio Imbrenda
2016-10-11 14:28 ` Marc-André Lureau
2016-10-11 16:18 ` Marc-André Lureau
2016-10-11 16:44 ` Daniel P. Berrange
2016-10-12 9:03 ` Marc-André Lureau
2016-10-03 9:56 ` [Qemu-devel] [PATCH 0/2] Fix use after free with mux Paolo Bonzini
2016-10-03 10:08 ` Marc-André Lureau
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.