fix(leaks): fix some memory leaks in rapiddisk #129
Conversation
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.
|
Before: After: |
Add one missing call to free() related to scandir() call in sys.c. Add in main.c a function to free the memory allocated in a linked list. Fix in json.c some missing free() calls related to the json_dumps() function calls.
There was a problem hiding this comment.
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.
ALSO, test my my comments and make sure that there are still stable before merging anything to the branch.
src/sys.c
Outdated
| @@ -106,6 +106,7 @@ struct VOLUME_PROFILE *search_volumes_targets(void) | |||
| } | |||
| if (list[n] != NULL) free(list[n]); | |||
| } | |||
| free(list); | |||
There was a problem hiding this comment.
Good catch. Although in good practice, I make sure that list is not null before I free to avoid a double freeing.
There was a problem hiding this comment.
Good catch. Although in good practice, I make sure that list is not null before I free to avoid a double freeing.
Ok, I'll add this test asap to the free() calls.
There was a problem hiding this comment.
Good catch. Although in good practice, I make sure that list is not null before I free to avoid a double freeing.
I read the docs, and found out that the pointer passed to free() is not nulled by default, I'll try to nullify it by hand.
src/sys.c
Outdated
| @@ -175,6 +176,7 @@ int check_loaded_modules(void) | |||
| } | |||
| if (list[n] != NULL) free(list[n]); | |||
| } | |||
| free(list); | |||
There was a problem hiding this comment.
So, there is a bigger "leak" here and while my if (list) free(list) comment still applies here, like above. Notice that we will exit on error at line 168/169 (the return rc) without freeing the array of structures because below, I am using them still.
Here is the thing though, it returns and exits the application, thereby freeing the memory regardless.
src/rdsk.c
Outdated
| @@ -97,6 +97,7 @@ struct RD_PROFILE *search_rdsk_targets(void) | |||
| } | |||
| if (list[n] != NULL) free(list[n]); | |||
| } | |||
| free(list); | |||
There was a problem hiding this comment.
if (list) free(list) is good practice to avoid a double free error.
src/rdsk.c
Outdated
| @@ -153,7 +155,9 @@ struct RC_PROFILE *search_cache_targets(void) | |||
| } | |||
| if (list[n] != NULL) free(list[n]); | |||
| } | |||
| free(list); | |||
There was a problem hiding this comment.
if (list) free(list) is good practice to avoid a double free error.
src/rdsk.c
Outdated
| for (i = 0;i < num2; i++) if (nodes[i] != NULL) free(nodes[i]); | ||
| free(nodes); |
There was a problem hiding this comment.
if (nodes) free(nodes) is good practice to avoid a double free error.
src/rdsk.c
Outdated
| @@ -144,6 +145,7 @@ struct RC_PROFILE *search_cache_targets(void) | |||
| } | |||
| } | |||
| } | |||
| free(maps); | |||
There was a problem hiding this comment.
This feels like it may be in the wrong spot and should be located above and below line 143/144 (after the closing brace) and after the for (z=0 loop.
Also if (maps) free(maps)
There was a problem hiding this comment.
Yes, you're right, fixed!
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.
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 Let me know wether my changes to Regards |
Add function clean_scandir() in sys.c to free lists created by scandir calls. Rework the switch(action) section in main.c. Add some #define for error messages in cli.h. Remove useless calls to json_decref() in json.c. Change some malloc() calls to calloc(). Change all the declarations of type "unsigned char *" to "char *". Add the debug target to Makefiles. Add test-leaks.sh script which invokes valgrind if found.
Add headers to targets in src/Makefile. Add new targets for debug in src/Makefile. Correct the debug target in main Makefile, add missing .PHONY and remove building of modules. Fix a printf() call not using the #defined error messages in main.c. Fix missing set to NULL of three vars in main.c.
Add three recursive functions to free linked lists, remove the old ones. Rework the switch(action) code section to improve json compatibility. Correct the test script to reflect the new name of debug executable.
|
Hi! These commits are included in PR #132 too, in branch Regards |
|
Closing per @matteotenca 's comments. |
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.