bgpd: When allocating prefix, free it when we are already tracking it #12539
Conversation
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 arm8 part 8: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 8: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-8948/artifact/TOPO8U18AMD64/TopotestDetails/ Topotests Ubuntu 18.04 arm8 part 8: No useful log foundSuccessful on other platforms/tests
|
|
ci:rerun test system goes belly up again |
There was a problem hiding this comment.
Can't we allocate that in stack instead of heap?
I think your fix is still not correct, because even when the command doesn't fail midway it will still leak. The bgp_debug_list_add doesn't keep the pointer:
static void bgp_debug_list_add_entry(struct list *list, const char *host,
const struct prefix *p)
{
struct bgp_debug_filter *filter;
filter = XCALLOC(MTYPE_BGP_DEBUG_FILTER,
sizeof(struct bgp_debug_filter));
if (host) {
filter->host = XSTRDUP(MTYPE_BGP_DEBUG_STR, host);
filter->p = NULL;
} else if (p) {
filter->host = NULL;
filter->p = prefix_new();
prefix_copy(filter->p, p);
}
listnode_add(list, filter);
}( https://github.com/FRRouting/frr/blob/master/bgpd/bgp_debug.c#L306 )
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-8967/ This is a comment from an automated CI system. |
Several functions had this pattern:
a) p = prefix_new
b) if (already_tracking)
return
Let's just stop allocating the prefix and use a prefix on the
stack, especially since the function used to hold the value
actually copies it.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
donaldsharp
force-pushed
the
bgp_debug_prefix_mem_leak
branch
from
df2bbb8
to
11bf274
Compare
|
@rzalamena fixed to not use allocated memory |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-8985/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
2 Static Analyzer issues remaining.See details at |
|
Ops. static analyzer issue sneaked in here. I'll open a PR to fix it. |
Several functions had this pattern:
a) p = prefix_new
b) if (already_tracking)
return
b) needs to free the newly allocated prefix else it is leaked.
Signed-off-by: Donald Sharp sharpd@nvidia.com