From eb8281aeb840b004f5a0c0218f2723afc26c9557 Mon Sep 17 00:00:00 2001 From: Louis Scalbert Date: Fri, 11 Apr 2025 17:39:28 +0200 Subject: [PATCH] bgpd: fix bgp_pbr_or_filter memory leaks Note that bgp_pbr_policyroute_add_from_zebra() and bgp_pbr_policyroute_remove_from_zebra() are only called from bgp_pbr_handle_entry(). > ==966967==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7fd447ab4a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 > #1 0x7fd44746f8dd in qcalloc lib/memory.c:105 > #2 0x7fd44744401a in list_new lib/linklist.c:49 > #3 0x560f8c094490 in bgp_pbr_handle_entry bgpd/bgp_pbr.c:2818 > #4 0x560f8c095993 in bgp_pbr_update_entry bgpd/bgp_pbr.c:2941 > #5 0x560f8c2164f3 in bgp_zebra_announce bgpd/bgp_zebra.c:1618 > #6 0x560f8c0bd668 in bgp_process_main_one bgpd/bgp_route.c:3691 > #7 0x560f8c0be7fe in process_subq_other_route bgpd/bgp_route.c:3856 > #8 0x560f8c0bf280 in process_subq bgpd/bgp_route.c:3955 > #9 0x560f8c0bf320 in meta_queue_process bgpd/bgp_route.c:3980 > #10 0x7fd44759fdfc in work_queue_run lib/workqueue.c:282 > #11 0x7fd4475779b2 in event_call lib/event.c:2011 > #12 0x7fd447442ff1 in frr_run lib/libfrr.c:1216 > #13 0x560f8bef0a15 in main bgpd/bgp_main.c:545 > #14 0x7fd446e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 > > Direct leak of 40 byte(s) in 1 object(s) allocated from: > #0 0x7fd447ab4a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 > #1 0x7fd44746f8dd in qcalloc lib/memory.c:105 > #2 0x7fd44744401a in list_new lib/linklist.c:49 > #3 0x560f8c09439d in bgp_pbr_handle_entry bgpd/bgp_pbr.c:2812 > #4 0x560f8c095993 in bgp_pbr_update_entry bgpd/bgp_pbr.c:2941 > #5 0x560f8c2164f3 in bgp_zebra_announce bgpd/bgp_zebra.c:1618 > #6 0x560f8c0bd668 in bgp_process_main_one bgpd/bgp_route.c:3691 > #7 0x560f8c0be7fe in process_subq_other_route bgpd/bgp_route.c:3856 > #8 0x560f8c0bf280 in process_subq bgpd/bgp_route.c:3955 > #9 0x560f8c0bf320 in meta_queue_process bgpd/bgp_route.c:3980 > #10 0x7fd44759fdfc in work_queue_run lib/workqueue.c:282 > #11 0x7fd4475779b2 in event_call lib/event.c:2011 > #12 0x7fd447442ff1 in frr_run lib/libfrr.c:1216 > #13 0x560f8bef0a15 in main bgpd/bgp_main.c:545 > #14 0x7fd446e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 > > Direct leak of 4 byte(s) in 1 object(s) allocated from: > #0 0x7fd447ab4a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 > #1 0x7fd44746f8dd in qcalloc lib/memory.c:105 > #2 0x560f8c080cec in bgp_pbr_extract_enumerate_unary bgpd/bgp_pbr.c:362 > #3 0x560f8c080f7e in bgp_pbr_extract_enumerate bgpd/bgp_pbr.c:400 > #4 0x560f8c094530 in bgp_pbr_handle_entry bgpd/bgp_pbr.c:2819 > #5 0x560f8c095993 in bgp_pbr_update_entry bgpd/bgp_pbr.c:2941 > #6 0x560f8c2164f3 in bgp_zebra_announce bgpd/bgp_zebra.c:1618 > #7 0x560f8c0bd668 in bgp_process_main_one bgpd/bgp_route.c:3691 > #8 0x560f8c0be7fe in process_subq_other_route bgpd/bgp_route.c:3856 > #9 0x560f8c0bf280 in process_subq bgpd/bgp_route.c:3955 > #10 0x560f8c0bf320 in meta_queue_process bgpd/bgp_route.c:3980 > #11 0x7fd44759fdfc in work_queue_run lib/workqueue.c:282 > #12 0x7fd4475779b2 in event_call lib/event.c:2011 > #13 0x7fd447442ff1 in frr_run lib/libfrr.c:1216 > #14 0x560f8bef0a15 in main bgpd/bgp_main.c:545 > #15 0x7fd446e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 > > Direct leak of 4 byte(s) in 1 object(s) allocated from: > #0 0x7fd447ab4a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 > #1 0x7fd44746f8dd in qcalloc lib/memory.c:105 > #2 0x560f8c080cec in bgp_pbr_extract_enumerate_unary bgpd/bgp_pbr.c:362 > #3 0x560f8c080f7e in bgp_pbr_extract_enumerate bgpd/bgp_pbr.c:400 > #4 0x560f8c09443d in bgp_pbr_handle_entry bgpd/bgp_pbr.c:2813 > #5 0x560f8c095993 in bgp_pbr_update_entry bgpd/bgp_pbr.c:2941 > #6 0x560f8c2164f3 in bgp_zebra_announce bgpd/bgp_zebra.c:1618 > #7 0x560f8c0bd668 in bgp_process_main_one bgpd/bgp_route.c:3691 > #8 0x560f8c0be7fe in process_subq_other_route bgpd/bgp_route.c:3856 > #9 0x560f8c0bf280 in process_subq bgpd/bgp_route.c:3955 > #10 0x560f8c0bf320 in meta_queue_process bgpd/bgp_route.c:3980 > #11 0x7fd44759fdfc in work_queue_run lib/workqueue.c:282 > #12 0x7fd4475779b2 in event_call lib/event.c:2011 > #13 0x7fd447442ff1 in frr_run lib/libfrr.c:1216 > #14 0x560f8bef0a15 in main bgpd/bgp_main.c:545 > #15 0x7fd446e29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 Signed-off-by: Louis Scalbert --- bgpd/bgp_pbr.c | 55 +++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 25 deletions(-) diff --git a/bgpd/bgp_pbr.c b/bgpd/bgp_pbr.c index 07de1e426e..2b13715da3 100644 --- a/bgpd/bgp_pbr.c +++ b/bgpd/bgp_pbr.c @@ -279,6 +279,13 @@ static void bgp_pbr_policyroute_add_to_zebra_unit(struct bgp *bgp, static void bgp_pbr_dump_entry(struct bgp_pbr_filter *bpf, bool add); +static void bgp_pbr_val_mask_free(void *arg) +{ + struct bgp_pbr_val_mask *pbr_val_mask = arg; + + XFREE(MTYPE_PBR_VALMASK, pbr_val_mask); +} + static bool bgp_pbr_extract_enumerate_unary_opposite( uint8_t unary_operator, struct bgp_pbr_val_mask *and_valmask, @@ -2130,17 +2137,6 @@ static void bgp_pbr_policyroute_remove_from_zebra( bgp, path, bpf, bpof, FLOWSPEC_ICMP_TYPE); else bgp_pbr_policyroute_remove_from_zebra_unit(bgp, path, bpf); - /* flush bpof */ - if (bpof->tcpflags) - list_delete_all_node(bpof->tcpflags); - if (bpof->dscp) - list_delete_all_node(bpof->dscp); - if (bpof->flowlabel) - list_delete_all_node(bpof->flowlabel); - if (bpof->pkt_len) - list_delete_all_node(bpof->pkt_len); - if (bpof->fragment) - list_delete_all_node(bpof->fragment); } static void bgp_pbr_dump_entry(struct bgp_pbr_filter *bpf, bool add) @@ -2625,19 +2621,6 @@ static void bgp_pbr_policyroute_add_to_zebra(struct bgp *bgp, bgp, path, bpf, bpof, nh, rate, FLOWSPEC_ICMP_TYPE); else bgp_pbr_policyroute_add_to_zebra_unit(bgp, path, bpf, nh, rate); - /* flush bpof */ - if (bpof->tcpflags) - list_delete_all_node(bpof->tcpflags); - if (bpof->dscp) - list_delete_all_node(bpof->dscp); - if (bpof->pkt_len) - list_delete_all_node(bpof->pkt_len); - if (bpof->fragment) - list_delete_all_node(bpof->fragment); - if (bpof->icmp_type) - list_delete_all_node(bpof->icmp_type); - if (bpof->icmp_code) - list_delete_all_node(bpof->icmp_code); } static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, @@ -2703,6 +2686,7 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, srcp = ⦥ else { bpof.icmp_type = list_new(); + bpof.icmp_type->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->icmp_type, api->match_icmp_type_num, OPERATOR_UNARY_OR, @@ -2718,6 +2702,7 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, dstp = &range_icmp_code; else { bpof.icmp_code = list_new(); + bpof.icmp_code->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->icmp_code, api->match_icmp_code_num, OPERATOR_UNARY_OR, @@ -2738,6 +2723,7 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, FLOWSPEC_TCP_FLAGS); } else if (kind_enum == OPERATOR_UNARY_OR) { bpof.tcpflags = list_new(); + bpof.tcpflags->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->tcpflags, api->match_tcpflags_num, OPERATOR_UNARY_OR, @@ -2755,6 +2741,7 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, bpf.pkt_len = &pkt_len; else { bpof.pkt_len = list_new(); + bpof.pkt_len->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->packet_length, api->match_packet_length_num, OPERATOR_UNARY_OR, @@ -2764,12 +2751,14 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, } if (api->match_dscp_num >= 1) { bpof.dscp = list_new(); + bpof.dscp->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->dscp, api->match_dscp_num, OPERATOR_UNARY_OR, bpof.dscp, FLOWSPEC_DSCP); } if (api->match_fragment_num) { bpof.fragment = list_new(); + bpof.fragment->del = bgp_pbr_val_mask_free; bgp_pbr_extract_enumerate(api->fragment, api->match_fragment_num, OPERATOR_UNARY_OR, @@ -2785,7 +2774,7 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, bpf.family = afi2family(api->afi); if (!add) { bgp_pbr_policyroute_remove_from_zebra(bgp, path, &bpf, &bpof); - return; + goto flush_bpof; } /* no action for add = true */ for (i = 0; i < api->action_num; i++) { @@ -2863,6 +2852,22 @@ static void bgp_pbr_handle_entry(struct bgp *bgp, struct bgp_path_info *path, if (continue_loop == 0) break; } + +flush_bpof: + if (bpof.tcpflags) + list_delete(&bpof.tcpflags); + if (bpof.dscp) + list_delete(&bpof.dscp); + if (bpof.flowlabel) + list_delete(&bpof.flowlabel); + if (bpof.pkt_len) + list_delete(&bpof.pkt_len); + if (bpof.fragment) + list_delete(&bpof.fragment); + if (bpof.icmp_type) + list_delete(&bpof.icmp_type); + if (bpof.icmp_code) + list_delete(&bpof.icmp_code); } void bgp_pbr_update_entry(struct bgp *bgp, const struct prefix *p, -- 2.39.5