Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full

[Daniel P. Berrange]

On Thu, Feb 04, 2016 at 04:10:38PM +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 03, 2015 at 10:53:17AM +0100, Didier Pallard wrote:
> > unix_send_msgfds is used by vhost-user control socket. qemu_chr_fe_write_all
> > is used to send a message and retries as long as EAGAIN errno is set,
> > but write_msgfds buffer is freed after first EAGAIN failure, causing
> > message to be sent without proper fds attachment.
> > 
> > In case unix_send_msgfds is called through qemu_chr_fe_write, it will be
> > user responsability to resend message as is or to free write_msgfds
> > using set_msgfds(0)
> > 
> > Signed-off-by: Didier Pallard <address@hidden>
> > Reviewed-by: Thibaut Collet <address@hidden>
> > ---
> >  qemu-char.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 5448b0f..26d5f2e 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -2614,6 +2614,16 @@ static int unix_send_msgfds(CharDriverState *chr, 
> > const uint8_t *buf, int len)
> >          r = sendmsg(s->fd, &msgh, 0);
> >      } while (r < 0 && errno == EINTR);
> >  
> > +    /* Ancillary data are not sent if no byte is written
> > +     * so don't free msgfds buffer if return value is EAGAIN
> > +     * If called from qemu_chr_fe_write_all retry will come soon
> > +     * If called from qemu_chr_fe_write, it is the user responsibility
> > +     * to resend message or free fds using set_msgfds(0)
> 
> Problem is, if ever anyone tries to send messages
> with qemu_chr_fe_write and does not retry, there will
> be a memory leak.
> 
> Rather than this, how about adding an assert in qemu_chr_fe_write
> to make sure its not used with msgfds?

NB, this TCP chardev code has completely changed since this patch
was submitted. It now uses QIOChannel instead of raw sockets APIs.
The same problem still exists though - when we get EAGAIN from
the sendmsg, we're releasing the file descriptors even though
they've not been sent.

Adding an assert in qemu_chr_fe_write() won't solve it - the
same problem still exists in qemu_chr_fe_write_all() - although
that loops to re-try on EAGAIN, the FDs are free'd before it
does the retry.

We need to update tcp_chr_write() to not free the FDs when
io_channel_send_full() returns with errno==EAGAIN, in the
same way this patch was doing.


> > +     */
> > +    if (r < 0 && errno == EAGAIN) {
> > +        return r;
> > +    }
> > +
> >      /* free the written msgfds, no matter what */
> >      if (s->write_msgfds_num) {
> >          g_free(s->write_msgfds);

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|