Skip to content
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

nhrpd: Fix nhrp_peer leak (backport #14579) #16165

Closed
wants to merge 1 commit into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Jun 5, 2024

  • Addressed memory leak by removing &c->peer_notifier from the notifier list on termination. Retaining it caused the notifier list to stay active, preventing the deletion of c->cur.peer thereby causing a memory leak.

  • Reordered termination steps to call vrf_terminate before nhrp_vc_terminate, preventing a heap-use-after-free issue when nhrp_vc_notify_del is invoked in nhrp_peer_check_delete.

  • Added an if statement to avoid passing NULL as hash to hash_release, which leads to a SIGSEGV.

The ASan leak log for reference:

***********************************************************************************
Address Sanitizer Error detected in nhrp_topo.test_nhrp_topo/r1.asan.nhrpd.20265

=================================================================
==20265==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7f80270c9b40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x7f8026ac1eb8 in qmalloc lib/memory.c:100
    #2 0x560fd648f0a6 in nhrp_peer_create nhrpd/nhrp_peer.c:175
    #3 0x7f8026a88d3f in hash_get lib/hash.c:147
    #4 0x560fd6490a5d in nhrp_peer_get nhrpd/nhrp_peer.c:228
    #5 0x560fd648a51a in nhrp_nhs_resolve_cb nhrpd/nhrp_nhs.c:297
    #6 0x7f80266b000f in resolver_cb_literal lib/resolver.c:234
    #7 0x7f8026b62e0e in event_call lib/event.c:1969
    #8 0x7f8026aa5437 in frr_run lib/libfrr.c:1213
    #9 0x560fd6488b4f in main nhrpd/nhrp_main.c:166
    #10 0x7f8025eb2c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: 112 byte(s) leaked in 1 allocation(s).
***********************************************************************************

***********************************************************************************
Address Sanitizer Error detected in nhrp_topo.test_nhrp_topo/r2.asan.nhrpd.20400

=================================================================
==20400==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7fb6e3ca5b40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x7fb6e369deb8 in qmalloc lib/memory.c:100
    #2 0x562652de40a6 in nhrp_peer_create nhrpd/nhrp_peer.c:175
    #3 0x7fb6e3664d3f in hash_get lib/hash.c:147
    #4 0x562652de5a5d in nhrp_peer_get nhrpd/nhrp_peer.c:228
    #5 0x562652de1e8e in nhrp_packet_recvraw nhrpd/nhrp_packet.c:325
    #6 0x7fb6e373ee0e in event_call lib/event.c:1969
    #7 0x7fb6e3681437 in frr_run lib/libfrr.c:1213
    #8 0x562652dddb4f in main nhrpd/nhrp_main.c:166
    #9 0x7fb6e2a8ec86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: 112 byte(s) leaked in 1 allocation(s).
***********************************************************************************
```<hr>This is an automatic backport of pull request #14579 done by [Mergify](https://mergify.com).

- Addressed memory leak by removing `&c->peer_notifier` from the notifier list on termination. Retaining it caused the notifier list to stay active, preventing the deletion of `c->cur.peer`
  thereby causing a memory leak.

- Reordered termination steps to call `vrf_terminate` before `nhrp_vc_terminate`, preventing a heap-use-after-free issue when `nhrp_vc_notify_del` is invoked in `nhrp_peer_check_delete`.

- Added an if statement to avoid passing NULL as hash to `hash_release`, which leads to a SIGSEGV.

The ASan leak log for reference:

```
***********************************************************************************
Address Sanitizer Error detected in nhrp_topo.test_nhrp_topo/r1.asan.nhrpd.20265

=================================================================
==20265==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7f80270c9b40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x7f8026ac1eb8 in qmalloc lib/memory.c:100
    #2 0x560fd648f0a6 in nhrp_peer_create nhrpd/nhrp_peer.c:175
    #3 0x7f8026a88d3f in hash_get lib/hash.c:147
    #4 0x560fd6490a5d in nhrp_peer_get nhrpd/nhrp_peer.c:228
    #5 0x560fd648a51a in nhrp_nhs_resolve_cb nhrpd/nhrp_nhs.c:297
    #6 0x7f80266b000f in resolver_cb_literal lib/resolver.c:234
    #7 0x7f8026b62e0e in event_call lib/event.c:1969
    #8 0x7f8026aa5437 in frr_run lib/libfrr.c:1213
    #9 0x560fd6488b4f in main nhrpd/nhrp_main.c:166
    #10 0x7f8025eb2c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: 112 byte(s) leaked in 1 allocation(s).
***********************************************************************************

***********************************************************************************
Address Sanitizer Error detected in nhrp_topo.test_nhrp_topo/r2.asan.nhrpd.20400

=================================================================
==20400==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 112 byte(s) in 1 object(s) allocated from:
    #0 0x7fb6e3ca5b40 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb40)
    #1 0x7fb6e369deb8 in qmalloc lib/memory.c:100
    #2 0x562652de40a6 in nhrp_peer_create nhrpd/nhrp_peer.c:175
    #3 0x7fb6e3664d3f in hash_get lib/hash.c:147
    #4 0x562652de5a5d in nhrp_peer_get nhrpd/nhrp_peer.c:228
    #5 0x562652de1e8e in nhrp_packet_recvraw nhrpd/nhrp_packet.c:325
    #6 0x7fb6e373ee0e in event_call lib/event.c:1969
    #7 0x7fb6e3681437 in frr_run lib/libfrr.c:1213
    #8 0x562652dddb4f in main nhrpd/nhrp_main.c:166
    #9 0x7fb6e2a8ec86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)

SUMMARY: AddressSanitizer: 112 byte(s) leaked in 1 allocation(s).
***********************************************************************************
```

Signed-off-by: Keelan Cannoo <keelan.cannoo@icloud.com>
(cherry picked from commit d163f89)

# Conflicts:
#	nhrpd/nhrp_main.c
Copy link
Author

mergify bot commented Jun 5, 2024

Cherry-pick of d163f89 has failed:

On branch mergify/bp/stable/9.1/pr-14579
Your branch is up to date with 'origin/stable/9.1'.

You are currently cherry-picking commit d163f89e1.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   nhrpd/nhrp_cache.c
	modified:   nhrpd/nhrp_peer.c

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   nhrpd/nhrp_main.c

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@ton31337
Copy link
Member

ton31337 commented Jun 5, 2024

Manual backport #16166, closing this.

@ton31337 ton31337 closed this Jun 5, 2024
@ton31337 ton31337 deleted the mergify/bp/stable/9.1/pr-14579 branch June 5, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants