bgpd: Avoid use-after-free when flushing bgp_distance_table #17748
Conversation
Fixes: 663281c ("BGP: Clean address-family config on daemon restart") Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
| @@ -15794,7 +15794,7 @@ void bgp_address_family_distance_delete(void) | |||
| bgp_distance_free(bdistance); | |||
|
|
|||
| bgp_dest_set_bgp_distance_info(dest, NULL); | |||
There was a problem hiding this comment.
hmm - I'm not quite following.
above, the one-dest code calls bgp_dest_set_bgp_path_info(dest, NULL) instead of set_bgp_distance_info() - is one of these wrong?
and why does adding the dest = help ? if that unlock_node frees dest, it'll be NULL, and the iteration will stop - and that seems ... wrong: shouldn't the iteration continue through all of the dests?
I thought in some other places where a loop could free an object, we find the "next" at the top of the clause, instead of using the object in the for clause directly?
There was a problem hiding this comment.
Maybe you are right, I'm just trying to fix use-after-free by using the same pattern it's used somewhere else in the code too. You can see bgp_static_delete() as an example.
There was a problem hiding this comment.
shouldn't this just be a dest_next = bgp_route_next() before the unlock? and in the for loop we set dest = dest_next;?
There was a problem hiding this comment.
I think that's one question, yes.
The other (that I had) was about the inconsistency in the clean-up api to use: "set...path_info()" above, vs "set...distance_info()" in this new code.
No description provided.