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

[Didier Pallard <didier.pallard@6wind.com>]

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 <didier.pallard@6wind.com>
>> Reviewed-by: Thibaut Collet <thibaut.collet@6wind.com>
>> ---
>>   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
>>
>