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

[Sai Pavan Boddu]

Hi Daniel,

> -----Original Message-----
> From: Daniel P. Berrangé <address@hidden>
> Sent: Friday, April 17, 2020 6:44 PM
> To: Marc-André Lureau <address@hidden>
> Cc: Sai Pavan Boddu <address@hidden>; Paolo Bonzini
> <address@hidden>; Edgar Iglesias <address@hidden>; QEMU <qemu-
> address@hidden>
> 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
> > <address@hidden> 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 <address@hidden>
> > > ---
> > >  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
> :|