Re: [Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
On Fri, Jun 01, 2018 at 06:27:38PM +0200, Marc-André Lureau wrote:
> A socket chardev may not have associated address (when adding client
> fd manually for example). But on disconnect, updating socket filename
> expects an address and may lead to this crash:
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> 388 switch (addr->type) {
> (gdb) bt
> #0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> #1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00)
> at /home/elmarco/src/qq/chardev/char-socket.c:419
> #2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:438
> #3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690,
> cond=G_IO_HUP, opaque=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:482
> #4 0x55da596e in qio_channel_fd_source_dispatch
> (source=0x56bb68b0, callback=0x55d8cb58 ,
> user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
>
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..f1b7907798 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev
> *s)
> Chardev *chr = CHARDEV(s);
>
> g_free(chr->filename);
> -chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> - s->is_listen, s->is_telnet);
> +chr->filename = NULL;
> +if (s->addr) {
> +chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> + s->is_listen, s->is_telnet);
> +}
> }
This will mean 'chr->filename' as NULL, which means other code using this
field may get NULL - especially the monitor looks like it will printf()
passing a NULL, which is a crash on some platforms. So I think you need
an else clause that will set it to some dummy value.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
On Fri, Jun 08, 2018 at 11:52:11AM -0300, Philippe Mathieu-Daudé wrote:
> On 06/01/2018 01:27 PM, Marc-André Lureau wrote:
> > A socket chardev may not have associated address (when adding client
> > fd manually for example). But on disconnect, updating socket filename
> > expects an address and may lead to this crash:
> >
> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> > "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> > /home/elmarco/src/qq/chardev/char-socket.c:388
> > 388 switch (addr->type) {
> > (gdb) bt
> > #0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> > "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> > /home/elmarco/src/qq/chardev/char-socket.c:388
> > #1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00)
> > at /home/elmarco/src/qq/chardev/char-socket.c:419
> > #2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
> > /home/elmarco/src/qq/chardev/char-socket.c:438
> > #3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690,
> > cond=G_IO_HUP, opaque=0x56b1ed00) at
> > /home/elmarco/src/qq/chardev/char-socket.c:482
> > #4 0x55da596e in qio_channel_fd_source_dispatch
> > (source=0x56bb68b0, callback=0x55d8cb58 ,
> > user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > chardev/char-socket.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 159e69c3b1..f1b7907798 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev
> > *s)
> > Chardev *chr = CHARDEV(s);
> >
> > g_free(chr->filename);
> > -chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> > - s->is_listen, s->is_telnet);
> > +chr->filename = NULL;
> > +if (s->addr) {
>
> Isn't it more robust to add this check in SocketAddress_to_str()?
IMHO that just shifts the problem elsewhere - currently SocketAddress_to_str
is assumed to return non-NULL, or to abort(). Shifting the check means it
can now return NULL, so there's every chance the caller will now reference
the NULL pointer that's returned.
>
> static char *SocketAddress_to_str(const char *prefix, SocketAddress
> *addr,
> bool is_listen, bool is_telnet)
> {
> if (!addr) {
> return NULL;
> }
> switch (addr->type) {
> ...
>
> > +chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> > + s->is_listen, s->is_telnet);
> > +}
> > }
> >
> > /* NB may be called even if tcp_chr_connect has not been
> >
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
On 06/01/2018 01:27 PM, Marc-André Lureau wrote:
> A socket chardev may not have associated address (when adding client
> fd manually for example). But on disconnect, updating socket filename
> expects an address and may lead to this crash:
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> 388 switch (addr->type) {
> (gdb) bt
> #0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> #1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00)
> at /home/elmarco/src/qq/chardev/char-socket.c:419
> #2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:438
> #3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690,
> cond=G_IO_HUP, opaque=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:482
> #4 0x55da596e in qio_channel_fd_source_dispatch
> (source=0x56bb68b0, callback=0x55d8cb58 ,
> user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
>
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..f1b7907798 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev
> *s)
> Chardev *chr = CHARDEV(s);
>
> g_free(chr->filename);
> -chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> - s->is_listen, s->is_telnet);
> +chr->filename = NULL;
> +if (s->addr) {
Isn't it more robust to add this check in SocketAddress_to_str()?
static char *SocketAddress_to_str(const char *prefix, SocketAddress
*addr,
bool is_listen, bool is_telnet)
{
if (!addr) {
return NULL;
}
switch (addr->type) {
...
> +chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> + s->is_listen, s->is_telnet);
> +}
> }
>
> /* NB may be called even if tcp_chr_connect has not been
>
[Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
A socket chardev may not have associated address (when adding client
fd manually for example). But on disconnect, updating socket filename
expects an address and may lead to this crash:
Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
"disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
/home/elmarco/src/qq/chardev/char-socket.c:388
388 switch (addr->type) {
(gdb) bt
#0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
"disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
/home/elmarco/src/qq/chardev/char-socket.c:388
#1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00) at
/home/elmarco/src/qq/chardev/char-socket.c:419
#2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
/home/elmarco/src/qq/chardev/char-socket.c:438
#3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690, cond=G_IO_HUP,
opaque=0x56b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
#4 0x55da596e in qio_channel_fd_source_dispatch
(source=0x56bb68b0, callback=0x55d8cb58 ,
user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
Signed-off-by: Marc-André Lureau
---
chardev/char-socket.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 159e69c3b1..f1b7907798 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev *s)
Chardev *chr = CHARDEV(s);
g_free(chr->filename);
-chr->filename = SocketAddress_to_str("disconnected:", s->addr,
- s->is_listen, s->is_telnet);
+chr->filename = NULL;
+if (s->addr) {
+chr->filename = SocketAddress_to_str("disconnected:", s->addr,
+ s->is_listen, s->is_telnet);
+}
}
/* NB may be called even if tcp_chr_connect has not been
--
2.17.1.906.g10fd178552
