[ovs-dev,1/2] lldp: fix string warnings
diff mbox series

Message ID 20180613194304.31548-1-aconole@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev,1/2] lldp: fix string warnings
Related show

Commit Message

Aaron Conole June 13, 2018, 7:43 p.m. UTC
lib/lldp/lldpd.c: In function :
lib/lldp/lldpd.c:520:17: warning:  output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
                strncat(buffer, cfg->g_protocols[i].name,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                strlen(cfg->g_protocols[i].name));
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

lib/lldp/lldpd.c: In function :
lib/lldp/lldpd.c:519:17: warning:  specified bound 2 equals source length [-Wstringop-overflow=]
                strncat(buffer, ", ", 2);
                ^~~~~~~~~~~~~~~~~~~~~~~~

Closer inspection shows that buffer is only used to output protocol names
when debug logging is enabled, so restructure the code a bit as well.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/lldp/lldpd.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

Comments

Yifeng Sun June 14, 2018, 5:34 p.m. UTC | #1
Thanks for the improvement.

Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>

On Wed, Jun 13, 2018 at 12:43 PM, Aaron Conole <aconole@redhat.com> wrote:

> lib/lldp/lldpd.c: In function :
> lib/lldp/lldpd.c:520:17: warning:  output truncated before terminating nul
> copying as many bytes from a string as its length [-Wstringop-truncation]
>                 strncat(buffer, cfg->g_protocols[i].name,
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                 strlen(cfg->g_protocols[i].name));
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> lib/lldp/lldpd.c: In function :
> lib/lldp/lldpd.c:519:17: warning:  specified bound 2 equals source length
> [-Wstringop-overflow=]
>                 strncat(buffer, ", ", 2);
>                 ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Closer inspection shows that buffer is only used to output protocol names
> when debug logging is enabled, so restructure the code a bit as well.
>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/lldp/lldpd.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
> index ff5e62846..e9f3b6355 100644
> --- a/lib/lldp/lldpd.c
> +++ b/lib/lldp/lldpd.c
> @@ -40,6 +40,7 @@
>  #include <sys/utsname.h>
>  #endif
>  #include "compiler.h"
> +#include "openvswitch/dynamic-string.h"
>  #include "openvswitch/list.h"
>  #include "packets.h"
>  #include "timeval.h"
> @@ -417,7 +418,6 @@ lldpd_hide_ports(struct lldpd *cfg,
>                   int mask) {
>      struct lldpd_port *port;
>      int protocols[LLDPD_MODE_MAX + 1];
> -    char buffer[256];
>      bool found = false;
>      int i, j, k;
>      unsigned int min;
> @@ -505,29 +505,27 @@ lldpd_hide_ports(struct lldpd *cfg,
>          j++;
>      }
>
> -    buffer[0] = '\0';
> -    for (i = 0; cfg->g_protocols[i].mode != 0; i++) {
> -        if (cfg->g_protocols[i].enabled &&
> -            protocols[cfg->g_protocols[i].mode]) {
> -            if (strlen(buffer) +
> -                strlen(cfg->g_protocols[i].name) + 3 > sizeof(buffer)) {
> -                /* Unlikely, our buffer is too small */
> -                memcpy(buffer + sizeof(buffer) - 4, "...", 4);
> -                break;
> -            }
> -            if (buffer[0]) {
> -                strncat(buffer, ", ", 2);
> -                strncat(buffer, cfg->g_protocols[i].name,
> -                strlen(cfg->g_protocols[i].name));
> +    if (VLOG_IS_DBG_ENABLED()) {
> +        struct ds buffer;
> +        ds_init(&buffer);
> +        for (i = 0; cfg->g_protocols[i].mode != 0; i++) {
> +            if (cfg->g_protocols[i].enabled &&
> +                protocols[cfg->g_protocols[i].mode]) {
> +                if (*ds_cstr(&buffer)) {
> +                    ds_put_cstr(&buffer, ", ");
> +                }
> +                ds_put_cstr(&buffer, cfg->g_protocols[i].name);
>              }
>          }
> +        VLOG_DBG("%s: %s: %d visible neighbors (out of %d)",
> +                 hw->h_ifname,
> +                 (mask == SMART_OUTGOING) ? "out filter" : "in filter",
> +                 k, j);
> +        VLOG_DBG("%s: protocols: %s",
> +                 hw->h_ifname,
> +                 *ds_cstr(&buffer) ? ds_cstr(&buffer) : "(none)");
> +        ds_destroy(&buffer);
>      }
> -    VLOG_DBG("%s: %s: %d visible neighbors (out of %d)",
> -             hw->h_ifname,
> -             (mask == SMART_OUTGOING) ? "out filter" : "in filter",
> -             k, j);
> -    VLOG_DBG("%s: protocols: %s",
> -             hw->h_ifname, buffer[0] ? buffer : "(none)");
>  }
>
>  /* Hide unwanted ports depending on smart mode set by the user */
> --
> 2.14.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff June 14, 2018, 8:23 p.m. UTC | #2
On Wed, Jun 13, 2018 at 03:43:03PM -0400, Aaron Conole wrote:
> lib/lldp/lldpd.c: In function :
> lib/lldp/lldpd.c:520:17: warning:  output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation]
>                 strncat(buffer, cfg->g_protocols[i].name,
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>                 strlen(cfg->g_protocols[i].name));
>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> lib/lldp/lldpd.c: In function :
> lib/lldp/lldpd.c:519:17: warning:  specified bound 2 equals source length [-Wstringop-overflow=]
>                 strncat(buffer, ", ", 2);
>                 ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Closer inspection shows that buffer is only used to output protocol names
> when debug logging is enabled, so restructure the code a bit as well.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>

Thanks, Aaron.  I made a few further simplifications and applied this to
master.

What version of GCC are you using?  Are you turning up the warning
level explicitly?  I don't get these warnings with OVS default warnings
and GCC 7.2.

Thanks,

Ben.
Aaron Conole June 14, 2018, 9:10 p.m. UTC | #3
Ben Pfaff <blp@ovn.org> writes:

> On Wed, Jun 13, 2018 at 03:43:03PM -0400, Aaron Conole wrote:
>> lib/lldp/lldpd.c: In function :
>> lib/lldp/lldpd.c:520:17: warning: output truncated before
>> terminating nul copying as many bytes from a string as its length
>> [-Wstringop-truncation]
>>                 strncat(buffer, cfg->g_protocols[i].name,
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>                 strlen(cfg->g_protocols[i].name));
>>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> lib/lldp/lldpd.c: In function :
>> lib/lldp/lldpd.c:519:17: warning: specified bound 2 equals source
>> length [-Wstringop-overflow=]
>>                 strncat(buffer, ", ", 2);
>>                 ^~~~~~~~~~~~~~~~~~~~~~~~
>> 
>> Closer inspection shows that buffer is only used to output protocol names
>> when debug logging is enabled, so restructure the code a bit as well.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>
> Thanks, Aaron.  I made a few further simplifications and applied this to
> master.
>
> What version of GCC are you using?  Are you turning up the warning
> level explicitly?  I don't get these warnings with OVS default warnings
> and GCC 7.2.

GCC 8.1.1

Shiny and new until the next release.

> Thanks,
>
> Ben.
Ben Pfaff June 14, 2018, 9:50 p.m. UTC | #4
On Thu, Jun 14, 2018 at 05:10:10PM -0400, Aaron Conole wrote:
> Ben Pfaff <blp@ovn.org> writes:
> 
> > On Wed, Jun 13, 2018 at 03:43:03PM -0400, Aaron Conole wrote:
> >> lib/lldp/lldpd.c: In function :
> >> lib/lldp/lldpd.c:520:17: warning: output truncated before
> >> terminating nul copying as many bytes from a string as its length
> >> [-Wstringop-truncation]
> >>                 strncat(buffer, cfg->g_protocols[i].name,
> >>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>                 strlen(cfg->g_protocols[i].name));
> >>                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> 
> >> lib/lldp/lldpd.c: In function :
> >> lib/lldp/lldpd.c:519:17: warning: specified bound 2 equals source
> >> length [-Wstringop-overflow=]
> >>                 strncat(buffer, ", ", 2);
> >>                 ^~~~~~~~~~~~~~~~~~~~~~~~
> >> 
> >> Closer inspection shows that buffer is only used to output protocol names
> >> when debug logging is enabled, so restructure the code a bit as well.
> >> 
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >
> > Thanks, Aaron.  I made a few further simplifications and applied this to
> > master.
> >
> > What version of GCC are you using?  Are you turning up the warning
> > level explicitly?  I don't get these warnings with OVS default warnings
> > and GCC 7.2.
> 
> GCC 8.1.1
> 
> Shiny and new until the next release.

Thanks.

I sent a patch to enable GCC 8.x new warning options:
        https://patchwork.ozlabs.org/patch/929717/
I also switched to using it here myself.

Patch
diff mbox series

diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index ff5e62846..e9f3b6355 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -40,6 +40,7 @@ 
 #include <sys/utsname.h>
 #endif
 #include "compiler.h"
+#include "openvswitch/dynamic-string.h"
 #include "openvswitch/list.h"
 #include "packets.h"
 #include "timeval.h"
@@ -417,7 +418,6 @@  lldpd_hide_ports(struct lldpd *cfg,
                  int mask) {
     struct lldpd_port *port;
     int protocols[LLDPD_MODE_MAX + 1];
-    char buffer[256];
     bool found = false;
     int i, j, k;
     unsigned int min;
@@ -505,29 +505,27 @@  lldpd_hide_ports(struct lldpd *cfg,
         j++;
     }
 
-    buffer[0] = '\0';
-    for (i = 0; cfg->g_protocols[i].mode != 0; i++) {
-        if (cfg->g_protocols[i].enabled &&
-            protocols[cfg->g_protocols[i].mode]) {
-            if (strlen(buffer) +
-                strlen(cfg->g_protocols[i].name) + 3 > sizeof(buffer)) {
-                /* Unlikely, our buffer is too small */
-                memcpy(buffer + sizeof(buffer) - 4, "...", 4);
-                break;
-            }
-            if (buffer[0]) {
-                strncat(buffer, ", ", 2);
-                strncat(buffer, cfg->g_protocols[i].name,
-                strlen(cfg->g_protocols[i].name));
+    if (VLOG_IS_DBG_ENABLED()) {
+        struct ds buffer;
+        ds_init(&buffer);
+        for (i = 0; cfg->g_protocols[i].mode != 0; i++) {
+            if (cfg->g_protocols[i].enabled &&
+                protocols[cfg->g_protocols[i].mode]) {
+                if (*ds_cstr(&buffer)) {
+                    ds_put_cstr(&buffer, ", ");
+                }
+                ds_put_cstr(&buffer, cfg->g_protocols[i].name);
             }
         }
+        VLOG_DBG("%s: %s: %d visible neighbors (out of %d)",
+                 hw->h_ifname,
+                 (mask == SMART_OUTGOING) ? "out filter" : "in filter",
+                 k, j);
+        VLOG_DBG("%s: protocols: %s",
+                 hw->h_ifname,
+                 *ds_cstr(&buffer) ? ds_cstr(&buffer) : "(none)");
+        ds_destroy(&buffer);
     }
-    VLOG_DBG("%s: %s: %d visible neighbors (out of %d)",
-             hw->h_ifname,
-             (mask == SMART_OUTGOING) ? "out filter" : "in filter",
-             k, j);
-    VLOG_DBG("%s: protocols: %s",
-             hw->h_ifname, buffer[0] ? buffer : "(none)");
 }
 
 /* Hide unwanted ports depending on smart mode set by the user */