diff mbox

[ovs-dev,patch_v2] build: Fix memset with zero size warning.

Message ID 1490034289-77787-1-git-send-email-dlu998@gmail.com
State Superseded
Headers show

Commit Message

Darrell Ball March 20, 2017, 6:24 p.m. UTC 
In file included from /usr/include/string.h:640:0,
                  from ./lib/string.h:20,
                  from /usr/include/netinet/icmp6.h:22,
                  from ../lib/flow.h:21,
                  from ../lib/flow.c:18:
In function 'memset',
     inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
/usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
call to '__warn_memset_zero_len' declared with attribute warning:
memset used with constant zero length parameter; this could be
due to transposed parameters [-Werror]
        __warn_memset_zero_len ();
                               ^
cc1: all warnings being treated as errors
make[2]: *** [lib/flow.lo] Error 1

At the same time, fix some similar bounds checks related to
the same commit.

Signed-off-by: Darrell Ball <dlu998@gmail.com>
Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
CC: Eric Garver <e@erig.me>
---

v1->v2: Change some cosmetic bounds checks.

 lib/flow.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Eric Garver March 20, 2017, 7:32 p.m. UTC | #1 
On Mon, Mar 20, 2017 at 11:24:49AM -0700, Darrell Ball wrote:
> In file included from /usr/include/string.h:640:0,
>                   from ./lib/string.h:20,
>                   from /usr/include/netinet/icmp6.h:22,
>                   from ../lib/flow.h:21,
>                   from ../lib/flow.c:18:
> In function 'memset',
>      inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
> /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
> call to '__warn_memset_zero_len' declared with attribute warning:
> memset used with constant zero length parameter; this could be
> due to transposed parameters [-Werror]
>         __warn_memset_zero_len ();
>                                ^
> cc1: all warnings being treated as errors
> make[2]: *** [lib/flow.lo] Error 1
> 
> At the same time, fix some similar bounds checks related to
> the same commit.
> 
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
> CC: Eric Garver <e@erig.me>
> ---
> 
> v1->v2: Change some cosmetic bounds checks.
> 
>  lib/flow.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index f628526..8e7d3ee 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -2167,10 +2167,10 @@ void
>  flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
>  {
>      int n = flow_count_vlan_headers(flow);
> -    if (n == 0) {
> +    if (n <= 0) {
>          return;
>      }
> -    if (wc) {
> +    if ((wc) && (n > 1)) {

This is redundant because of the if (n <= 0) return above, but it's
fine.

>          memset(&wc->masks.vlans[1], 0xff,
>                 sizeof(union flow_vlan_hdr) * (n - 1));
>      }
> @@ -2184,7 +2184,9 @@ flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
>  {
>      if (wc) {
>          int n = flow_count_vlan_headers(flow);
> -        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> +        if (n > 0) {
> +            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
> +        }
>      }
>      memmove(&flow->vlans[1], &flow->vlans[0],
>              sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
> -- 
> 1.9.1
> 

Thanks for the clean up!

Acked-by: Eric Garver <e@erig.me>
Darrell Ball March 20, 2017, 8:26 p.m. UTC | #2 
On 3/20/17, 12:32 PM, "ovs-dev-bounces@openvswitch.org on behalf of Eric Garver" <ovs-dev-bounces@openvswitch.org on behalf of e@erig.me> wrote:

    On Mon, Mar 20, 2017 at 11:24:49AM -0700, Darrell Ball wrote:
    > In file included from /usr/include/string.h:640:0,
    >                   from ./lib/string.h:20,
    >                   from /usr/include/netinet/icmp6.h:22,
    >                   from ../lib/flow.h:21,
    >                   from ../lib/flow.c:18:
    > In function 'memset',
    >      inlined from 'flow_push_vlan_uninit' at ../lib/flow.c:2188:19:
    > /usr/include/x86_64-linux-gnu/bits/string3.h:81:30: error:
    > call to '__warn_memset_zero_len' declared with attribute warning:
    > memset used with constant zero length parameter; this could be
    > due to transposed parameters [-Werror]
    >         __warn_memset_zero_len ();
    >                                ^
    > cc1: all warnings being treated as errors
    > make[2]: *** [lib/flow.lo] Error 1
    > 
    > At the same time, fix some similar bounds checks related to
    > the same commit.
    > 
    > Signed-off-by: Darrell Ball <dlu998@gmail.com>
    > Fixes: f0fb825a3785 ("Add support for 802.1ad (QinQ tunneling)")
    > CC: Eric Garver <e@erig.me>
    > ---
    > 
    > v1->v2: Change some cosmetic bounds checks.
    > 
    >  lib/flow.c | 8 +++++---
    >  1 file changed, 5 insertions(+), 3 deletions(-)
    > 
    > diff --git a/lib/flow.c b/lib/flow.c
    > index f628526..8e7d3ee 100644
    > --- a/lib/flow.c
    > +++ b/lib/flow.c
    > @@ -2167,10 +2167,10 @@ void
    >  flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
    >  {
    >      int n = flow_count_vlan_headers(flow);
    > -    if (n == 0) {
    > +    if (n <= 0) {
    >          return;
    >      }
    > -    if (wc) {
    > +    if ((wc) && (n > 1)) {
    
    This is redundant because of the if (n <= 0) return above, but it's
    fine.

It is just for consistency w.r.t. memset size as per other gcc/glibc
warning, even though memset is safe with zero size. 
i.e. your code is functionally correct.
    
    >          memset(&wc->masks.vlans[1], 0xff,
    >                 sizeof(union flow_vlan_hdr) * (n - 1));
    >      }
    > @@ -2184,7 +2184,9 @@ flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
    >  {
    >      if (wc) {
    >          int n = flow_count_vlan_headers(flow);
    > -        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
    > +        if (n > 0) {
    > +            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
    > +        }
    >      }
    >      memmove(&flow->vlans[1], &flow->vlans[0],
    >              sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));
    > -- 
    > 1.9.1
    > 
    
    Thanks for the clean up!
    
    Acked-by: Eric Garver <e@erig.me>
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=94Xk72jLRHCUW_F-65jVvIes5pnWIgOdCNVKzAALs5I&s=HtjN2Mk985ZmYSnekbzOffPHRwhE2FZH-cCaZzdnoiQ&e=
diff mbox

Patch

diff --git a/lib/flow.c b/lib/flow.c
index f628526..8e7d3ee 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -2167,10 +2167,10 @@  void
 flow_pop_vlan(struct flow *flow, struct flow_wildcards *wc)
 {
     int n = flow_count_vlan_headers(flow);
-    if (n == 0) {
+    if (n <= 0) {
         return;
     }
-    if (wc) {
+    if ((wc) && (n > 1)) {
         memset(&wc->masks.vlans[1], 0xff,
                sizeof(union flow_vlan_hdr) * (n - 1));
     }
@@ -2184,7 +2184,9 @@  flow_push_vlan_uninit(struct flow *flow, struct flow_wildcards *wc)
 {
     if (wc) {
         int n = flow_count_vlan_headers(flow);
-        memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
+        if (n > 0) {
+            memset(wc->masks.vlans, 0xff, sizeof(union flow_vlan_hdr) * n);
+        }
     }
     memmove(&flow->vlans[1], &flow->vlans[0],
             sizeof(union flow_vlan_hdr) * (FLOW_MAX_VLAN_HEADERS - 1));