lib: Free Temporary Memory for AF_FLOWSPEC Prefix

[Keelan10]

This commit addresses a leak where temporary memory allocated earlier by the prefix_copy function for AF_FLOWSPEC prefixes was not being freed. To ensure proper memory management, we now release this temporary memory using XFREE.

The ASan leak log for reference:

***********************************************************************************
Address Sanitizer Error detected in bgp_flowspec.test_bgp_flowspec_topo/r1.asan.bgpd.11539

=================================================================
==11539==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 56 byte(s) in 2 object(s) allocated from:
    #0 0x7feaa956ad28 in __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
    #1 0x7feaa8f670da in qcalloc lib/memory.c:105
    #2 0x7feaa8fac1d4 in prefix_copy lib/prefix.c:346
    #3 0x7feaa8ff43e8 in route_node_get lib/table.c:274
    #4 0x56247cc798c0 in bgp_node_get bgpd/bgp_table.h:236
    #5 0x56247cc798c0 in bgp_afi_node_get bgpd/bgp_route.c:145
    #6 0x56247cc92622 in bgp_update bgpd/bgp_route.c:4188
    #7 0x56247ce55b21 in bgp_nlri_parse_flowspec bgpd/bgp_flowspec.c:193
    #8 0x56247cc4cdd8 in bgp_nlri_parse bgpd/bgp_packet.c:350
    #9 0x56247cc4f37c in bgp_update_receive bgpd/bgp_packet.c:2153
    #10 0x56247cc591e2 in bgp_process_packet bgpd/bgp_packet.c:3214
    #11 0x7feaa9005b99 in event_call lib/event.c:1979
    #12 0x7feaa8f4a379 in frr_run lib/libfrr.c:1213
    #13 0x56247cb51b21 in main bgpd/bgp_main.c:510
    #14 0x7feaa7f8dc86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: 56 byte(s) leaked in 2 allocation(s).
***********************************************************************************

[donaldsharp]

the flowspec memory leak is a fundamental problem in how prefixes are handled. This will not actually fix the problem, but instead create fun down the road

[donaldsharp]

Yeah no. This is just a crash waiting to happen. This is not how to fix this problem

[Keelan10]

It appears that the CI tests passed successfully.

To clarify, the reason for suggesting to check for the AF_FLOWSPEC condition when freeing the temporary memory allocated by prefix_copy is because the same condition is used for memory allocation. Furthermore, it's worth noting that the value stored in this temporary memory is subsequently copied to node->p within the route_node_set function, which means the temporary memory becomes unnecessary at that point.

Did I get that correctly? Thanks for your input!

[pguibert6WIND]

@Keelan10, the good solution has to deal with a clean architecture. and what you do here it to mess flowspec with tables, which has nothing to do together.
I tried to do something without success . please see #7485 (comment)

[Keelan10]

@pguibert6WIND You make a valid point. Thanks for the feedback

[NetDEF-CI]

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-14048/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for table.c | 4 issues
===============================================
< ERROR: space required before the open brace '{'
< #327: FILE: /tmp/f1-2279504/table.c:327:
< WARNING: Missing a blank line after declarations
< #329: FILE: /tmp/f1-2279504/table.c:329:

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[louis-6wind]

@Keelan10 thank you for this pull request.

prefix_flowspec_ptr_free() looks now good to me.

However, you must use it in other places in the code to solve all the possible memory leaks.

You can search for prefix_linkstate_ptr_free in the code

[louis-6wind]

You need that:

diff --git a/bgpd/bgp_table.c b/bgpd/bgp_table.c
index e01bf39113..a64477047c 100644
--- a/bgpd/bgp_table.c
+++ b/bgpd/bgp_table.c
@@ -119,6 +119,8 @@ static void bgp_node_destroy(route_table_delegate_t *delegate,
 
 	if (family2afi(node->p.family) == AFI_LINKSTATE)
 		prefix_linkstate_ptr_free(&node->p);
+	else if (node->p.family == AF_FLOWSPEC)
+		prefix_flowspec_ptr_free(&node->p);
 
 	XFREE(MTYPE_ROUTE_NODE, node);
 }
diff --git a/lib/prefix.c b/lib/prefix.c
index d7a800d1ca..092581aeb8 100644
--- a/lib/prefix.c
+++ b/lib/prefix.c
@@ -1466,7 +1466,6 @@ unsigned prefix_hash_key(const void *pp)
 {
 	struct prefix copy;
 	uint32_t len;
-	void *temp;
 
 	/* make sure *all* unused bits are zero, particularly including
 	 * alignment /
@@ -1478,9 +1477,7 @@ unsigned prefix_hash_key(const void *pp)
 		len = jhash((void *)copy.u.prefix_flowspec.ptr,
 			    copy.u.prefix_flowspec.prefixlen,
 			    0x55aa5a5a);
-		temp = (void *)copy.u.prefix_flowspec.ptr;
-		XFREE(MTYPE_PREFIX_FLOWSPEC, temp);
-		copy.u.prefix_flowspec.ptr = (uintptr_t)NULL;
+		prefix_flowspec_ptr_free(&copy);
 		return len;
 	} else if (((struct prefix *)pp)->family == AF_LINKSTATE) {
 		len = jhash((void *)copy.u.prefix_linkstate.ptr, copy.prefixlen,
diff --git a/lib/table.c b/lib/table.c
index 3a1cbc52d6..8e829d434d 100644
--- a/lib/table.c
+++ b/lib/table.c
@@ -284,6 +284,8 @@ struct route_node *route_node_get(struct route_table *table,
 	if (node && node->info) {
 		if (family2afi(p->family) == AFI_LINKSTATE)
 			prefix_linkstate_ptr_free(p);
+		else if (p->family == AF_FLOWSPEC)
+			prefix_flowspec_ptr_free(p);
 
 		return route_lock_node(node);
 	}
@@ -295,6 +297,8 @@ struct route_node *route_node_get(struct route_table *table,
 		if (node->p.prefixlen == prefixlen) {
 			if (family2afi(p->family) == AFI_LINKSTATE)
 				prefix_linkstate_ptr_free(p);
+			else if (p->family == AF_FLOWSPEC)
+				prefix_flowspec_ptr_free(p);
 			return route_lock_node(node);
 		}
 

[Keelan10]

@louis-6wind thank you for the valuable input!

[louis-6wind]

@Keelan10 Donald Sharp expressed some concerns on Slack about this.

See #14539