[Qemu-devel] [PATCH 0/4] net: fix high impact outstanding defects reported by Coverity

[Qemu-devel] [PATCH 0/4] net: fix high impact outstanding defects reported by Coverity

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Please see details in every patch.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

Gonglei (4):
  net/slirp: fix memory leak
  net/socket: fix Uninitialized scalar variable
  pcnet: fix Negative array index read
  rtl8139: fix Pointer to local outside scope

 hw/net/pcnet.c   |  2 +-
 hw/net/rtl8139.c | 36 ++++++++++++------------------------
 net/slirp.c      |  3 +--
 net/socket.c     | 11 ++++++-----
 4 files changed, 20 insertions(+), 32 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/4] net/slirp: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

commit b412eb61 introduce 'cmd:' target for guestfwd,
and fwd don't be used in this scenario, and will leak
memory in true branch with 'cmd:'. Let's allocate memory
for fwd variable just in else statement.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 net/slirp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index dc89e6b..377d7ef 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -643,17 +643,16 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
         goto fail_syntax;
     }
 
-    fwd = g_malloc(sizeof(struct GuestFwd));
     snprintf(buf, sizeof(buf), "guestfwd.tcp.%d", port);
 
     if ((strlen(p) > 4) && !strncmp(p, "cmd:", 4)) {
         if (slirp_add_exec(s->slirp, 0, &p[4], &server, port) < 0) {
             error_report("conflicting/invalid host:port in guest forwarding "
                          "rule '%s'", config_str);
-            g_free(fwd);
             return -1;
         }
     } else {
+        fwd = g_malloc(sizeof(struct GuestFwd));
         fwd->hd = qemu_chr_new(buf, p, NULL);
         if (!fwd->hd) {
             error_report("could not open guest forwarding device '%s'", buf);
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/4] net/socket: fix Uninitialized scalar variable

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

If is_connected parameter is false, the saddr
variable will no initialize. Coverity report:
uninit_use: Using uninitialized value saddr.sin_port.

We don't need add saddr information to nc->info_str
when is_connected is false.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 net/socket.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index ca4b8ba..68a93cd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -389,11 +389,6 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
 
     nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name);
 
-    snprintf(nc->info_str, sizeof(nc->info_str),
-            "socket: fd=%d (%s mcast=%s:%d)",
-            fd, is_connected ? "cloned" : "",
-            inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
-
     s = DO_UPCAST(NetSocketState, nc, nc);
 
     s->fd = fd;
@@ -404,6 +399,12 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
     /* mcast: save bound address as dst */
     if (is_connected) {
         s->dgram_dst = saddr;
+        snprintf(nc->info_str, sizeof(nc->info_str),
+                 "socket: fd=%d (cloned mcast=%s:%d)",
+                 fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
+    } else {
+        snprintf(nc->info_str, sizeof(nc->info_str),
+                 "socket: fd=%d", fd);
     }
 
     return s;
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

s->xmit_pos maybe assigned to a negative value (-1),
but in this branch variable s->xmit_pos as an index to
array s->buffer. Let's add a check for s->xmit_pos.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/net/pcnet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
index d344c15..2bb6417 100644
--- a/hw/net/pcnet.c
+++ b/hw/net/pcnet.c
@@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s)
             s->xmit_pos = -1;
             goto txdone;
         }
-        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
+        if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) {
             int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
             s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
                              s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
-- 
1.7.12.4

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

[arei.gonglei]

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;
+    }
 
+    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)
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/4] net/slirp: fix memory leak

[Jason Wang]

