From d13d137a1baaea39a24d857de54200dca39410b2 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 26 May 2022 14:03:02 +0300 Subject: [PATCH 1/3] bgpd: Fix memory leak for BGP community alias in CLI Before: ``` root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community' Large Community : 100 40 4000 100 4000 Large Community value : 100 12 2400 100 2400 root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community' Large Community : 200 40 8000 200 8000 Large Community value : 200 12 4800 200 4800 root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community' Large Community : 300 40 12000 300 12000 Large Community value : 300 12 7200 300 7200 root@spine1-debian-11:~/frr# ``` After: ``` root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community' Large Community : 0 40 0 1 56 Large Community display string: 0 8192 0 1 8200 Large Community value : 0 12 0 1 24 root@spine1-debian-11:~/frr# for x in $(seq 1 100); do vtysh -c 'conf' -c 'bgp community alias 123:123:123 testas' > /dev/null; done root@spine1-debian-11:~/frr# vtysh -c 'show memory bgpd | include Large Community' Large Community : 0 40 0 1 56 Large Community display string: 0 8192 0 1 8200 Large Community value : 0 12 0 1 24 root@spine1-debian-11:~/frr# ``` After we call [l]community_str2com(), we should free the memory. Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 9cfec0fe2f..c013160d95 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -1644,8 +1644,21 @@ DEFPY(bgp_community_alias, bgp_community_alias_cmd, struct community_alias ca2; struct community_alias *lookup_community; struct community_alias *lookup_alias; + struct community *comm; + struct lcommunity *lcomm; + uint8_t invalid = 0; - if (!community_str2com(community) && !lcommunity_str2com(community)) { + comm = community_str2com(community); + if (!comm) + invalid++; + community_free(&comm); + + lcomm = lcommunity_str2com(community); + if (!lcomm) + invalid++; + lcommunity_free(&lcomm); + + if (invalid > 1) { vty_out(vty, "Invalid community format\n"); return CMD_WARNING; } From 5b0f36a8f70a6c224771bea4707f6d3c869c0145 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 26 May 2022 15:43:42 +0300 Subject: [PATCH 2/3] bgpd: Distinguish BGP community alias memory separately from community Signed-off-by: Donatas Abraitis --- bgpd/bgp_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_memory.c b/bgpd/bgp_memory.c index 1dc992fb94..b9f1ba3971 100644 --- a/bgpd/bgp_memory.c +++ b/bgpd/bgp_memory.c @@ -65,7 +65,7 @@ DEFINE_MTYPE(BGPD, AS_LIST, "BGP AS list"); DEFINE_MTYPE(BGPD, AS_FILTER, "BGP AS filter"); DEFINE_MTYPE(BGPD, AS_FILTER_STR, "BGP AS filter str"); -DEFINE_MTYPE(BGPD, COMMUNITY_ALIAS, "community"); +DEFINE_MTYPE(BGPD, COMMUNITY_ALIAS, "community alias"); DEFINE_MTYPE(BGPD, COMMUNITY, "community"); DEFINE_MTYPE(BGPD, COMMUNITY_VAL, "community val"); From 8cfa1e78463844518bb95d59fee2da440bbd208e Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Thu, 26 May 2022 20:15:35 +0300 Subject: [PATCH 3/3] bgpd: Simplify BGP community alias handling Also, warn in CLI an operator if we are trying to overwrite an existing community alias with an existing alias. Signed-off-by: Donatas Abraitis --- bgpd/bgp_vty.c | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index c013160d95..0a4083e5f6 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -1640,8 +1640,7 @@ DEFPY(bgp_community_alias, bgp_community_alias_cmd, "Community (AA:BB or AA:BB:CC)\n" "Alias name\n") { - struct community_alias ca1; - struct community_alias ca2; + struct community_alias ca = {}; struct community_alias *lookup_community; struct community_alias *lookup_alias; struct community *comm; @@ -1663,39 +1662,50 @@ DEFPY(bgp_community_alias, bgp_community_alias_cmd, return CMD_WARNING; } - memset(&ca1, 0, sizeof(ca1)); - memset(&ca2, 0, sizeof(ca2)); - strlcpy(ca1.community, community, sizeof(ca1.community)); - strlcpy(ca1.alias, alias_name, sizeof(ca1.alias)); + strlcpy(ca.community, community, sizeof(ca.community)); + strlcpy(ca.alias, alias_name, sizeof(ca.alias)); - lookup_community = bgp_ca_community_lookup(&ca1); - lookup_alias = bgp_ca_alias_lookup(&ca1); + lookup_community = bgp_ca_community_lookup(&ca); + lookup_alias = bgp_ca_alias_lookup(&ca); if (no) { - bgp_ca_alias_delete(&ca1); - bgp_ca_community_delete(&ca1); + bgp_ca_alias_delete(&ca); + bgp_ca_community_delete(&ca); } else { if (lookup_alias) { /* Lookup if community hash table has an item * with the same alias name. */ - strlcpy(ca2.community, lookup_alias->community, - sizeof(ca2.community)); - if (bgp_ca_community_lookup(&ca2)) { + strlcpy(ca.community, lookup_alias->community, + sizeof(ca.community)); + if (bgp_ca_community_lookup(&ca)) { vty_out(vty, "community (%s) already has this alias (%s)\n", lookup_alias->community, lookup_alias->alias); return CMD_WARNING; } - bgp_ca_alias_delete(&ca1); + bgp_ca_alias_delete(&ca); } - if (lookup_community) - bgp_ca_community_delete(&ca1); + if (lookup_community) { + /* Lookup if alias hash table has an item + * with the same community. + */ + strlcpy(ca.alias, lookup_community->alias, + sizeof(ca.alias)); + if (bgp_ca_alias_lookup(&ca)) { + vty_out(vty, + "alias (%s) already has this community (%s)\n", + lookup_community->alias, + lookup_community->community); + return CMD_WARNING; + } + bgp_ca_community_delete(&ca); + } - bgp_ca_alias_insert(&ca1); - bgp_ca_community_insert(&ca1); + bgp_ca_alias_insert(&ca); + bgp_ca_community_insert(&ca); } return CMD_SUCCESS;