From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34892) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aSlcI-0005di-Sc for qemu-devel@nongnu.org; Mon, 08 Feb 2016 08:12:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aSlcF-0004fb-LZ for qemu-devel@nongnu.org; Mon, 08 Feb 2016 08:12:34 -0500 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]:34834) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aSlcF-0004eP-Aw for qemu-devel@nongnu.org; Mon, 08 Feb 2016 08:12:31 -0500 Received: by mail-wm0-x231.google.com with SMTP id c200so14859045wme.0 for ; Mon, 08 Feb 2016 05:12:30 -0800 (PST) Message-ID: <56B8942E.7050106@6wind.com> Date: Mon, 08 Feb 2016 14:12:14 +0100 From: Didier Pallard MIME-Version: 1.0 References: <1449136399-4158-1-git-send-email-didier.pallard@6wind.com> <1449136399-4158-2-git-send-email-didier.pallard@6wind.com> <20160204154232-mutt-send-email-mst@redhat.com> In-Reply-To: <20160204154232-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] char: fix vhost-user socket full List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: thibaut.collet@6wind.com, jmg@6wind.com, qemu-devel@nongnu.org On 02/04/2016 03:10 PM, 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 >> Reviewed-by: Thibaut Collet >> --- >> 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? Well, in qemu_chr_fe_write, all i have is a CharDriverState. Information on number of fds to pass are located in TCPCharDriver structure. should i assert as soon as a set_msgfds method is set: assert(s->set_msgfds == NULL); but it may trigger on unix sockets were message fds are never used. or try to detect the implementation and really check the number of fds passed: assert((s->chr_write != tcp_chr_write) && ((TCPCharDriver *)s->opaque)->write_msgfds_num == 0)) but i didn't find better than checking a method to detect type of underlying driver and I don't like this kind of code. Another solution should be to add a new method to get pending fds number, assert would become: assert((s->get_write_msgfds_num == NULL) || (s->get_write_msgfds_num(s) == 0)) s->get_write_msgfds_num being set only for tcp_ socket, with a simple function: tcp_chr_get_write_msgfds_num(CharDriverState *chr) { TCPCharDriver *s = chr->opaque; return s->write_msgfds_num; } ... s->get_write_msgfds_num = tcp_chr_get_write_msgfds_num; ... ? > >> + */ >> + if (r < 0 && errno == EAGAIN) { >> + return r; >> + } >> + >> /* free the written msgfds, no matter what */ >> if (s->write_msgfds_num) { >> g_free(s->write_msgfds); >> -- >> 2.1.4 >> >