Devel fix valgrind #77
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
This should fix 2 errors reported by valgrind about accessing already free'd memory. We should not try to touch the memory pointed by t->lac after the lac was removed by xl2tpd-control. Relevant valgrind for the record ==27360== 2 errors in context 1 of 2: ==27360== Invalid write of size 8 ==27360== at 0x402D3F: destroy_tunnel (xl2tpd.c:663) ==27360== by 0x40DF5B: control_xmit (network.c:260) ==27360== by 0x40F696: process_schedule (scheduler.c:47) ==27360== by 0x40E1D5: network_thread (network.c:457) ==27360== by 0x401C35: main (xl2tpd.c:1881) ==27360== Address 0x51bbbe0 is 608 bytes inside a block of size 624 free'd ==27360== at 0x4C27D4E: free (vg_replace_malloc.c:427) ==27360== by 0x403661: control_handle_lac_remove (xl2tpd.c:1514) ==27360== by 0x40462D: do_control (xl2tpd.c:1593) ==27360== by 0x40E841: network_thread (network.c:481) ==27360== by 0x401C35: main (xl2tpd.c:1881) ==27360== ==27360== ==27360== 2 errors in context 2 of 2: ==27360== Invalid read of size 4 ==27360== at 0x402D38: destroy_tunnel (xl2tpd.c:664) ==27360== by 0x40DF5B: control_xmit (network.c:260) ==27360== by 0x40F696: process_schedule (scheduler.c:47) ==27360== by 0x40E1D5: network_thread (network.c:457) ==27360== by 0x401C35: main (xl2tpd.c:1881) ==27360== Address 0x51bbb74 is 500 bytes inside a block of size 624 free'd ==27360== at 0x4C27D4E: free (vg_replace_malloc.c:427) ==27360== by 0x403661: control_handle_lac_remove (xl2tpd.c:1514) ==27360== by 0x40462D: do_control (xl2tpd.c:1593) ==27360== by 0x40E841: network_thread (network.c:481) ==27360== by 0x401C35: main (xl2tpd.c:1881) Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
struct host was dynamically allocated, yet never released. Valgrind report excerpt for the record ==15901== 8,928 bytes in 93 blocks are definitely lost in loss record 6 of 8 ==15901== at 0x4C28BED: malloc (vg_replace_malloc.c:263) ==15901== by 0x40F9CB: set_lns (file.c:1108) ==15901== by 0x4116C6: parse_one_option (file.c:1474) ==15901== by 0x403FBC: parse_one_line (xl2tpd.c:992) ==15901== by 0x404143: control_handle_lac_add_modify (xl2tpd.c:1456) ==15901== by 0x40464D: do_control (xl2tpd.c:1594) ==15901== by 0x40E861: network_thread (network.c:481) ==15901== by 0x401C35: main (xl2tpd.c:1882) Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
- Simply free(me) would lose reference to me->ppp_buf. - Destroy_call(me) will do the job right (also free's up me->oldptyconf). - Destroy_call(me) before free(t) to prevent incorrectly accessing already free'd data - Struct call needs to be free'd in destroy_call. The original free() call was commented out code in 7c3a27c "Some malloc/free sanity patches". Not sure what the "totally wrong" referred to Relevant valgrind report ==15901== 391,792 (6,768 direct, 385,024 indirect) bytes in 94 blocks are defi nitely lost in loss record 8 of 8 ==15901== at 0x4C28BED: malloc (vg_replace_malloc.c:263) ==15901== by 0x405852: new_buf (misc.c:88) ==15901== by 0x40CAF6: new_payload (call.c:33) ==15901== by 0x40D5C0: new_call (call.c:549) ==15901== by 0x40345B: new_tunnel (xl2tpd.c:927) ==15901== by 0x40D7C9: get_call (call.c:665) ==15901== by 0x402F26: l2tp_call (xl2tpd.c:723) ==15901== by 0x403B47: control_handle_lac_connect (xl2tpd.c:1271) ==15901== by 0x40464D: do_control (xl2tpd.c:1594) ==15901== by 0x40E861: network_thread (network.c:481) ==15901== by 0x401C35: main (xl2tpd.c:1882) Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
References to lac structure should be cleared when removing the lac with xl2tpd-control. Relevant valgrind report ==22558== 1 errors in context 1 of 2: ==22558== Invalid write of size 8 ==22558== at 0x40D5C7: destroy_call (call.c:469) ==22558== by 0x402DC2: destroy_tunnel (xl2tpd.c:691) ==22558== by 0x40E762: control_xmit (network.c:260) ==22558== by 0x40FF16: process_schedule (scheduler.c:47) ==22558== by 0x40EA55: network_thread (network.c:457) ==22558== by 0x401C35: main (xl2tpd.c:1894) ==22558== Address 0x51bbbe8 is 616 bytes inside a block of size 624 free'd ==22558== at 0x4C27D4E: free (vg_replace_malloc.c:427) ==22558== by 0x40369C: control_handle_lac_remove (xl2tpd.c:1527) ==22558== by 0x4047BD: do_control (xl2tpd.c:1606) ==22558== by 0x40F0C1: network_thread (network.c:481) ==22558== by 0x401C35: main (xl2tpd.c:1894) ==22558== ==22558== ==22558== 1 errors in context 2 of 2: ==22558== Invalid read of size 4 ==22558== at 0x40D5C1: destroy_call (call.c:470) ==22558== by 0x402DC2: destroy_tunnel (xl2tpd.c:691) ==22558== by 0x40E762: control_xmit (network.c:260) ==22558== by 0x40FF16: process_schedule (scheduler.c:47) ==22558== by 0x40EA55: network_thread (network.c:457) ==22558== by 0x401C35: main (xl2tpd.c:1894) ==22558== Address 0x51bbb74 is 500 bytes inside a block of size 624 free'd ==22558== at 0x4C27D4E: free (vg_replace_malloc.c:427) ==22558== by 0x40369C: control_handle_lac_remove (xl2tpd.c:1527) ==22558== by 0x4047BD: do_control (xl2tpd.c:1606) ==22558== by 0x40F0C1: network_thread (network.c:481) ==22558== by 0x401C35: main (xl2tpd.c:1894) Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
CristiCimpianu
added a commit
to CristiCimpianu/xl2tpd
that referenced
this pull request
Sep 21, 2015
|
Hi, someone have a look at this please? Memory corruption and segmentation faults are really bad for a network service. |
|
HI. Thank you for your commit. We will be testing your pull request soon. |
|
Well, just to add that oom-killer came up after running lac remove/add for one night... |
yousong
added a commit
to openwrt/packages
that referenced
this pull request
Oct 2, 2015
The update is mainly for addressing some memory corruption and segementation faults issues observed when running xl2tpd in OpenWrt. The relevant upstream pull request was at link [1] [1] Devel fix valgrind #77, xelerance/xl2tpd#77 Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
yousong
added a commit
to openwrt/packages
that referenced
this pull request
Oct 2, 2015
The update is mainly for addressing some memory corruption and segementation faults issues observed when running xl2tpd in OpenWrt. The relevant upstream pull request was at link [1] [1] Devel fix valgrind #77, xelerance/xl2tpd#77 Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
luizluca
pushed a commit
to luizluca/openwrt-packages
that referenced
this pull request
Dec 16, 2015
The update is mainly for addressing some memory corruption and segementation faults issues observed when running xl2tpd in OpenWrt. The relevant upstream pull request was at link [1] [1] Devel fix valgrind openwrt#77, xelerance/xl2tpd#77 Signed-off-by: Yousong Zhou <yszhou4tech@gmail.com>
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.
This series started because a few memory corruption was observed when running xl2tpd under OpenWrt causing many segmentation fault errors.
The series was made on a x86_64 host with help of valgrind.
Then tested with xl2tpd-control by adding and removing lac to false lns.