On Saturday, February 22, 2020, Mark Michelson <[email protected]> wrote:
> Looks good to me > > Acked-by: Mark Michelson <[email protected]> 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 <[email protected]> >> --- >> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
