Skip to content

Commit

Permalink
route-table: Fix potential memory leak.
Browse files Browse the repository at this point in the history
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: 91fc511 ("route-table: Support parsing multipath routes.")
Signed-off-by: Frode Nordahl <[email protected]>
Signed-off-by: 0-day Robot <[email protected]>
  • Loading branch information
fnordahl authored and ovsrobot committed Jan 20, 2025
1 parent bb91a9f commit 263a100
Showing 1 changed file with 12 additions and 7 deletions.
19 changes: 12 additions & 7 deletions lib/route-table.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)
{
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -408,6 +411,8 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
goto error_out;
}

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;
Expand Down

0 comments on commit 263a100

Please sign in to comment.