[v1] chardev/char-socket: Properly make qio connections non blocking

[PATCH] chardev/char-socket: Properly make qio connections non blocking

Posted by Sai Pavan Boddu 4 years ago

In tcp_chr_sync_read function, there is a possibility of socket
disconnection during read, then tcp_chr_hup function would clean up
the qio channel pointers(i.e ioc, sioc).

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
---
 chardev/char-socket.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 185fe38..30f2b2b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
 
     qio_channel_set_blocking(s->ioc, true, NULL);
     size = tcp_chr_recv(chr, (void *) buf, len);
-    qio_channel_set_blocking(s->ioc, false, NULL);
     if (size == 0) {
         /* connection closed */
         tcp_chr_disconnect(chr);
+        return 0;
     }
+    /* Connection is good */
+    qio_channel_set_blocking(s->ioc, false, NULL);
 
     return size;
 }
-- 
2.7.4

Re: [PATCH] chardev/char-socket: Properly make qio connections non blocking

Posted by Marc-André Lureau 4 years ago

Hi

On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> In tcp_chr_sync_read function, there is a possibility of socket
> disconnection during read, then tcp_chr_hup function would clean up
> the qio channel pointers(i.e ioc, sioc).
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  chardev/char-socket.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 185fe38..30f2b2b 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
>
>      qio_channel_set_blocking(s->ioc, true, NULL);
>      size = tcp_chr_recv(chr, (void *) buf, len);
> -    qio_channel_set_blocking(s->ioc, false, NULL);

But here it calls tcp_chr_recv(). And I can't find cleanup there.
Nevertheless, I think this patch should be harmless.

I'd ask Daniel to have a second look.


>      if (size == 0) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
> +        return 0;
>      }
> +    /* Connection is good */
> +    qio_channel_set_blocking(s->ioc, false, NULL);
>
>      return size;
>  }
> --
> 2.7.4
>
>


-- 
Marc-André Lureau

Re: [PATCH] chardev/char-socket: Properly make qio connections non blocking

Posted by Daniel P. Berrangé 4 years ago

On Fri, Apr 17, 2020 at 03:01:09PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu
> <sai.pavan.boddu@xilinx.com> wrote:
> >
> > In tcp_chr_sync_read function, there is a possibility of socket
> > disconnection during read, then tcp_chr_hup function would clean up
> > the qio channel pointers(i.e ioc, sioc).
> >
> > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > ---
> >  chardev/char-socket.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 185fe38..30f2b2b 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr, const uint8_t *buf, int len)
> >
> >      qio_channel_set_blocking(s->ioc, true, NULL);
> >      size = tcp_chr_recv(chr, (void *) buf, len);
> > -    qio_channel_set_blocking(s->ioc, false, NULL);
> 
> But here it calls tcp_chr_recv(). And I can't find cleanup there.
> Nevertheless, I think this patch should be harmless.
> 
> I'd ask Daniel to have a second look.

I don't see any bug that needs fixing here, and I prefer the current
code as it gives confidence that nothing tcp_chr_disconnect does
will accidentally block.


> >      if (size == 0) {
> >          /* connection closed */
> >          tcp_chr_disconnect(chr);
> > +        return 0;
> >      }
> > +    /* Connection is good */
> > +    qio_channel_set_blocking(s->ioc, false, NULL);
> >
> >      return size;
> >  }
> > --
> > 2.7.4
> >
> >
> 
> 
> -- 
> Marc-André Lureau
> 

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: [PATCH] chardev/char-socket: Properly make qio connections non blocking

Posted by Sai Pavan Boddu 4 years ago

Hi Daniel,

> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: Friday, April 17, 2020 6:44 PM
> To: Marc-André Lureau <marcandre.lureau@gmail.com>
> Cc: Sai Pavan Boddu <saipava@xilinx.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Edgar Iglesias <edgari@xilinx.com>; QEMU <qemu-
> devel@nongnu.org>
> Subject: Re: [PATCH] chardev/char-socket: Properly make qio connections
> non blocking
> 
> On Fri, Apr 17, 2020 at 03:01:09PM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Apr 17, 2020 at 2:38 PM Sai Pavan Boddu
> > <sai.pavan.boddu@xilinx.com> wrote:
> > >
> > > In tcp_chr_sync_read function, there is a possibility of socket
> > > disconnection during read, then tcp_chr_hup function would clean up
> > > the qio channel pointers(i.e ioc, sioc).
> > >
> > > Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> > > ---
> > >  chardev/char-socket.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c index
> > > 185fe38..30f2b2b 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -549,11 +549,13 @@ static int tcp_chr_sync_read(Chardev *chr,
> > > const uint8_t *buf, int len)
> > >
> > >      qio_channel_set_blocking(s->ioc, true, NULL);
> > >      size = tcp_chr_recv(chr, (void *) buf, len);
> > > -    qio_channel_set_blocking(s->ioc, false, NULL);
> >
> > But here it calls tcp_chr_recv(). And I can't find cleanup there.
> > Nevertheless, I think this patch should be harmless.
> >
> > I'd ask Daniel to have a second look.
> 
> I don't see any bug that needs fixing here, and I prefer the current code as it
> gives confidence that nothing tcp_chr_disconnect does will accidentally
> block.
[Sai Pavan Boddu] The issue is triggered when 'tcp_chr_hup' callback, is dispatched when socket hung up during a blocking read. Which also calls tcp_chr_disconnect and cleans up ioc, sioc pointers. Later below line segfaults due to null pointers

	" qio_channel_set_blocking(s->ioc, false, NULL); "

Regards,
Sai Pavan
> 
> 
> > >      if (size == 0) {
> > >          /* connection closed */
> > >          tcp_chr_disconnect(chr);
> > > +        return 0;
> > >      }
> > > +    /* Connection is good */
> > > +    qio_channel_set_blocking(s->ioc, false, NULL);
> > >
> > >      return size;
> > >  }
> > > --
> > > 2.7.4
> > >
> > >
> >
> >
> > --
> > Marc-André Lureau
> >
> 
> 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
> :|