Fix potential memory leak#1584
Conversation
|
Thanks for another PR! I'm working on prep to publish ClamAV 1.5.0 early next week and won't be able to merge this until after. |
Add free and replace with textDestroy to avoid potential memory leak. Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to address potential memory leaks in libclamav by ensuring dynamically allocated text/sample-name resources are properly freed on certain paths.
Changes:
- Replace a raw
free()withtextDestroy()aftertextMove()intextAddMessage(). - Add additional cleanup loops in
clamav_stats_add_sample()to freevirus_nameentries on allocation failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libclamav/text.c | Switches cleanup from free() to textDestroy() for anotherText after moving text nodes. |
| libclamav/stats.c | Adds additional freeing of virus_name strings in several allocation-failure paths. |
Comments suppressed due to low confidence (3)
libclamav/stats.c:217
- In this error path, if the new sample was prepended to an existing intel->samples list, setting intel->samples = NULL when (sample == intel->samples) drops the rest of the list and leaks existing samples. The failure cleanup should detach the new node and restore intel->samples to sample->next (and clear the new head's prev).
free(sample);
if (sample == intel->samples)
intel->samples = NULL;
libclamav/stats.c:242
- Same list-corruption issue in this error path: if the newly allocated sample was inserted at the head, setting intel->samples = NULL will orphan any previously queued samples. Cleanup should remove only the new sample and restore the list head/prev links.
free(sample);
if (sample == intel->samples)
intel->samples = NULL;
libclamav/stats.c:254
- Same list-corruption issue in this error path: if there were existing samples and the new one was prepended, setting intel->samples = NULL discards the rest of the list. Cleanup should restore intel->samples to the previous head (sample->next) and fix prev pointers.
free(sample);
if (sample == intel->samples)
intel->samples = NULL;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (aText) { | ||
| text *newHead = textMove(aText, anotherText); | ||
| free(anotherText); | ||
| textDestroy(anotherText); |
| for (i = 0; sample->virus_name[i] != NULL; i++) | ||
| free(sample->virus_name[i]); |
|
Interestingly, there was overlap with this change and a larger PR i'm working on for the email module: https://github.com/Cisco-Talos/clamav/pull/1720/changes#diff-44edf90419994260f33966e8a9d984e79383352ab996de74121c2f227b85eae5 My PR didn't entirely fix the issue you found in text.c. I'm going to update it to do that rather than take this PR because it will have a merge conflcit otherwise. The changes to the stats module are likely good other than the issue Copilot just found. But the capability to send those stats was yanked a long time ago (removed in version 0.100.0). So the stats code is dead code. Instead of working on code quality tweaks to dead code, we should just delete the stats code. Because of this, I'm going to close this PR rather than merge it. |
|
I added a commit to that other PR and gave you credit in the commit message. |
Add free and replace with textDestroy to avoid potential memory leak.