| Message ID | 20180613194304.31548-1-aconole@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] lldp: fix string warnings | expand |
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 >
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.
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.
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.
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 */
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(-)