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.
> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
