| Message ID | 20240103123223.435262-1-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] 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 |
On 1/3/24 13:32, 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> > --- Thanks for the fixes, Ales! A few comments below. > controller-vtep/ovn-controller-vtep.c | 2 ++ > controller/lflow.c | 1 + > controller/ovn-controller.c | 1 + > controller/pinctrl.c | 20 +++++++++++++------- > utilities/ovn-nbctl.c | 1 + > utilities/ovn-trace.c | 1 + > 6 files changed, 19 insertions(+), 7 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); Nit: e is always NULL here but OK, can't hurt. > 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..169c02c32 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6407,23 +6407,26 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) > while (userdata->size) { > userdata_opt = ofpbuf_try_pull(userdata, sizeof opt_hdr); > if (!userdata_opt) { > - return false; > + goto err; > } > 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 err; > } > 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,10 +6437,7 @@ 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 err; > } > > struct empty_lb_backends_event *event; > @@ -6447,7 +6447,7 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) > if (!event) { > if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { > COVERAGE_INC(pinctrl_drop_controller_event); > - return false; > + goto err; > } > > event = xzalloc(sizeof *event); > @@ -6464,6 +6464,12 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) > free(load_balancer); > } > return true; > + > +err: > + free(vip); > + free(protocol); > + free(load_balancer); > + return false; For context, just above we have: 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); goto err; } 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); } return true; err: free(vip); free(protocol); free(load_balancer); return false; } Can we unify the freeing? We could just set vip, protocol and load_balancer to NULL after they're stashed into an 'event'. Then we can always just call free at the end of the function. We'd also need to change the label to something like 'cleanup' and we'd need to set a return code variable. What do you think of that? > } > > 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); Same nit about match always being NULL at this point but OK, can't hurt. > free(error); > return; > } Thanks, Dumitru
On Thu, Jan 4, 2024 at 12:34 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 1/3/24 13:32, 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> > > --- > > Thanks for the fixes, Ales! A few comments below. > Hi Dumitru, thank you for the review. > > > controller-vtep/ovn-controller-vtep.c | 2 ++ > > controller/lflow.c | 1 + > > controller/ovn-controller.c | 1 + > > controller/pinctrl.c | 20 +++++++++++++------- > > utilities/ovn-nbctl.c | 1 + > > utilities/ovn-trace.c | 1 + > > 6 files changed, 19 insertions(+), 7 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); > > Nit: e is always NULL here but OK, can't hurt. > > > 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..169c02c32 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -6407,23 +6407,26 @@ pinctrl_handle_empty_lb_backends_opts(struct > ofpbuf *userdata) > > while (userdata->size) { > > userdata_opt = ofpbuf_try_pull(userdata, sizeof opt_hdr); > > if (!userdata_opt) { > > - return false; > > + goto err; > > } > > 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 err; > > } > > 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,10 +6437,7 @@ 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 err; > > } > > > > struct empty_lb_backends_event *event; > > @@ -6447,7 +6447,7 @@ pinctrl_handle_empty_lb_backends_opts(struct > ofpbuf *userdata) > > if (!event) { > > if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= > 1000) { > > COVERAGE_INC(pinctrl_drop_controller_event); > > - return false; > > + goto err; > > } > > > > event = xzalloc(sizeof *event); > > @@ -6464,6 +6464,12 @@ pinctrl_handle_empty_lb_backends_opts(struct > ofpbuf *userdata) > > free(load_balancer); > > } > > return true; > > + > > +err: > > + free(vip); > > + free(protocol); > > + free(load_balancer); > > + return false; > > For context, just above we have: > > 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); > > > > goto err; > > > > } > > > > > 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); > > > > } > > > > return true; > > > > > err: > > > > free(vip); > > > > free(protocol); > > > > free(load_balancer); > > > > return false; > > > > } > > Can we unify the freeing? We could just set vip, protocol and > load_balancer to NULL after they're stashed into an 'event'. > > Then we can always just call free at the end of the function. > > We'd also need to change the label to something like 'cleanup' > and we'd need to set a return code variable. What do you think > of that? > I tried that actually, and it becomes a little clunky when we have to handle the return value in addition. I'll give it another try, maybe I will come up with something better. > > > } > > > > 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); > > Same nit about match always being NULL at this point but OK, > can't hurt. > > > free(error); > > return; > > } > > Thanks, > Dumitru > > Thanks, Ales
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..169c02c32 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -6407,23 +6407,26 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) while (userdata->size) { userdata_opt = ofpbuf_try_pull(userdata, sizeof opt_hdr); if (!userdata_opt) { - return false; + goto err; } 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 err; } 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,10 +6437,7 @@ 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 err; } struct empty_lb_backends_event *event; @@ -6447,7 +6447,7 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) if (!event) { if (hmap_count(&event_table[OVN_EVENT_EMPTY_LB_BACKENDS]) >= 1000) { COVERAGE_INC(pinctrl_drop_controller_event); - return false; + goto err; } event = xzalloc(sizeof *event); @@ -6464,6 +6464,12 @@ pinctrl_handle_empty_lb_backends_opts(struct ofpbuf *userdata) free(load_balancer); } return true; + +err: + free(vip); + free(protocol); + free(load_balancer); + return false; } 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> --- controller-vtep/ovn-controller-vtep.c | 2 ++ controller/lflow.c | 1 + controller/ovn-controller.c | 1 + controller/pinctrl.c | 20 +++++++++++++------- utilities/ovn-nbctl.c | 1 + utilities/ovn-trace.c | 1 + 6 files changed, 19 insertions(+), 7 deletions(-)