rapiddisk
rapiddisk copied to clipboard
pkoutoupis
fix(leaks): fix some memory leaks in rapiddisk
Add some missing calls to free() related to scandir() calls. Add in main.c two function to free the memory allocated in two linked list.
The two function in main.c are not an elegant solution, but I did not have other ideas.
Before:
shub@ubuserver:/tmp/rapiddisk/src$ sudo valgrind -s --leak-check=full --show-leak-kinds=all ./rapiddisk -l
==6462== Memcheck, a memory error detector
==6462== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6462== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==6462== Command: ./rapiddisk -l
==6462==
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis
==6462== Warning: noted but unhandled ioctl 0x530 with no size/direction hints.
==6462== This could cause spurious value errors to appear.
==6462== See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
List of RapidDisk device(s):
RapidDisk Device 1: rd1 Size (KB): 262144 Usage (KB): 18014398509481983 Status: Unlocked
RapidDisk Device 2: rd2 Size (KB): 262144 Usage (KB): 18014398509481983 Status: Unlocked
RapidDisk Device 3: rd0 Size (KB): 65536 Usage (KB): 18014398509481983 Status: Unlocked
List of RapidDisk-Cache mapping(s):
RapidDisk-Cache Target 1: rc-wa_sda2 Cache: rd1 Target: sda2 (WRITE AROUND)
RapidDisk-Cache Target 2: rc-wt_sdb1 Cache: rd0 Target: sdb1 (WRITE THROUGH)
dm-writecache Target 3: rc-wb_rc-wa_sda2 Cache: rd2 Target: dm-1 (WRITEBACK)
==6462==
==6462== HEAP SUMMARY:
==6462== in use at exit: 6,536 bytes in 13 blocks
==6462== total heap usage: 294 allocs, 281 frees, 302,144 bytes allocated
==6462==
==6462== 80 bytes in 1 blocks are definitely lost in loss record 1 of 7
==6462== at 0x48487A9: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x495AD3A: __scandir64_tail (scandir-tail-common.c:62)
==6462== by 0x110C02: search_cache_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AFA8: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 144 bytes in 3 blocks are still reachable in loss record 2 of 7
==6462== at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x110993: search_rdsk_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AF9C: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 160 bytes in 1 blocks are definitely lost in loss record 3 of 7
==6462== at 0x484DCD3: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x495AD3A: __scandir64_tail (scandir-tail-common.c:62)
==6462== by 0x1108FC: search_rdsk_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AF9C: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 160 bytes in 1 blocks are definitely lost in loss record 4 of 7
==6462== at 0x484DCD3: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x495AD3A: __scandir64_tail (scandir-tail-common.c:62)
==6462== by 0x110C3E: search_cache_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AFA8: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 240 bytes in 3 blocks are definitely lost in loss record 5 of 7
==6462== at 0x48487A9: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x495AD3A: __scandir64_tail (scandir-tail-common.c:62)
==6462== by 0x110E75: search_cache_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AFA8: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 2,560 bytes in 1 blocks are definitely lost in loss record 6 of 7
==6462== at 0x484DCD3: realloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x495AD3A: __scandir64_tail (scandir-tail-common.c:62)
==6462== by 0x1144A2: check_loaded_modules (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7B4: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== 3,192 bytes in 3 blocks are still reachable in loss record 7 of 7
==6462== at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==6462== by 0x110CD5: search_cache_targets (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10AFA8: exec_cmdline_arg (in /tmp/rapiddisk/src/rapiddisk)
==6462== by 0x10B7F3: main (in /tmp/rapiddisk/src/rapiddisk)
==6462==
==6462== LEAK SUMMARY:
==6462== definitely lost: 3,200 bytes in 7 blocks
==6462== indirectly lost: 0 bytes in 0 blocks
==6462== possibly lost: 0 bytes in 0 blocks
==6462== still reachable: 3,336 bytes in 6 blocks
==6462== suppressed: 0 bytes in 0 blocks
==6462==
==6462== ERROR SUMMARY: 5 errors from 5 contexts (suppressed: 0 from 0)
After:
shub@ubuserver:/tmp/rapiddisk/src$ sudo valgrind -s --leak-check=full --show-leak-kinds=all ./rapiddisk -l
==6518== Memcheck, a memory error detector
==6518== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6518== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==6518== Command: ./rapiddisk -l
==6518==
rapiddisk 8.2.0
Copyright 2011 - 2022 Petros Koutoupis
==6518== Warning: noted but unhandled ioctl 0x530 with no size/direction hints.
==6518== This could cause spurious value errors to appear.
==6518== See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.
List of RapidDisk device(s):
RapidDisk Device 1: rd1 Size (KB): 262144 Usage (KB): 18014398509481983 Status: Unlocked
RapidDisk Device 2: rd2 Size (KB): 262144 Usage (KB): 18014398509481983 Status: Unlocked
RapidDisk Device 3: rd0 Size (KB): 65536 Usage (KB): 18014398509481983 Status: Unlocked
List of RapidDisk-Cache mapping(s):
RapidDisk-Cache Target 1: rc-wa_sda2 Cache: rd1 Target: sda2 (WRITE AROUND)
RapidDisk-Cache Target 2: rc-wt_sdb1 Cache: rd0 Target: sdb1 (WRITE THROUGH)
dm-writecache Target 3: rc-wb_rc-wa_sda2 Cache: rd2 Target: dm-1 (WRITEBACK)
==6518==
==6518== HEAP SUMMARY:
==6518== in use at exit: 0 bytes in 0 blocks
==6518== total heap usage: 294 allocs, 294 frees, 302,144 bytes allocated
==6518==
==6518== All heap blocks were freed -- no leaks are possible
==6518==
==6518== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
I still need to check more but I gave it an initial look this morning.
Originally, I was less concerned about memory leaks in this code only because the application lived a short life and exited, freeing all memory on exit. There seemed little risk. That being said, in good practice, we should still probably fix most, if not all of them.
I think you had much more complex pieces of code (kernel modules) han the CLI command to optimize/pay attention to. The "easy" part is in the cli commands code, this is clear to me, I found leaks because I started looking for those json leaks we discussed, and on the go I started fixing (or try to fix) the leaks valgrind complained with (both json-code related and non json-related). That's why I found some.
ALSO, test my my comments and make sure that there are still stable before merging anything to the branch.
Sorry for that, I will check it better.
I have ready a rather big commit with many leaks fixed (same kind of the ones I figured out and that you already saw).
While doing the work, I believe I did an error: you always used unsigned char * rather than char * for many pointer declarations, and array definitions, when you need to handle string-like data. I changed nearly all of them to char * anche char[] without asking... the IDE complained some macros/functions like strcmp expected char * and not unsigned char * and since we are speaking of strings I went hard and changed them. BUT this is not the right way to go, so before commit my latest changes I ask you why you went with unsigned char and not char for strings and if I should revert to unsigned char if I did wrong.
Let me know wether my changes to char and char * from unsigned char and unsigned char * are ok, or if I have to revert before commit.
Regards
Hi!
These commits are included in PR #132 too, in branch leaks/daemon_leaks_json, so I believe this request can be closed!
Regards
