On Thu, Jan 4, 2024 at 12:34 PM Dumitru Ceara <dce...@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 <amu...@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
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
