From 7ee3fb2e032244fd2366c00d58d5426672a50cb2 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 21 Nov 2023 10:38:12 +0200 Subject: [PATCH 1/2] tests: Set community for conditionally advertised routes Just to make sure we don't crash bgpd with double-free if an existing route already exists. Signed-off-by: Donatas Abraitis (cherry picked from commit 8eba97d3954d466b171e283b289710ad7544df65) --- .../r2/frr.conf | 1 + ..._conditional_advertisement_static_route.py | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/tests/topotests/bgp_conditional_advertisement_static_route/r2/frr.conf b/tests/topotests/bgp_conditional_advertisement_static_route/r2/frr.conf index 9dc4099341d9..3ced9340ca15 100644 --- a/tests/topotests/bgp_conditional_advertisement_static_route/r2/frr.conf +++ b/tests/topotests/bgp_conditional_advertisement_static_route/r2/frr.conf @@ -36,4 +36,5 @@ route-map exist-map permit 10 ! route-map advertise-map permit 10 match ip address prefix-list advertise + set community 65000:1 ! diff --git a/tests/topotests/bgp_conditional_advertisement_static_route/test_bgp_conditional_advertisement_static_route.py b/tests/topotests/bgp_conditional_advertisement_static_route/test_bgp_conditional_advertisement_static_route.py index 9d61bbd643fe..4180bfcdf6a2 100644 --- a/tests/topotests/bgp_conditional_advertisement_static_route/test_bgp_conditional_advertisement_static_route.py +++ b/tests/topotests/bgp_conditional_advertisement_static_route/test_bgp_conditional_advertisement_static_route.py @@ -54,6 +54,7 @@ def test_bgp_conditional_advertisements_static_route(): if tgen.routers_have_failure(): pytest.skip(tgen.errors) + r1 = tgen.gears["r1"] r2 = tgen.gears["r2"] def _bgp_converge(): @@ -112,6 +113,25 @@ def _bgp_check_advertised_after_update(): _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) assert result is None, "10.10.10.2/32 is not advertised after prefix-list update" + def _bgp_check_received_routes(): + output = json.loads(r1.vtysh_cmd("show bgp ipv4 unicast 10.10.10.1/32 json")) + expected = { + "paths": [ + { + "community": { + "string": "65000:1", + } + } + ] + } + return topotest.json_cmp(output, expected) + + test_func = functools.partial( + _bgp_check_received_routes, + ) + _, result = topotest.run_and_expect(test_func, None, count=30, wait=1) + assert result is None, "10.10.10.1/32 does not have 65000:1 community attached" + if __name__ == "__main__": args = ["-s"] + sys.argv[1:] From b9a7d582cceb5613fbfc34084c64f8ce643bff81 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Tue, 21 Nov 2023 10:40:58 +0200 Subject: [PATCH 2/2] bgpd: Flush attrs only if we don't have to announce a conditional route To avoid USE: ``` ==587645==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000074050 at pc 0x55b34337d96c bp 0x7ffda59bb4c0 sp 0x7ffda59bb4b0 READ of size 8 at 0x604000074050 thread T0 0 0x55b34337d96b in bgp_attr_flush bgpd/bgp_attr.c:1289 1 0x55b34368ef85 in bgp_conditional_adv_routes bgpd/bgp_conditional_adv.c:111 2 0x55b34368ff58 in bgp_conditional_adv_timer bgpd/bgp_conditional_adv.c:301 3 0x7f7d41cdf81c in event_call lib/event.c:1980 4 0x7f7d41c1da37 in frr_run lib/libfrr.c:1214 5 0x55b343371e22 in main bgpd/bgp_main.c:510 6 0x7f7d41517082 in __libc_start_main ../csu/libc-start.c:308 7 0x55b3433769fd in _start (/usr/lib/frr/bgpd+0x2e29fd) 0x604000074050 is located 0 bytes inside of 40-byte region [0x604000074050,0x604000074078) freed by thread T0 here: #0 0x7f7d4207540f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:122 1 0x55b343396afd in community_free bgpd/bgp_community.c:41 2 0x55b343396afd in community_free bgpd/bgp_community.c:28 3 0x55b343397373 in community_intern bgpd/bgp_community.c:458 4 0x55b34337bed4 in bgp_attr_intern bgpd/bgp_attr.c:967 5 0x55b34368165b in bgp_advertise_attr_intern bgpd/bgp_advertise.c:106 6 0x55b3435277d7 in bgp_adj_out_set_subgroup bgpd/bgp_updgrp_adv.c:587 7 0x55b34368f36b in bgp_conditional_adv_routes bgpd/bgp_conditional_adv.c:125 8 0x55b34368ff58 in bgp_conditional_adv_timer bgpd/bgp_conditional_adv.c:301 9 0x7f7d41cdf81c in event_call lib/event.c:1980 10 0x7f7d41c1da37 in frr_run lib/libfrr.c:1214 11 0x55b343371e22 in main bgpd/bgp_main.c:510 12 0x7f7d41517082 in __libc_start_main ../csu/libc-start.c:308 previously allocated by thread T0 here: #0 0x7f7d42075a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 1 0x7f7d41c3c28e in qcalloc lib/memory.c:105 2 0x55b3433976e8 in community_dup bgpd/bgp_community.c:514 3 0x55b34350273a in route_set_community bgpd/bgp_routemap.c:2589 4 0x7f7d41c96c06 in route_map_apply_ext lib/routemap.c:2690 5 0x55b34368f2d8 in bgp_conditional_adv_routes bgpd/bgp_conditional_adv.c:107 6 0x55b34368ff58 in bgp_conditional_adv_timer bgpd/bgp_conditional_adv.c:301 7 0x7f7d41cdf81c in event_call lib/event.c:1980 8 0x7f7d41c1da37 in frr_run lib/libfrr.c:1214 9 0x55b343371e22 in main bgpd/bgp_main.c:510 10 0x7f7d41517082 in __libc_start_main ../csu/libc-start.c:308 ``` And also a crash: ``` (gdb) bt 0 raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 1 0x00007ff3b7048ce0 in core_handler (signo=6, siginfo=0x7ffc8cf724b0, context=) at lib/sigevent.c:246 2 3 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 4 0x00007ff3b6bb8859 in __GI_abort () at abort.c:79 5 0x00007ff3b6c2326e in __libc_message (action=action@entry=do_abort, fmt=fmt@entry=0x7ff3b6d4d298 "%s\n") at ../sysdeps/posix/libc_fatal.c:155 6 0x00007ff3b6c2b2fc in malloc_printerr ( str=str@entry=0x7ff3b6d4f628 "double free or corruption (fasttop)") at malloc.c:5347 7 0x00007ff3b6c2cc65 in _int_free (av=0x7ff3b6d82b80 , p=0x555c8fa70a10, have_lock=0) at malloc.c:4266 8 0x0000555c8da94bd3 in community_free (com=0x7ffc8cf72e70) at bgpd/bgp_community.c:41 9 community_free (com=com@entry=0x7ffc8cf72e70) at bgpd/bgp_community.c:28 10 0x0000555c8da8afc1 in bgp_attr_flush (attr=attr@entry=0x7ffc8cf73040) at bgpd/bgp_attr.c:1290 11 0x0000555c8dbc0760 in bgp_conditional_adv_routes (peer=peer@entry=0x555c8fa627c0, afi=afi@entry=AFI_IP, safi=SAFI_UNICAST, table=table@entry=0x555c8fa510b0, rmap=0x555c8fa71cb0, update_type=UPDATE_TYPE_ADVERTISE) at bgpd/bgp_conditional_adv.c:111 12 0x0000555c8dbc0b75 in bgp_conditional_adv_timer (t=) at bgpd/bgp_conditional_adv.c:301 13 0x00007ff3b705b84c in event_call (thread=thread@entry=0x7ffc8cf73440) at lib/event.c:1980 14 0x00007ff3b700bf98 in frr_run (master=0x555c8f27c090) at lib/libfrr.c:1214 15 0x0000555c8da85f05 in main (argc=, argv=0x7ffc8cf736a8) at bgpd/bgp_main.c:510 ``` Signed-off-by: Donatas Abraitis (cherry picked from commit d410587bab67d1d492f724c6111ff7238a78e7b2) --- bgpd/bgp_conditional_adv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_conditional_adv.c b/bgpd/bgp_conditional_adv.c index 6685a4f44787..ff805cb70a97 100644 --- a/bgpd/bgp_conditional_adv.c +++ b/bgpd/bgp_conditional_adv.c @@ -141,8 +141,9 @@ static void bgp_conditional_adv_routes(struct peer *peer, afi_t afi, bgp_addpath_id_for_peer( peer, afi, safi, &pi->tx_addpath)); + + bgp_attr_flush(&advmap_attr); } - bgp_attr_flush(&advmap_attr); } } UNSET_FLAG(subgrp->sflags, SUBGRP_STATUS_TABLE_REPARSING);