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

[Paolo Bonzini <pbonzini@redhat.com>]

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

    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, but I
may be wrong).

Stefan, what do you think?

Paolo

>  
> +    if (TxLoopBack == (s->TxConfig & TxLoopBack)) {
>          DPRINTF("+++ transmit loopback mode\n");
>          rtl8139_do_receive(qemu_get_queue(s->nic), buf, size, do_interrupt);
> -
> -        if (iov) {
> -            g_free(buf2);
> -        }
> -    }
> -    else
> -    {
> -        if (iov) {
> -            qemu_sendv_packet(qemu_get_queue(s->nic), iov, 3);
> -        } else {
> -            qemu_send_packet(qemu_get_queue(s->nic), buf, size);
> -        }
> +    } else {
> +        qemu_send_packet(qemu_get_queue(s->nic), buf, size);
>      }
> +
> +    g_free(buf2);
>  }
>  
>  static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
>