Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in control_out() message

[Michael Tokarev]

On 12.03.2012 12:59, Amit Shah wrote:
> On (Sun) 11 Mar 2012 [17:52:59], Michael Tokarev wrote:
>> In case of more than one control message, the code will use
>> size of the largest message so far for all subsequent messages,
>> instead of using size of current one.  Fix it.
> 
> Makes sense.  How did you detect this?  Any reproducible test-case?

There's no test-case, and no detection, just reading the code.
Actually, I think, there's no bug here, but a very, well,
difficult to read code.

> One comment below.

and the answer is below, too.

>> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> index e22940e..abe48ec 100644
>> --- a/hw/virtio-serial-bus.c
>> +++ b/hw/virtio-serial-bus.c
>> @@ -451,28 +451,28 @@ static void control_out(VirtIODevice *vdev, VirtQueue 
>> *vq)
>>  
>>      vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
>>  
>>      len = 0;
>>      buf = NULL;
>>      while (virtqueue_pop(vq, &elem)) {
>> -        size_t cur_len, copied;
>> +        size_t cur_len;
>>  
>>          cur_len = iov_size(elem.out_sg, elem.out_num);
>>          /*
>>           * Allocate a new buf only if we didn't have one previously or
>>           * if the size of the buf differs
>>           */
>>          if (cur_len > len) {
>>              g_free(buf);
>>  
>>              buf = g_malloc(cur_len);
>>              len = cur_len;
>>          }
>> -        copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
>> +        iov_to_buf(elem.out_sg, elem.out_num, buf, 0, cur_len);
> 
> Why drop 'copied'?  I don't think we have had a situation where copied
> can be less than cur_len, and in any case we don't do anything special
> as a recovery mechanism, but a warning message or an abort in case
> copied != cur_len should work, I think.

In this case, copied was _always_ == cur_len.  That's why there's
actually no bug.  See:

   cur_len = iov_size(elem.out_sg, elem.out_num);
   len = max(cur_len, buflen) <= "roughly"
   copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);

iov_to_buf() will stop copying when it reaches end of buf
(which is "len" bytes long) or end of iov, which is cur_len
bytes long.  Obviously in all cases it will be cur_len.
But it is obvious only when you write it one near another
and _think_.  And the reason for this confusion is the
introduction of this `copied' variable, which shouldn't
be there in the first place.

It is like doing, for a memcpy-like function:

 void *memdup(const void *src, size_t size) {
   char *dest = malloc(size+1);
   size_t copied = copybytes(dest, src, size+1);
   if (copied != size+1) {
      /* What?? */
   }
   return dest;
}

The only sane thing here, I think, is to drop 'copied',
to stop any possible confusion :)

>> -        handle_control_message(vser, buf, copied);
>> +        handle_control_message(vser, buf, cur_len);
>>          virtqueue_push(vq, &elem, 0);
>>      }
>>      g_free(buf);
>>      virtio_notify(vdev, vq);
>>  }

Please belive me, I realized that the original code is
actually right only after re-reading your reply.  And
please note that even you, the author, don't understand
what it is doing :)  So I think the patch is correct
still ;)

Thanks!

/mjt