On 11/20/2014 01:57 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> commit b412eb61 introduce 'cmd:' target for guestfwd,
> and fwd don't be used in this scenario, and will leak
> memory in true branch with 'cmd:'. Let's allocate memory
> for fwd variable just in else statement.
>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/slirp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index dc89e6b..377d7ef 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -643,17 +643,16 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
>          goto fail_syntax;
>      }
>  
> -    fwd = g_malloc(sizeof(struct GuestFwd));
>      snprintf(buf, sizeof(buf), "guestfwd.tcp.%d", port);
>  
>      if ((strlen(p) > 4) && !strncmp(p, "cmd:", 4)) {
>          if (slirp_add_exec(s->slirp, 0, &p[4], &server, port) < 0) {
>              error_report("conflicting/invalid host:port in guest forwarding "
>                           "rule '%s'", config_str);
> -            g_free(fwd);
>              return -1;
>          }
>      } else {
> +        fwd = g_malloc(sizeof(struct GuestFwd));
>          fwd->hd = qemu_chr_new(buf, p, NULL);
>          if (!fwd->hd) {
>              error_report("could not open guest forwarding device '%s'", buf);

Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] net/socket: fix Uninitialized scalar variable

[Jason Wang]

On 11/20/2014 01:57 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> If is_connected parameter is false, the saddr
> variable will no initialize. Coverity report:
> uninit_use: Using uninitialized value saddr.sin_port.
>
> We don't need add saddr information to nc->info_str
> when is_connected is false.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/socket.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index ca4b8ba..68a93cd 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -389,11 +389,6 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>  
>      nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name);
>  
> -    snprintf(nc->info_str, sizeof(nc->info_str),
> -            "socket: fd=%d (%s mcast=%s:%d)",
> -            fd, is_connected ? "cloned" : "",
> -            inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> -
>      s = DO_UPCAST(NetSocketState, nc, nc);
>  
>      s->fd = fd;
> @@ -404,6 +399,12 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
>      /* mcast: save bound address as dst */
>      if (is_connected) {
>          s->dgram_dst = saddr;
> +        snprintf(nc->info_str, sizeof(nc->info_str),
> +                 "socket: fd=%d (cloned mcast=%s:%d)",
> +                 fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> +    } else {
> +        snprintf(nc->info_str, sizeof(nc->info_str),
> +                 "socket: fd=%d", fd);
>      }
>  
>      return s;

Reviewed-by: Jason Wang <jasowang@redhat.com>

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

[Paolo Bonzini]

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)
> 

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Jason Wang]

On 11/20/2014 01:57 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
>
> s->xmit_pos maybe assigned to a negative value (-1),
> but in this branch variable s->xmit_pos as an index to
> array s->buffer. Let's add a check for s->xmit_pos.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/net/pcnet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index d344c15..2bb6417 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s)
>              s->xmit_pos = -1;
>              goto txdone;
>          }
> -        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
> +        if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) {
>              int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>              s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>                               s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));

Reviewed-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Paolo Bonzini]

On 20/11/2014 06:57, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> s->xmit_pos maybe assigned to a negative value (-1),
> but in this branch variable s->xmit_pos as an index to
> array s->buffer. Let's add a check for s->xmit_pos.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/net/pcnet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
> index d344c15..2bb6417 100644
> --- a/hw/net/pcnet.c
> +++ b/hw/net/pcnet.c
> @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s)
>              s->xmit_pos = -1;
>              goto txdone;
>          }
> -        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
> +        if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) {
>              int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>              s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>                               s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
> 

Even better:

    if (s->xmit_pos < 0) {
        goto txdone;
    }
    bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
    s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
                     s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
    s->xmit_pos += bcnt;
    if (FIELD(tmd.status, TMDS, ENP)) {
#ifdef PCNET_DEBUG
        printf("pcnet_transmit size=%d\n", s->xmit_pos);
#endif
        if (CSR_LOOP(s)) {
            if (BCR_SWSTYLE(s) == 1)
        ...
    }

since there is duplicated code in the two "if" arms.  But the call is
Stefan's, as he's the maintainer.

Paolo

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Gonglei]

On 2014/11/20 14:36, Paolo Bonzini wrote:

> 
> 
> On 20/11/2014 06:57, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> s->xmit_pos maybe assigned to a negative value (-1),
>> but in this branch variable s->xmit_pos as an index to
>> array s->buffer. Let's add a check for s->xmit_pos.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  hw/net/pcnet.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/pcnet.c b/hw/net/pcnet.c
>> index d344c15..2bb6417 100644
>> --- a/hw/net/pcnet.c
>> +++ b/hw/net/pcnet.c
>> @@ -1247,7 +1247,7 @@ static void pcnet_transmit(PCNetState *s)
>>              s->xmit_pos = -1;
>>              goto txdone;
>>          }
>> -        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
>> +        if (!GET_FIELD(tmd.status, TMDS, ENP) && (s->xmit_pos >= 0)) {
>>              int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>>              s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>>                               s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>>
> 
> Even better:
> 
>     if (s->xmit_pos < 0) {
>         goto txdone;
>     }
>     bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>     s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>                      s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>     s->xmit_pos += bcnt;
>     if (FIELD(tmd.status, TMDS, ENP)) {
> #ifdef PCNET_DEBUG
>         printf("pcnet_transmit size=%d\n", s->xmit_pos);
> #endif
>         if (CSR_LOOP(s)) {
>             if (BCR_SWSTYLE(s) == 1)
>         ...
>     }
> 
> since there is duplicated code in the two "if" arms.

Maybe not, since two branch are "if and else if" not "if and else",
so this change make the below code segment's wide ...
>     bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>     s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>                      s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>     s->xmit_pos += bcnt;
... more extensive.

Best regards,
-Gonglei

>  But the call is
> Stefan's, as he's the maintainer.
> 
> Paolo

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

[Jason Wang]

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
>
>     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
>
>

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)

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Paolo Bonzini]

