Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope

[Jason Wang]

On 11/20/2014 03:12 PM, Gonglei wrote:
> On 2014/11/20 14:55, Jason Wang wrote:
>
>> On 11/20/2014 02:29 PM, Paolo Bonzini wrote:
>>> On 20/11/2014 06:57, address@hidden wrote:
>>>> From: Gonglei <address@hidden>
>>>>
>>>> Coverity spot:
>>>>  Assigning: iov = struct iovec [3]({{buf, 12UL},
>>>>                        {(void *)dot1q_buf, 4UL},
>>>>                        {buf + 12, size - 12}})
>>>>  (address of temporary variable of type struct iovec [3]).
>>>>  out_of_scope: Temporary variable of type struct iovec [3] goes out of 
>>>> scope.
>>>>
>>>> Pointer to local outside scope (RETURN_LOCAL)
>>>> use_invalid:
>>>>  Using iov, which points to an out-of-scope temporary variable of type 
>>>> struct iovec [3].
>>>>
>>>> Signed-off-by: Gonglei <address@hidden>
>>>> ---
>>>>  hw/net/rtl8139.c | 36 ++++++++++++------------------------
>>>>  1 file changed, 12 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
>>>> index 8b8a1b1..426171b 100644
>>>> --- a/hw/net/rtl8139.c
>>>> +++ b/hw/net/rtl8139.c
>>>> @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, 
>>>> uint8_t *buf, int size,
>>>>      int do_interrupt, const uint8_t *dot1q_buf)
>>>>  {
>>>>      struct iovec *iov = NULL;
>>>> +    size_t buf2_size;
>>>> +    uint8_t *buf2 = NULL;
>>>>  
>>>>      if (!size)
>>>>      {
>>>> @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State 
>>>> *s, uint8_t *buf, int size,
>>>>              { .iov_base = buf + ETHER_ADDR_LEN * 2,
>>>>                  .iov_len = size - ETHER_ADDR_LEN * 2 },
>>>>          };
>>>> -    }
>>>>  
>>>> -    if (TxLoopBack == (s->TxConfig & TxLoopBack))
>>>> -    {
>>>> -        size_t buf2_size;
>>>> -        uint8_t *buf2;
>>>> -
>>>> -        if (iov) {
>>>> -            buf2_size = iov_size(iov, 3);
>>>> -            buf2 = g_malloc(buf2_size);
>>>> -            iov_to_buf(iov, 3, 0, buf2, buf2_size);
>>>> -            buf = buf2;
>>>> -        }
>>>> +        buf2_size = iov_size(iov, 3);
>>>> +        buf2 = g_malloc(buf2_size);
>>>> +        iov_to_buf(iov, 3, 0, buf2, buf2_size);
>>>> +        buf = buf2;
>>>> +    }
>>> This makes rtl8139 even slower than it is for the vlantag case, using a
>>> bounce buffer for every packet.  Perhaps another solution could be to do
> Indeed. Your approach is better. :)
>
>>>     struct iovec *iov = NULL;
>>>     struct iovec vlan_iov[3];
>>>
>>>     ...
>>>     if (dot1q_buf && size >= ETHER_ADDR_LEN * 2) {
>>>        ...
>>>        memcpy(vlan_iov, &(struct iovec[3]) {
>>>                ...
>>>            }, sizeof(vlan_iov));
>>>        iov = vlan_iov;
>>>     }
>>>
>>> (I think "vlan_iov = (struct iovec[3]) { ... };" does not work,
> Yes. the same reason with the original issue.
>
>>>  but I
>>> may be wrong).
>>>
>>> Stefan, what do you think?
>>>
>>> Paolo
>>>
>>>
>> Maybe just initialize iov unconditionally at the beginning and check
>> dot1q_buf instead of iov for the rest of the functions. (Need deal with
>> size < ETHER_ADDR_LEN * 2)
> More complicated, because we can't initialize iov when
>  "size < ETHER_ADDR_LEN * 2".
>
> Best regards,
> -Gonglei
>

Probably not: you can just do something like:

    if (dot1q_buf && size < ETHER_ADDR_LEN * 2) {
        dot1q_buf = NULL;
    }

and check dot1q_buf afterwards. Or just drop the packet since its size
was less than mininum frame length that Ethernet allows.