diff mbox series

[RFC,v2,01/12] chardev: avoid crash if no associated address

Message ID 20180601162749.27406-2-marcandre.lureau@redhat.com
State New
Headers show
Series vhost-user for input & GPU | expand

Commit Message

Marc-André Lureau June 1, 2018, 4:27 p.m. UTC 
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.
  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "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  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
  #1  0x0000555555d8c8aa in update_disconnected_filename (s=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:419
  #2  0x0000555555d8c959 in tcp_chr_disconnect (chr=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:438
  #3  0x0000555555d8cba1 in tcp_chr_hup (channel=0x555556b75690, cond=G_IO_HUP, opaque=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
  #4  0x0000555555da596e in qio_channel_fd_source_dispatch (source=0x555556bb68b0, callback=0x555555d8cb58 <tcp_chr_hup>, user_data=0x555556b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé June 8, 2018, 2:52 p.m. UTC | #1 
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.
>   0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "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  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
>   #1  0x0000555555d8c8aa in update_disconnected_filename (s=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:419
>   #2  0x0000555555d8c959 in tcp_chr_disconnect (chr=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:438
>   #3  0x0000555555d8cba1 in tcp_chr_hup (channel=0x555556b75690, cond=G_IO_HUP, opaque=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
>   #4  0x0000555555da596e in qio_channel_fd_source_dispatch (source=0x555556bb68b0, callback=0x555555d8cb58 <tcp_chr_hup>, user_data=0x555556b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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
>
Daniel P. Berrangé June 11, 2018, 8:59 a.m. UTC | #2 
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.
> >   0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "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  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
> >   #1  0x0000555555d8c8aa in update_disconnected_filename (s=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:419
> >   #2  0x0000555555d8c959 in tcp_chr_disconnect (chr=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:438
> >   #3  0x0000555555d8cba1 in tcp_chr_hup (channel=0x555556b75690, cond=G_IO_HUP, opaque=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
> >   #4  0x0000555555da596e in qio_channel_fd_source_dispatch (source=0x555556bb68b0, callback=0x555555d8cb58 <tcp_chr_hup>, user_data=0x555556b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  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
Daniel P. Berrangé June 11, 2018, 9:04 a.m. UTC | #3 
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.
>   0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "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  0x0000555555d8c70c in SocketAddress_to_str (prefix=0x555556043062 "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at /home/elmarco/src/qq/chardev/char-socket.c:388
>   #1  0x0000555555d8c8aa in update_disconnected_filename (s=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:419
>   #2  0x0000555555d8c959 in tcp_chr_disconnect (chr=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:438
>   #3  0x0000555555d8cba1 in tcp_chr_hup (channel=0x555556b75690, cond=G_IO_HUP, opaque=0x555556b1ed00) at /home/elmarco/src/qq/chardev/char-socket.c:482
>   #4  0x0000555555da596e in qio_channel_fd_source_dispatch (source=0x555556bb68b0, callback=0x555555d8cb58 <tcp_chr_hup>, user_data=0x555556b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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
diff mbox series

Patch

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