On Saturday, February 22, 2020, Mark Michelson <mmich...@redhat.com> wrote:
> Looks good to me
>
> Acked-by: Mark Michelson <mmich...@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 <dce...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