On 20/11/2014 07:44, Gonglei wrote:
> Maybe not, since two branch are "if and else if" not "if and else",
> so this change make the below code segment's wide ...
>> >     bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>> >     s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>> >                      s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>> >     s->xmit_pos += bcnt;
> ... more extensive.

After your patch that fixes the coverity report, they are

   if (a && b)
   else if (b)

so you can change it to

   if (!b) goto txdone;
   if (a) ...
   else ...

and then

   if (!b) goto txdone;
   <common part>
   if (!a) {
       <extra part from else>
   }

Paolo

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

[Gonglei]

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

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Gonglei]

On 2014/11/20 15:08, Paolo Bonzini wrote:

> 
> 
> On 20/11/2014 07:44, Gonglei wrote:
>> Maybe not, since two branch are "if and else if" not "if and else",
>> so this change make the below code segment's wide ...
>>>>     bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>>>>     s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>>>>                      s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>>>>     s->xmit_pos += bcnt;
>> ... more extensive.
> 
> After your patch that fixes the coverity report, they are
> 
>    if (a && b)
>    else if (b)
> 
> so you can change it to
> 
>    if (!b) goto txdone;
>    if (a) ...
>    else ...
> 
> and then
> 
>    if (!b) goto txdone;
>    <common part>
>    if (!a) {
>        <extra part from else>
>    }
> 
> Paolo

I know your mean now, thanks ;)
What about this below way? Maybe more clear.

        if (s->xmit_pos < 0) {
            goto txdone;
        }
        int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
        s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
                         s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
        s->xmit_pos += bcnt;

        if (!GET_FIELD(tmd.status, TMDS, ENP)) {
            goto txdone;
        }

#ifdef PCNET_DEBUG
        printf("pcnet_transmit size=%d\n", s->xmit_pos);
#endif
        if (CSR_LOOP(s)) {
            if (BCR_SWSTYLE(s) == 1)
                add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
            s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
            pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
            s->looptest = 0;
        } else
            if (s->nic)
                qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
                                 s->xmit_pos);

        s->csr[0] &= ~0x0008;   /* clear TDMD */
        s->csr[4] |= 0x0004;    /* set TXSTRT */
        s->xmit_pos = -1;

 txdone:

Best regards,
-Gonglei

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, 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
>

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.

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

[Gonglei]

On 2014/11/20 15:50, Jason Wang wrote:

>>> 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.

Sorry, I don't understand. But,
what's your meaning "initialize iov unconditionally at the beginning"?

Best regards,
-Gonglei

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

[Jason Wang]

