| Message ID | 1582286368-5120-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-northd: Fix use of uninitialized variables. | expand |
On 2/21/20 12:59 PM, Dumitru Ceara wrote: > Calls to ip_address_and_port_from_lb_key() could fail parsing the 'key' > argument and would return without setting *ip_address. The code in > ovn_lb_create() was passing an unitialized 'backend_ip' pointer and > using it unconditionally afterwards. > > With CFLAGS="-O3" gcc reports this issue too: > $ ./configure CFLAGS="-O3" --enable-Werror [...] > $ make > [...] > northd/ovn-northd.c: In function ‘ovnnb_db_run.isra.65’: > northd/ovn-northd.c:3116:14: error: ‘backend_port’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > uint32_t hash = service_port; > ^ > northd/ovn-northd.c:3207:22: note: ‘backend_port’ was declared here > uint16_t backend_port; > ^ > In file included from northd/ovn-northd.c:27:0: > /home/dceara/git-repos/ovs/lib/hash.h:342:5: error: ‘backend_ip’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > return hash_bytes(s, strlen(s), basis); > ^ > northd/ovn-northd.c:3206:19: note: ‘backend_ip’ was declared here > char *backend_ip; > > Fix ip_address_and_port_from_lb_key() and make it return true if parsing > was successful. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovn-northd.c | 37 +++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 18 deletions(-) > Hi, If possible, please backport this to branch 20.03 too. Thanks, Dumitru
Looks good to me Acked-by: Mark Michelson <mmichels@redhat.com> On 2/21/20 6:59 AM, Dumitru Ceara wrote: > Calls to ip_address_and_port_from_lb_key() could fail parsing the 'key' > argument and would return without setting *ip_address. The code in > ovn_lb_create() was passing an unitialized 'backend_ip' pointer and > using it unconditionally afterwards. > > With CFLAGS="-O3" gcc reports this issue too: > $ ./configure CFLAGS="-O3" --enable-Werror [...] > $ make > [...] > northd/ovn-northd.c: In function ‘ovnnb_db_run.isra.65’: > northd/ovn-northd.c:3116:14: error: ‘backend_port’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > uint32_t hash = service_port; > ^ > northd/ovn-northd.c:3207:22: note: ‘backend_port’ was declared here > uint16_t backend_port; > ^ > In file included from northd/ovn-northd.c:27:0: > /home/dceara/git-repos/ovs/lib/hash.h:342:5: error: ‘backend_ip’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > return hash_bytes(s, strlen(s), basis); > ^ > northd/ovn-northd.c:3206:19: note: ‘backend_ip’ was declared here > char *backend_ip; > > Fix ip_address_and_port_from_lb_key() and make it return true if parsing > was successful. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovn-northd.c | 37 +++++++++++++++++++------------------ > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 721cb05..3aba048 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -2253,7 +2253,7 @@ join_logical_ports(struct northd_context *ctx, > } > } > > -static void > +static bool > ip_address_and_port_from_lb_key(const char *key, char **ip_address, > uint16_t *port, int *addr_family); > > @@ -2272,13 +2272,12 @@ get_router_load_balancer_ips(const struct ovn_datapath *od, > > SMAP_FOR_EACH (node, vips) { > /* node->key contains IP:port or just IP. */ > - char *ip_address = NULL; > + char *ip_address; > uint16_t port; > int addr_family; > > - ip_address_and_port_from_lb_key(node->key, &ip_address, &port, > - &addr_family); > - if (!ip_address) { > + if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port, > + &addr_family)) { > continue; > } > > @@ -3156,13 +3155,12 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > size_t n_vips = 0; > > SMAP_FOR_EACH (node, &nbrec_lb->vips) { > - char *vip = NULL; > + char *vip; > uint16_t port; > int addr_family; > > - ip_address_and_port_from_lb_key(node->key, &vip, &port, > - &addr_family); > - if (!vip) { > + if (!ip_address_and_port_from_lb_key(node->key, &vip, &port, > + &addr_family)) { > continue; > } > > @@ -3206,10 +3204,9 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, > char *backend_ip; > uint16_t backend_port; > > - ip_address_and_port_from_lb_key(token, &backend_ip, &backend_port, > - &addr_family); > - > - if (!backend_ip) { > + if (!ip_address_and_port_from_lb_key(token, &backend_ip, > + &backend_port, > + &addr_family)) { > continue; > } > > @@ -4682,8 +4679,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) > > /* For a 'key' of the form "IP:port" or just "IP", sets 'port' and > * 'ip_address'. The caller must free() the memory allocated for > - * 'ip_address'. */ > -static void > + * 'ip_address'. > + * Returns true if parsing of 'key' was successful, false otherwise. > + */ > +static bool > ip_address_and_port_from_lb_key(const char *key, char **ip_address, > uint16_t *port, int *addr_family) > { > @@ -4692,16 +4691,18 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address, > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s", > key); > - return; > + *ip_address = NULL; > + *port = 0; > + *addr_family = 0; > + return false; > } > > struct ds s = DS_EMPTY_INITIALIZER; > ss_format_address_nobracks(&ss, &s); > *ip_address = ds_steal_cstr(&s); > - > *port = ss_get_port(&ss); > - > *addr_family = ss.ss_family; > + return true; > } > > /* >
On Saturday, February 22, 2020, Mark Michelson <mmichels@redhat.com> wrote: > Looks good to me > > Acked-by: Mark Michelson <mmichels@redhat.com> I applied this patch to master and branch-20.03 Thanks Numan > > On 2/21/20 6:59 AM, Dumitru Ceara wrote: > >> Calls to ip_address_and_port_from_lb_key() could fail parsing the 'key' >> argument and would return without setting *ip_address. The code in >> ovn_lb_create() was passing an unitialized 'backend_ip' pointer and >> using it unconditionally afterwards. >> >> With CFLAGS="-O3" gcc reports this issue too: >> $ ./configure CFLAGS="-O3" --enable-Werror [...] >> $ make >> [...] >> northd/ovn-northd.c: In function ‘ovnnb_db_run.isra.65’: >> northd/ovn-northd.c:3116:14: error: ‘backend_port’ may be used >> uninitialized in this function [-Werror=maybe-uninitialized] >> uint32_t hash = service_port; >> ^ >> northd/ovn-northd.c:3207:22: note: ‘backend_port’ was declared here >> uint16_t backend_port; >> ^ >> In file included from northd/ovn-northd.c:27:0: >> /home/dceara/git-repos/ovs/lib/hash.h:342:5: error: ‘backend_ip’ may be >> used uninitialized in this function [-Werror=maybe-uninitialized] >> return hash_bytes(s, strlen(s), basis); >> ^ >> northd/ovn-northd.c:3206:19: note: ‘backend_ip’ was declared here >> char *backend_ip; >> >> Fix ip_address_and_port_from_lb_key() and make it return true if parsing >> was successful. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> northd/ovn-northd.c | 37 +++++++++++++++++++------------------ >> 1 file changed, 19 insertions(+), 18 deletions(-) >> >> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> index 721cb05..3aba048 100644 >> --- a/northd/ovn-northd.c >> +++ b/northd/ovn-northd.c >> @@ -2253,7 +2253,7 @@ join_logical_ports(struct northd_context *ctx, >> } >> } >> -static void >> +static bool >> ip_address_and_port_from_lb_key(const char *key, char **ip_address, >> uint16_t *port, int *addr_family); >> @@ -2272,13 +2272,12 @@ get_router_load_balancer_ips(const struct >> ovn_datapath *od, >> SMAP_FOR_EACH (node, vips) { >> /* node->key contains IP:port or just IP. */ >> - char *ip_address = NULL; >> + char *ip_address; >> uint16_t port; >> int addr_family; >> - ip_address_and_port_from_lb_key(node->key, &ip_address, >> &port, >> - &addr_family); >> - if (!ip_address) { >> + if (!ip_address_and_port_from_lb_key(node->key, >> &ip_address, &port, >> + &addr_family)) { >> continue; >> } >> @@ -3156,13 +3155,12 @@ ovn_lb_create(struct northd_context *ctx, >> struct hmap *lbs, >> size_t n_vips = 0; >> SMAP_FOR_EACH (node, &nbrec_lb->vips) { >> - char *vip = NULL; >> + char *vip; >> uint16_t port; >> int addr_family; >> - ip_address_and_port_from_lb_key(node->key, &vip, &port, >> - &addr_family); >> - if (!vip) { >> + if (!ip_address_and_port_from_lb_key(node->key, &vip, &port, >> + &addr_family)) { >> continue; >> } >> @@ -3206,10 +3204,9 @@ ovn_lb_create(struct northd_context *ctx, struct >> hmap *lbs, >> char *backend_ip; >> uint16_t backend_port; >> - ip_address_and_port_from_lb_key(token, &backend_ip, >> &backend_port, >> - &addr_family); >> - >> - if (!backend_ip) { >> + if (!ip_address_and_port_from_lb_key(token, &backend_ip, >> + &backend_port, >> + &addr_family)) { >> continue; >> } >> @@ -4682,8 +4679,10 @@ build_pre_acls(struct ovn_datapath *od, struct >> hmap *lflows) >> /* For a 'key' of the form "IP:port" or just "IP", sets 'port' and >> * 'ip_address'. The caller must free() the memory allocated for >> - * 'ip_address'. */ >> -static void >> + * 'ip_address'. >> + * Returns true if parsing of 'key' was successful, false otherwise. >> + */ >> +static bool >> ip_address_and_port_from_lb_key(const char *key, char **ip_address, >> uint16_t *port, int *addr_family) >> { >> @@ -4692,16 +4691,18 @@ ip_address_and_port_from_lb_key(const char *key, >> char **ip_address, >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key >> %s", >> key); >> - return; >> + *ip_address = NULL; >> + *port = 0; >> + *addr_family = 0; >> + return false; >> } >> struct ds s = DS_EMPTY_INITIALIZER; >> ss_format_address_nobracks(&ss, &s); >> *ip_address = ds_steal_cstr(&s); >> - >> *port = ss_get_port(&ss); >> - >> *addr_family = ss.ss_family; >> + return true; >> } >> /* >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 2/24/20 3:21 PM, Numan Siddique wrote: > > > On Saturday, February 22, 2020, Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > Looks good to me > > Acked-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > > I applied this patch to master and branch-20.03 > > Thanks > Numan > > Thanks Mark and Numan!
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 721cb05..3aba048 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -2253,7 +2253,7 @@ join_logical_ports(struct northd_context *ctx, } } -static void +static bool ip_address_and_port_from_lb_key(const char *key, char **ip_address, uint16_t *port, int *addr_family); @@ -2272,13 +2272,12 @@ get_router_load_balancer_ips(const struct ovn_datapath *od, SMAP_FOR_EACH (node, vips) { /* node->key contains IP:port or just IP. */ - char *ip_address = NULL; + char *ip_address; uint16_t port; int addr_family; - ip_address_and_port_from_lb_key(node->key, &ip_address, &port, - &addr_family); - if (!ip_address) { + if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port, + &addr_family)) { continue; } @@ -3156,13 +3155,12 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, size_t n_vips = 0; SMAP_FOR_EACH (node, &nbrec_lb->vips) { - char *vip = NULL; + char *vip; uint16_t port; int addr_family; - ip_address_and_port_from_lb_key(node->key, &vip, &port, - &addr_family); - if (!vip) { + if (!ip_address_and_port_from_lb_key(node->key, &vip, &port, + &addr_family)) { continue; } @@ -3206,10 +3204,9 @@ ovn_lb_create(struct northd_context *ctx, struct hmap *lbs, char *backend_ip; uint16_t backend_port; - ip_address_and_port_from_lb_key(token, &backend_ip, &backend_port, - &addr_family); - - if (!backend_ip) { + if (!ip_address_and_port_from_lb_key(token, &backend_ip, + &backend_port, + &addr_family)) { continue; } @@ -4682,8 +4679,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) /* For a 'key' of the form "IP:port" or just "IP", sets 'port' and * 'ip_address'. The caller must free() the memory allocated for - * 'ip_address'. */ -static void + * 'ip_address'. + * Returns true if parsing of 'key' was successful, false otherwise. + */ +static bool ip_address_and_port_from_lb_key(const char *key, char **ip_address, uint16_t *port, int *addr_family) { @@ -4692,16 +4691,18 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address, static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s", key); - return; + *ip_address = NULL; + *port = 0; + *addr_family = 0; + return false; } struct ds s = DS_EMPTY_INITIALIZER; ss_format_address_nobracks(&ss, &s); *ip_address = ds_steal_cstr(&s); - *port = ss_get_port(&ss); - *addr_family = ss.ss_family; + return true; } /*
Calls to ip_address_and_port_from_lb_key() could fail parsing the 'key' argument and would return without setting *ip_address. The code in ovn_lb_create() was passing an unitialized 'backend_ip' pointer and using it unconditionally afterwards. With CFLAGS="-O3" gcc reports this issue too: $ ./configure CFLAGS="-O3" --enable-Werror [...] $ make [...] northd/ovn-northd.c: In function ‘ovnnb_db_run.isra.65’: northd/ovn-northd.c:3116:14: error: ‘backend_port’ may be used uninitialized in this function [-Werror=maybe-uninitialized] uint32_t hash = service_port; ^ northd/ovn-northd.c:3207:22: note: ‘backend_port’ was declared here uint16_t backend_port; ^ In file included from northd/ovn-northd.c:27:0: /home/dceara/git-repos/ovs/lib/hash.h:342:5: error: ‘backend_ip’ may be used uninitialized in this function [-Werror=maybe-uninitialized] return hash_bytes(s, strlen(s), basis); ^ northd/ovn-northd.c:3206:19: note: ‘backend_ip’ was declared here char *backend_ip; Fix ip_address_and_port_from_lb_key() and make it return true if parsing was successful. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)