bgpd: fix various memory leaks in dampening #9217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
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 amd64 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9U18AMD64-20615/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO9U18AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO9U18I386-20615/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 9: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO9U18I386/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 arm8 part 3: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 3: No useful log foundTopotests Ubuntu 18.04 amd64 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP3U1804AMD64-20615/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TP3U1804AMD64/ErrorLog/log_topotests.txt Topotests debian 10 amd64 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3DEB10AMD64-20615/test Topology Tests failed for Topotests debian 10 amd64 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO3DEB10AMD64/ErrorLog/log_topotests.txt Topotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO3U18I386-20615/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 3: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20615/artifact/TOPO3U18I386/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
Current code is a complete misuse of SLIST structure. Instead of just adding a SLIST_ENTRY to struct bgp_damp_info, it allocates a separate structure to be a node in the list. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
bdi->path is never NULL, therefore the structure was never freed. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
bgp_damp_config, afi and safi are never used. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
When bgp_damp_info_free is called from bgp_route.c, we were never deleting the BDI from the list. Move the deletion inside bgp_damp_info_free to cover all cases. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
This code always used double-linked list before this rework in 8.0 that introduced all these crashes and memory leaks. Using single-linked list is actually a performance regression, because there are frequent removes here and single-linked list obviously handles removes much worse. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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 amd64 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-20647/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20647/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
|
ci:rerun |
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-FRRPULLREQ-20653/ This is a comment from an automated CI system. |
ton31337
reviewed
Jul 30, 2021
|
@Mergifyio backport stable/8.0 |
|
Command
|
|
Command
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overall this PR aims to fix multiple memory leaks in the dampening code. All the leaks are regressions in 8.0 and I want this code to be backported even though there are a couple of cleanup-only commits. It is difficult to fix the leaks without doing the cleanup.
Please, check individual commits for descriptions.
The last commit changes the SLIST to a LIST. The dampening code was always using a double-linked list before the rework in 8.0 that introduced all those crashes and memory leaks. As there are frequent removals from the list, using a single-linked list is actually a real performance regression so changing it back to double-linked list should be considered as a fix.