On 11/20/2014 04:05 PM, Gonglei wrote:
> On 2014/11/20 15:50, Jason Wang wrote:
>
>>>> 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.
> Sorry, I don't understand. But,
> what's your meaning "initialize iov unconditionally at the beginning"?

Something like:

@@ -1774,7 +1774,12 @@ static uint32_t
rtl8139_RxConfig_read(RTL8139State *s)
 static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
     int do_interrupt, const uint8_t *dot1q_buf)
 {
-    struct iovec *iov = NULL;
+    struct iovec iov[3] = {
+        { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
+        { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
+        { .iov_base = buf + ETHER_ADDR_LEN * 2,
+          .iov_len = size - ETHER_ADDR_LEN * 2 },
+    };

and assign dot1q_buf to NULL is size is not ok.

Just a suggestion, your call.

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

[Gonglei]

On 2014/11/20 16:11, Jason Wang wrote:

> On 11/20/2014 04:05 PM, Gonglei wrote:
>> On 2014/11/20 15:50, Jason Wang wrote:
>>
>>>>> 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.
>> Sorry, I don't understand. But,
>> what's your meaning "initialize iov unconditionally at the beginning"?
> 
> Something like:
> 
> @@ -1774,7 +1774,12 @@ static uint32_t
> rtl8139_RxConfig_read(RTL8139State *s)
>  static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>      int do_interrupt, const uint8_t *dot1q_buf)
>  {
> -    struct iovec *iov = NULL;
> +    struct iovec iov[3] = {
> +        { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
> +        { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
> +        { .iov_base = buf + ETHER_ADDR_LEN * 2,
> +          .iov_len = size - ETHER_ADDR_LEN * 2 },
> +    };
> 
> and assign dot1q_buf to NULL is size is not ok.
> 

If "size <  ETHER_ADDR_LEN * 2", .iov_len = size - ETHER_ADDR_LEN * 2 will be
negative value;
and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect?

> Just a suggestion, your call.

Thanks, Jason :)

Best regards,
-Gonglei

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

[Jason Wang]

On 11/20/2014 04:18 PM, Gonglei wrote:
> On 2014/11/20 16:11, Jason Wang wrote:
>
>> On 11/20/2014 04:05 PM, Gonglei wrote:
>>> On 2014/11/20 15:50, Jason Wang wrote:
>>>
>>>>>> 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.
>>> Sorry, I don't understand. But,
>>> what's your meaning "initialize iov unconditionally at the beginning"?
>> Something like:
>>
>> @@ -1774,7 +1774,12 @@ static uint32_t
>> rtl8139_RxConfig_read(RTL8139State *s)
>>  static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>>      int do_interrupt, const uint8_t *dot1q_buf)
>>  {
>> -    struct iovec *iov = NULL;
>> +    struct iovec iov[3] = {
>> +        { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
>> +        { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
>> +        { .iov_base = buf + ETHER_ADDR_LEN * 2,
>> +          .iov_len = size - ETHER_ADDR_LEN * 2 },
>> +    };
>>
>> and assign dot1q_buf to NULL is size is not ok.
>>
> If "size <  ETHER_ADDR_LEN * 2", .iov_len = size - ETHER_ADDR_LEN * 2 will be
> negative value;
> and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect?

Then you need check dot1q_buf instead of iov after. Iov won't be used if
dot1q_buf is NULL.
>
>> Just a suggestion, your call.
> Thanks, Jason :)
>
> Best regards,
> -Gonglei
>

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

[Gonglei]

On 2014/11/20 16:24, Jason Wang wrote:

> On 11/20/2014 04:18 PM, Gonglei wrote:
>> On 2014/11/20 16:11, Jason Wang wrote:
>>
>>> On 11/20/2014 04:05 PM, Gonglei wrote:
>>>> On 2014/11/20 15:50, Jason Wang wrote:
>>>>
>>>>>>> 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.
>>>> Sorry, I don't understand. But,
>>>> what's your meaning "initialize iov unconditionally at the beginning"?
>>> Something like:
>>>
>>> @@ -1774,7 +1774,12 @@ static uint32_t
>>> rtl8139_RxConfig_read(RTL8139State *s)
>>>  static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>>>      int do_interrupt, const uint8_t *dot1q_buf)
>>>  {
>>> -    struct iovec *iov = NULL;
>>> +    struct iovec iov[3] = {
>>> +        { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
>>> +        { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
>>> +        { .iov_base = buf + ETHER_ADDR_LEN * 2,
>>> +          .iov_len = size - ETHER_ADDR_LEN * 2 },
>>> +    };
>>>
>>> and assign dot1q_buf to NULL is size is not ok.
>>>
>> If "size <  ETHER_ADDR_LEN * 2", .iov_len = size - ETHER_ADDR_LEN * 2 will be
>> negative value;
>> and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect?
> 
> Then you need check dot1q_buf instead of iov after. Iov won't be used if
> dot1q_buf is NULL.
>>

But that's  hacking IMHO.  Let's don't do this. ;)

>>> Just a suggestion, your call.
>> Thanks, Jason :)
>>
>> Best regards,
>> -Gonglei
>>
> 

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

[Paolo Bonzini]

On 20/11/2014 09:24, Jason Wang wrote:
> On 11/20/2014 04:18 PM, Gonglei wrote:
>> On 2014/11/20 16:11, Jason Wang wrote:
>>
>>> On 11/20/2014 04:05 PM, Gonglei wrote:
>>>> On 2014/11/20 15:50, Jason Wang wrote:
>>>>
>>>>>>> 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.
>>>> Sorry, I don't understand. But,
>>>> what's your meaning "initialize iov unconditionally at the beginning"?
>>> Something like:
>>>
>>> @@ -1774,7 +1774,12 @@ static uint32_t
>>> rtl8139_RxConfig_read(RTL8139State *s)
>>>  static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>>>      int do_interrupt, const uint8_t *dot1q_buf)
>>>  {
>>> -    struct iovec *iov = NULL;
>>> +    struct iovec iov[3] = {
>>> +        { .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 },
>>> +        { .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN },
>>> +        { .iov_base = buf + ETHER_ADDR_LEN * 2,
>>> +          .iov_len = size - ETHER_ADDR_LEN * 2 },
>>> +    };
>>>
>>> and assign dot1q_buf to NULL is size is not ok.
>>>
>> If "size <  ETHER_ADDR_LEN * 2", .iov_len = size - ETHER_ADDR_LEN * 2 will be
>> negative value;
>> and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect?
> 
> Then you need check dot1q_buf instead of iov after. Iov won't be used if
> dot1q_buf is NULL.

Or just ignore the rules and use an initializer in the middle of the code:

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

    struct iovec iov[3] = { ... };

and then check dot1q_buf instead of iov.  Plenty of choices, choose your
favorite.

Paolo

Re: [Qemu-devel] [PATCH 3/4] pcnet: fix Negative array index read

[Paolo Bonzini]

On 20/11/2014 08:38, Gonglei wrote:
> On 2014/11/20 15:08, Paolo Bonzini wrote:
> 
>>
>>
>> On 20/11/2014 07:44, Gonglei wrote:
>>> Maybe not, since two branch are "if and else if" not "if and else",
>>> so this change make the below code segment's wide ...
>>>>>     bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>>>>>     s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>>>>>                      s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>>>>>     s->xmit_pos += bcnt;
>>> ... more extensive.
>>
>> After your patch that fixes the coverity report, they are
>>
>>    if (a && b)
>>    else if (b)
>>
>> so you can change it to
>>
>>    if (!b) goto txdone;
>>    if (a) ...
>>    else ...
>>
>> and then
>>
>>    if (!b) goto txdone;
>>    <common part>
>>    if (!a) {
>>        <extra part from else>
>>    }
>>
>> Paolo
> 
> I know your mean now, thanks ;)
> What about this below way? Maybe more clear.

As you prefer.

Paolo

>         if (s->xmit_pos < 0) {
>             goto txdone;
>         }
>         int bcnt = 4096 - GET_FIELD(tmd.length, TMDL, BCNT);
>         s->phys_mem_read(s->dma_opaque, PHYSADDR(s, tmd.tbadr),
>                          s->buffer + s->xmit_pos, bcnt, CSR_BSWP(s));
>         s->xmit_pos += bcnt;
> 
>         if (!GET_FIELD(tmd.status, TMDS, ENP)) {
>             goto txdone;
>         }
> 
> #ifdef PCNET_DEBUG
>         printf("pcnet_transmit size=%d\n", s->xmit_pos);
> #endif
>         if (CSR_LOOP(s)) {
>             if (BCR_SWSTYLE(s) == 1)
>                 add_crc = !GET_FIELD(tmd.status, TMDS, NOFCS);
>             s->looptest = add_crc ? PCNET_LOOPTEST_CRC : PCNET_LOOPTEST_NOCRC;
>             pcnet_receive(qemu_get_queue(s->nic), s->buffer, s->xmit_pos);
>             s->looptest = 0;
>         } else
>             if (s->nic)
>                 qemu_send_packet(qemu_get_queue(s->nic), s->buffer,
>                                  s->xmit_pos);
> 
>         s->csr[0] &= ~0x0008;   /* clear TDMD */
>         s->csr[4] |= 0x0004;    /* set TXSTRT */
>         s->xmit_pos = -1;
> 
>  txdone:
> 
> Best regards,
> -Gonglei
> 
> 

Re: [Qemu-devel] [PATCH 1/4] net/slirp: fix memory leak

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Thu, Nov 20, 2014 at 01:57:11PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> commit b412eb61 introduce 'cmd:' target for guestfwd,
> and fwd don't be used in this scenario, and will leak
> memory in true branch with 'cmd:'. Let's allocate memory
> for fwd variable just in else statement.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/slirp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 2/4] net/socket: fix Uninitialized scalar variable

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Thu, Nov 20, 2014 at 01:57:12PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If is_connected parameter is false, the saddr
> variable will no initialize. Coverity report:
> uninit_use: Using uninitialized value saddr.sin_port.
> 
> We don't need add saddr information to nc->info_str
> when is_connected is false.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/socket.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/4] net: fix high impact outstanding defects reported by Coverity

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On Thu, Nov 20, 2014 at 01:57:10PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Please see details in every patch.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Gonglei (4):
>   net/slirp: fix memory leak
>   net/socket: fix Uninitialized scalar variable
>   pcnet: fix Negative array index read
>   rtl8139: fix Pointer to local outside scope
> 
>  hw/net/pcnet.c   |  2 +-
>  hw/net/rtl8139.c | 36 ++++++++++++------------------------
>  net/slirp.c      |  3 +--
>  net/socket.c     | 11 ++++++-----
>  4 files changed, 20 insertions(+), 32 deletions(-)

Thanks, please respin with updated Patch 3 & 4.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/4] net: fix high impact outstanding defects reported by Coverity

[Gonglei]

On 2014/11/20 19:51, Stefan Hajnoczi wrote:

> On Thu, Nov 20, 2014 at 01:57:10PM +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Please see details in every patch.
>>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> Gonglei (4):
>>   net/slirp: fix memory leak
>>   net/socket: fix Uninitialized scalar variable
>>   pcnet: fix Negative array index read
>>   rtl8139: fix Pointer to local outside scope
>>
>>  hw/net/pcnet.c   |  2 +-
>>  hw/net/rtl8139.c | 36 ++++++++++++------------------------
>>  net/slirp.c      |  3 +--
>>  net/socket.c     | 11 ++++++-----
>>  4 files changed, 20 insertions(+), 32 deletions(-)
> 
> Thanks, please respin with updated Patch 3 & 4.
> 
> Stefan

Hi, Stefan
I had posted v2 a few minutes ago :)
 [PATCH v2 for-2.2 0/4] net: fix high impact outstanding defects reported by Coverity

Best regards,
-Gonglei