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]> --- lib/route-table.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/lib/route-table.c b/lib/route-table.c index b7964522f..6dfb364a4 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,20 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs, goto error_out; } + /* The change->rd->nexthops list is unconditionally populated with + * a single rdnh entry as we start parsing above. Multiple + * branches above may access it or jump to error_out, and having it + * on the list is the only way to ensure proper cleanup. + * + * Getting to this point, we know that the above branches has not + * provided next hop information, because information about + * multiple next hops is encoded in the nested attributes after the + * RTA_MULTIPATH attribute. + * + * Before retrieving those we need to remove the empty rdnh entry + * from the list. */ + 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
