| Message ID | 1572470787-2935-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] pinctrl.c: Fix maybe-uninitialized warnings. | expand |
On Thu, Oct 31, 2019 at 2:57 AM Han Zhou <hzhou@ovn.org> wrote: > > There are warnings like: > === > ../controller/pinctrl.c: In function ‘ipv6_ra_send’: > ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > memcpy(&dnssl[i], t1, strlen(t1)); > ^ > ../controller/pinctrl.c:2383:20: note: ‘r1’ was declared here > char *t1, *r1; > ^ > === > This is not a real problem but compile fails because of it. This patch > fixes it by initializing the pointers to NULL, to avoid the warnings. > > CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Fixes: 5a12a940f63a ("Add DNSSL support to OVN") > Signed-off-by: Han Zhou <hzhou@ovn.org> Acked-by: Numan Siddique <numans@ovn.org> I didn't see this warning with --enable-Werror and --enable-sparse. Do you enable any other flags ? I have gcc version - gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) in my Fedora 30. Thanks Numan > --- > controller/pinctrl.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 655ba54..a90ee73 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -2369,7 +2369,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, > size_t prev_l4_size = dp_packet_l4_size(b); > size_t size = sizeof(struct ovs_nd_dnssl); > struct ip6_hdr *nh = dp_packet_l3(b); > - char *t0, *r0, dnssl[255] = {}; > + char *t0, *r0 = NULL, dnssl[255] = {}; > int i = 0; > > /* Multiple DNS Search List must be 'comma' separated > @@ -2380,7 +2380,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, > */ > for (t0 = strtok_r(dnssl_list, ",", &r0); t0; > t0 = strtok_r(NULL, ",", &r0)) { > - char *t1, *r1; > + char *t1, *r1 = NULL; > > size += strlen(t0) + 2; > if (size > sizeof(dnssl)) { > -- > 2.1.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Oct 30, 2019 at 02:26:27PM -0700, Han Zhou wrote: > ../controller/pinctrl.c: In function ‘ipv6_ra_send’: > ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > memcpy(&dnssl[i], t1, strlen(t1)); > ^ This is weird: the message is about r1 but the line of code it cites does not mention r1. Is there some macro trickery or something else unusual going on?
On Thu, Oct 31, 2019 at 9:21 AM Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Oct 30, 2019 at 02:26:27PM -0700, Han Zhou wrote: > > ../controller/pinctrl.c: In function ‘ipv6_ra_send’: > > ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > memcpy(&dnssl[i], t1, strlen(t1)); > > ^ > > This is weird: the message is about r1 but the line of code it cites > does not mention r1. Is there some macro trickery or something else > unusual going on? Yes, it's weird. I think this is a bug of gcc that it complained in the wrong line. The real line with the use of r1 is in the for() line. If I remove the memcpy() line, the error just reports the next line, and it disappears when I remove all lines in the for block.
On Thu, Oct 31, 2019 at 9:00 AM Numan Siddique <nusiddiq@redhat.com> wrote: > > On Thu, Oct 31, 2019 at 2:57 AM Han Zhou <hzhou@ovn.org> wrote: > > > > There are warnings like: > > === > > ../controller/pinctrl.c: In function ‘ipv6_ra_send’: > > ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > memcpy(&dnssl[i], t1, strlen(t1)); > > ^ > > ../controller/pinctrl.c:2383:20: note: ‘r1’ was declared here > > char *t1, *r1; > > ^ > > === > > This is not a real problem but compile fails because of it. This patch > > fixes it by initializing the pointers to NULL, to avoid the warnings. > > > > CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Fixes: 5a12a940f63a ("Add DNSSL support to OVN") > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > Acked-by: Numan Siddique <numans@ovn.org> > > I didn't see this warning with --enable-Werror and --enable-sparse. Do > you enable any other flags ? > I have gcc version - gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) in my Fedora 30. > > Thanks > Numan > Thanks for review. Yes, it seems to be only a problem with old GCC. I see all the other places in the repo that use strtok_r() are with the saveptr parameter initialized to NULL before use. I will update the commit message stating it is only for working with old GCC version. > > --- > > controller/pinctrl.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 655ba54..a90ee73 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -2369,7 +2369,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, > > size_t prev_l4_size = dp_packet_l4_size(b); > > size_t size = sizeof(struct ovs_nd_dnssl); > > struct ip6_hdr *nh = dp_packet_l3(b); > > - char *t0, *r0, dnssl[255] = {}; > > + char *t0, *r0 = NULL, dnssl[255] = {}; > > int i = 0; > > > > /* Multiple DNS Search List must be 'comma' separated > > @@ -2380,7 +2380,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, > > */ > > for (t0 = strtok_r(dnssl_list, ",", &r0); t0; > > t0 = strtok_r(NULL, ",", &r0)) { > > - char *t1, *r1; > > + char *t1, *r1 = NULL; > > > > size += strlen(t0) + 2; > > if (size > sizeof(dnssl)) { > > -- > > 2.1.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Thu, Oct 31, 2019 at 11:54 AM Han Zhou <hzhou@ovn.org> wrote: > > > > On Thu, Oct 31, 2019 at 9:00 AM Numan Siddique <nusiddiq@redhat.com> wrote: > > > > On Thu, Oct 31, 2019 at 2:57 AM Han Zhou <hzhou@ovn.org> wrote: > > > > > > There are warnings like: > > > === > > > ../controller/pinctrl.c: In function ‘ipv6_ra_send’: > > > ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > memcpy(&dnssl[i], t1, strlen(t1)); > > > ^ > > > ../controller/pinctrl.c:2383:20: note: ‘r1’ was declared here > > > char *t1, *r1; > > > ^ > > > === > > > This is not a real problem but compile fails because of it. This patch > > > fixes it by initializing the pointers to NULL, to avoid the warnings. > > > > > > CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > Fixes: 5a12a940f63a ("Add DNSSL support to OVN") > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > > Acked-by: Numan Siddique <numans@ovn.org> > > > > I didn't see this warning with --enable-Werror and --enable-sparse. Do > > you enable any other flags ? > > I have gcc version - gcc (GCC) 9.2.1 20190827 (Red Hat 9.2.1-1) in my Fedora 30. > > > > Thanks > > Numan > > > > Thanks for review. Yes, it seems to be only a problem with old GCC. I see all the other places in the repo that use strtok_r() are with the saveptr parameter initialized to NULL before use. I will update the commit message stating it is only for working with old GCC version. > I applied this to master with commit message changed as: --- pinctrl.c: Fix maybe-uninitialized warnings with old GCC versions. There are warnings with older GCC versions (e.g. 4.9.2): === ../controller/pinctrl.c: In function ‘ipv6_ra_send’: ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] memcpy(&dnssl[i], t1, strlen(t1)); ^ ../controller/pinctrl.c:2383:20: note: ‘r1’ was declared here char *t1, *r1; ^ === This is not a real problem but compile fails because of it. This patch fixes it by initializing the pointers to NULL, to avoid the warnings with older GCC. CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Fixes: 5a12a940f63a ("Add DNSSL support to OVN") Acked-by: Numan Siddique <nusiddiq@redhat.com> Signed-off-by: Han Zhou <hzhou@ovn.org> ---
On Thu, Oct 31, 2019 at 12:11:39PM -0700, Han Zhou wrote: > Thanks for review. Yes, it seems to be only a problem with old GCC. I see > all the other places in the repo that use strtok_r() are with the saveptr > parameter initialized to NULL before use. Yes, that's exactly because GCC tends to complain otherwise. Initializing saveptr is the right thing to do.
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 655ba54..a90ee73 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2369,7 +2369,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, size_t prev_l4_size = dp_packet_l4_size(b); size_t size = sizeof(struct ovs_nd_dnssl); struct ip6_hdr *nh = dp_packet_l3(b); - char *t0, *r0, dnssl[255] = {}; + char *t0, *r0 = NULL, dnssl[255] = {}; int i = 0; /* Multiple DNS Search List must be 'comma' separated @@ -2380,7 +2380,7 @@ packet_put_ra_dnssl_opt(struct dp_packet *b, ovs_be32 lifetime, */ for (t0 = strtok_r(dnssl_list, ",", &r0); t0; t0 = strtok_r(NULL, ",", &r0)) { - char *t1, *r1; + char *t1, *r1 = NULL; size += strlen(t0) + 2; if (size > sizeof(dnssl)) {
There are warnings like: === ../controller/pinctrl.c: In function ‘ipv6_ra_send’: ../controller/pinctrl.c:2393:13: error: ‘r1’ may be used uninitialized in this function [-Werror=maybe-uninitialized] memcpy(&dnssl[i], t1, strlen(t1)); ^ ../controller/pinctrl.c:2383:20: note: ‘r1’ was declared here char *t1, *r1; ^ === This is not a real problem but compile fails because of it. This patch fixes it by initializing the pointers to NULL, to avoid the warnings. CC: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Fixes: 5a12a940f63a ("Add DNSSL support to OVN") Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/pinctrl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)