On 20 Jan 2025, at 9:14, Frode Nordahl wrote:
> As highlighted by static analysis [0], a potential memory leak
> was introduced in the commit referenced in the fixes tag.
>
> The issue was introduced by what I can describe as premature
> optimization, and while very unlikely to hit, let's make the code
> correct.
>
> The logic for cleanup assumes rdnh will always be added to the
> list of nexthops, and then I apparently chose to skip that when
> processing the outer message with a RTA_MULTIPATH attribute,
> presumably because its nexthop attributes will be added when
> processing nested attributes, making the list addition
> redundant.
>
> Skipping the list addition was technically safe, because at
> this point rdnh would be pointing at primary_next_hop__ on the
> stack.
>
> Separate out the nexthop cleanup code in private helper for
> internal use, while this is the only action for the public
> route_data_destroy() today, it might grow other powers in the
> future.
>
> Always add rdnh to list of nexthops and remove it when processing
> RTA_MULTIPATH nested attributes.
>
> 0: https://mail.openvswitch.org/pipermail/ovs-dev/2025-January/419818.html
> Fixes: 91fc51106cfe ("route-table: Support parsing multipath routes.")
> Signed-off-by: Frode Nordahl <[email protected]>
Hi Frode,
Thanks for looking into this. And I can confirm this patch will get rid of the
Coverity issue.
I have one comment below about adding a comment ;)
Cheers,
Eelco
> ---
> lib/route-table.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/lib/route-table.c b/lib/route-table.c
> index b7964522f..bec628405 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -71,8 +71,8 @@ static void route_map_clear(void);
> static void name_table_init(void);
> static void name_table_change(const struct rtnetlink_change *, void *);
>
> -void
> -route_data_destroy(struct route_data *rd)
> +static void
> +route_data_destroy_nexthops__(struct route_data *rd)
> {
> struct route_data_nexthop *rdnh;
>
> @@ -83,6 +83,12 @@ route_data_destroy(struct route_data *rd)
> }
> }
>
> +void
> +route_data_destroy(struct route_data *rd)
> +{
> + route_data_destroy_nexthops__(rd);
> +}
> +
> uint64_t
> route_table_get_change_seq(void)
> {
> @@ -275,12 +281,9 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
>
> ovs_list_init(&change->rd.nexthops);
> rdnh = rtnh ? xzalloc(sizeof *rdnh) : &change->rd.primary_next_hop__;
> + ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
>
> - if (!attrs[RTA_MULTIPATH]) {
> - rdnh->family = rtm->rtm_family;
> - ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);
> - }
> -
> + rdnh->family = rtm->rtm_family;
> change->relevant = true;
>
> if (rtm->rtm_scope == RT_SCOPE_NOWHERE) {
> @@ -408,6 +411,8 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> goto error_out;
> }
>
Can we add a comment here explaining why we removed the next hop(s) we added
earlier?
> + route_data_destroy_nexthops__(&change->rd);
> +
> NL_NESTED_FOR_EACH (nla, left, attrs[RTA_MULTIPATH]) {
> struct route_table_msg mp_change;
> struct rtnexthop *mp_rtnh;
> --
> 2.43.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
