| Message ID | 20240104125424.528637-1-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] treewide: Fix small memory leaks reported by static analysis | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thanks, Ales. Acked-by: Mark Michelson <mmichels@redhat.com> On 1/4/24 07:54, Ales Musil wrote: > Fix a couple of small memory leaks that were reported by coverity > static analysis on ovn rpm package. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Rebase on top of current main. > Address comment from Dumitru: > - Update the pinctrl_handle_empty_lb_backends_opts() to have > only single point of cleanup. > --- > controller-vtep/ovn-controller-vtep.c | 2 + > controller/lflow.c | 1 + > controller/ovn-controller.c | 1 + > controller/pinctrl.c | 60 +++++++++++++++------------ > utilities/ovn-nbctl.c | 1 + > utilities/ovn-trace.c | 1 + > 6 files changed, 40 insertions(+), 26 deletions(-) > > diff --git a/controller-vtep/ovn-controller-vtep.c b/controller-vtep/ovn-controller-vtep.c > index 23b368179..4472e5285 100644 > --- a/controller-vtep/ovn-controller-vtep.c > +++ b/controller-vtep/ovn-controller-vtep.c > @@ -306,10 +306,12 @@ parse_options(int argc, char *argv[]) > > switch (c) { > case 'd': > + free(ovnsb_remote); > ovnsb_remote = xstrdup(optarg); > break; > > case 'D': > + free(vtep_remote); > vtep_remote = xstrdup(optarg); > break; > > diff --git a/controller/lflow.c b/controller/lflow.c > index 85a4943dc..b0cf4253c 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -1017,6 +1017,7 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", > lflow->match, error); > + expr_destroy(e); > free(error); > return NULL; > } > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 760b7788b..7cd146f48 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -6186,6 +6186,7 @@ parse_options(int argc, char *argv[]) > break; > > case 'n': > + free(cli_system_id); > cli_system_id = xstrdup(optarg); > break; > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 5a35d56f6..64a15ed02 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6404,26 +6404,31 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) > char *protocol = NULL; > char *load_balancer = NULL; > > + struct empty_lb_backends_event *event = NULL; > + > while (userdata->size) { > userdata_opt = ofpbuf_try_pull(userdata, sizeof opt_hdr); > if (!userdata_opt) { > - return false; > + goto cleanup; > } > memcpy(&opt_hdr, userdata_opt, sizeof opt_hdr); > > size_t size = ntohs(opt_hdr.size); > char *userdata_opt_data = ofpbuf_try_pull(userdata, size); > if (!userdata_opt_data) { > - return false; > + goto cleanup; > } > switch (ntohs(opt_hdr.opt_code)) { > case EMPTY_LB_VIP: > + free(vip); > vip = xmemdup0(userdata_opt_data, size); > break; > case EMPTY_LB_PROTOCOL: > + free(protocol); > protocol = xmemdup0(userdata_opt_data, size); > break; > case EMPTY_LB_LOAD_BALANCER: > + free(load_balancer); > load_balancer = xmemdup0(userdata_opt_data, size); > break; > default: > @@ -6434,36 +6439,39 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) > if (!vip || !protocol || !load_balancer) { > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > VLOG_WARN_RL(&rl, "missing lb parameters in userdata"); > - free(vip); > - free(protocol); > - free(load_balancer); > - return false; > + goto cleanup; > } > > - struct empty_lb_backends_event *event; > - > event = pinctrl_find_empty_lb_backends_event(vip, protocol, > load_balancer, hash); > - if (!event) { > - if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { > - COVERAGE_INC(pinctrl_drop_controller_event); > - return false; > - } > + if (event) { > + goto cleanup; > + } > > - event = xzalloc(sizeof *event); > - hmap_insert(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS], > - &event->hmap_node, hash); > - event->vip = vip; > - event->protocol = protocol; > - event->load_balancer = load_balancer; > - event->timestamp = time_msec(); > - notify_pinctrl_main(); > - } else { > - free(vip); > - free(protocol); > - free(load_balancer); > + if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { > + COVERAGE_INC(pinctrl_drop_controller_event); > + goto cleanup; > } > - return true; > + > + event = xzalloc(sizeof *event); > + hmap_insert(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS], > + &event->hmap_node, hash); > + event->vip = vip; > + event->protocol = protocol; > + event->load_balancer = load_balancer; > + event->timestamp = time_msec(); > + notify_pinctrl_main(); > + > + vip = NULL; > + protocol = NULL; > + load_balancer = NULL; > + > +cleanup: > + free(vip); > + free(protocol); > + free(load_balancer); > + > + return event != NULL; > } > > static void > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 9522078c1..526369b68 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -4694,6 +4694,7 @@ static void > nexthop = normalize_prefix_str(ctx->argv[3]); > if (!nexthop) { > ctl_error(ctx, "bad nexthop argument: %s", ctx->argv[3]); > + free(prefix); > return; > } > } > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index 0b86eae7b..13ae464ad 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -983,6 +983,7 @@ parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf, > if (error) { > VLOG_WARN("%s: parsing expression failed (%s)", > sblf->match, error); > + expr_destroy(match); > free(error); > return; > }
On 1/4/24 20:24, Mark Michelson wrote: > Thanks, Ales. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks, Ales and Mark! I applied this to main and backported it to all stable branches down to 22.03. Regards, Dumitru
diff --git a/controller-vtep/ovn-controller-vtep.c b/controller-vtep/ovn-controller-vtep.c index 23b368179..4472e5285 100644 --- a/controller-vtep/ovn-controller-vtep.c +++ b/controller-vtep/ovn-controller-vtep.c @@ -306,10 +306,12 @@ parse_options(int argc, char *argv[]) switch (c) { case 'd': + free(ovnsb_remote); ovnsb_remote = xstrdup(optarg); break; case 'D': + free(vtep_remote); vtep_remote = xstrdup(optarg); break; diff --git a/controller/lflow.c b/controller/lflow.c index 85a4943dc..b0cf4253c 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -1017,6 +1017,7 @@ convert_match_to_expr(const struct sbrec_logical_flow *lflow, static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", lflow->match, error); + expr_destroy(e); free(error); return NULL; } diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 760b7788b..7cd146f48 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -6186,6 +6186,7 @@ parse_options(int argc, char *argv[]) break; case 'n': + free(cli_system_id); cli_system_id = xstrdup(optarg); break; diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 5a35d56f6..64a15ed02 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -6404,26 +6404,31 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) char *protocol = NULL; char *load_balancer = NULL; + struct empty_lb_backends_event *event = NULL; + while (userdata->size) { userdata_opt = ofpbuf_try_pull(userdata, sizeof opt_hdr); if (!userdata_opt) { - return false; + goto cleanup; } memcpy(&opt_hdr, userdata_opt, sizeof opt_hdr); size_t size = ntohs(opt_hdr.size); char *userdata_opt_data = ofpbuf_try_pull(userdata, size); if (!userdata_opt_data) { - return false; + goto cleanup; } switch (ntohs(opt_hdr.opt_code)) { case EMPTY_LB_VIP: + free(vip); vip = xmemdup0(userdata_opt_data, size); break; case EMPTY_LB_PROTOCOL: + free(protocol); protocol = xmemdup0(userdata_opt_data, size); break; case EMPTY_LB_LOAD_BALANCER: + free(load_balancer); load_balancer = xmemdup0(userdata_opt_data, size); break; default: @@ -6434,36 +6439,39 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) if (!vip || !protocol || !load_balancer) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); VLOG_WARN_RL(&rl, "missing lb parameters in userdata"); - free(vip); - free(protocol); - free(load_balancer); - return false; + goto cleanup; } - struct empty_lb_backends_event *event; - event = pinctrl_find_empty_lb_backends_event(vip, protocol, load_balancer, hash); - if (!event) { - if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { - COVERAGE_INC(pinctrl_drop_controller_event); - return false; - } + if (event) { + goto cleanup; + } - event = xzalloc(sizeof *event); - hmap_insert(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS], - &event->hmap_node, hash); - event->vip = vip; - event->protocol = protocol; - event->load_balancer = load_balancer; - event->timestamp = time_msec(); - notify_pinctrl_main(); - } else { - free(vip); - free(protocol); - free(load_balancer); + if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { + COVERAGE_INC(pinctrl_drop_controller_event); + goto cleanup; } - return true; + + event = xzalloc(sizeof *event); + hmap_insert(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS], + &event->hmap_node, hash); + event->vip = vip; + event->protocol = protocol; + event->load_balancer = load_balancer; + event->timestamp = time_msec(); + notify_pinctrl_main(); + + vip = NULL; + protocol = NULL; + load_balancer = NULL; + +cleanup: + free(vip); + free(protocol); + free(load_balancer); + + return event != NULL; } static void diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 9522078c1..526369b68 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -4694,6 +4694,7 @@ static void nexthop = normalize_prefix_str(ctx->argv[3]); if (!nexthop) { ctl_error(ctx, "bad nexthop argument: %s", ctx->argv[3]); + free(prefix); return; } } diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index 0b86eae7b..13ae464ad 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -983,6 +983,7 @@ parse_lflow_for_datapath(const struct sbrec_logical_flow *sblf, if (error) { VLOG_WARN("%s: parsing expression failed (%s)", sblf->match, error); + expr_destroy(match); free(error); return; }
Fix a couple of small memory leaks that were reported by coverity static analysis on ovn rpm package. Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Rebase on top of current main. Address comment from Dumitru: - Update the pinctrl_handle_empty_lb_backends_opts() to have only single point of cleanup. --- controller-vtep/ovn-controller-vtep.c | 2 + controller/lflow.c | 1 + controller/ovn-controller.c | 1 + controller/pinctrl.c | 60 +++++++++++++++------------ utilities/ovn-nbctl.c | 1 + utilities/ovn-trace.c | 1 + 6 files changed, 40 insertions(+), 26 deletions(-)