-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Items found via valgrind running and gcoverage #19719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
mjstapp
merged 5 commits into
FRRouting:master
from
donaldsharp:check_address_sanitizer
Oct 14, 2025
Merged
Items found via valgrind running and gcoverage #19719
mjstapp
merged 5 commits into
FRRouting:master
from
donaldsharp:check_address_sanitizer
Oct 14, 2025
Conversation
This file contains hidden or 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
Member
donaldsharp
commented
Oct 9, 2025
- babeld has a branch decision made on uninited data, this includes changes to a topotest to give us a bit more code coverage of babel
- Removed the bgp hope of SIGHUP. In other words we gave up on SIGHUP allowing BGP to start everything from anew a long time ago via function commenting out. Let's just finish it.
- ldpd has a send of uninitied data. Fix that.
Add a test to babel_topo1 that the interface is shut down and then set back up. Signed-off-by: Donald Sharp <[email protected]>
The SIGHUP code was intended to allow the operator to completely reset all of BGP. It was commented out several years ago because there had been no attempt at keeping it working and due to the complexity of BGP it was woefully out of date. Here we are several years beyond that now and we still have no way to do this and things have gotten more complicated since then. Let's just remove this completely dead code since it is no longer being used. Signed-off-by: Donald Sharp <[email protected]>
Looking at the imsg api it is clear to me that there is no way other daemons could use this api. I don't see any aplicability here, so lets let ldpd keep the overhead of these two .c files instead of having every daemon carry this unused data. Signed-off-by: Donald Sharp <[email protected]>
Stop this from happening: ==140921== Syscall param sendmsg(msg.msg_iov[2]) points to uninitialised byte(s) ==140921== at 0x4C5C9D7: sendmsg (sendmsg.c:28) ==140921== by 0x4942620: msgbuf_write (imsg-buffer.c:234) ==140921== by 0x12CA2F: ldp_write_handler (ldpd.c:753) ==140921== by 0x49E1037: event_call (event.c:2009) ==140921== by 0x495043B: frr_run (libfrr.c:1252) ==140921== by 0x12BCB3: main (ldpd.c:442) ==140921== Address 0x5fc0e38 is 72 bytes inside a block of size 136 alloc'd ==140921== at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==140921== by 0x4941D32: ibuf_open (imsg-buffer.c:24) ==140921== by 0x4941DBB: ibuf_dynamic (imsg-buffer.c:41) ==140921== by 0x494309B: imsg_create (imsg.c:248) ==140921== by 0x4942E8C: imsg_compose (imsg.c:192) ==140921== by 0x12CD5A: imsg_compose_event (ldpd.c:813) ==140921== by 0x12CBE3: main_imsg_compose_both (ldpd.c:786) ==140921== by 0x12D5F2: main_imsg_send_config (ldpd.c:1028) ==140921== by 0x12D827: ldp_config_apply (ldpd.c:1083) ==140921== by 0x14C968: ldp_vty_neighbor_targeted (ldp_vty_conf.c:883) ==140921== by 0x1493CB: ldp_neighbor_ipv4_targeted_magic (ldp_vty_cmds.c:388) ==140921== by 0x144D85: ldp_neighbor_ipv4_targeted (ldp_vty_cmds_clippy.c:1700) ==140921== by 0x490DB3D: cmd_execute_command_real (command.c:1010) ==140921== by 0x490DCB6: cmd_execute_command (command.c:1069) ==140921== by 0x490E263: cmd_execute (command.c:1235) ==140921== by 0x49E891E: vty_command (vty.c:643) ==140921== by 0x49EA69C: vty_execute (vty.c:1406) ==140921== by 0x49ECDF9: vtysh_read (vty.c:2431) ==140921== by 0x49E1037: event_call (event.c:2009) ==140921== by 0x495043B: frr_run (libfrr.c:1252) ==140921== by 0x12BCB3: main (ldpd.c:442) Signed-off-by: Donald Sharp <[email protected]>
Testing for a Sequence number in the sent message makes no sense as that a MESSAGE_REQUEST never attempts to send this data. Additionally there is no need to test for this because this code does not read past it. Signed-off-by: Donald Sharp <[email protected]>
a40b0fb to
0bbe9b1
Compare
mjstapp
approved these changes
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks fine; can't really comment on the babel test though!
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.