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

[Gonglei <arei.gonglei@huawei.com>]

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, arei.gonglei@huawei.com wrote:
>>> From: Gonglei <arei.gonglei@huawei.com>
>>>
>>> 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 <arei.gonglei@huawei.com>
>>> ---
>>>  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