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

Fix memory leak when freeing unknown frame types. #56

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

sizeak
Copy link
Contributor

@sizeak sizeak commented Apr 25, 2024

Hi,

There seems to be a memory leak when freeing frames of an unknown type. I discovered this while analysing my library that depends on id3v2 with valgrind. I've added a test case and supporting code to demonstrate the leak, then fixed it in the next commit.

It's an easy thing to miss, the frame itself wasn't being freed when the frame type was unknown, whereas it is freed by the frame types specific functions.

I've verified before & after with valgrind --leak-check=full ./main_test:

Valgrind logs with leak
==8597== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8597== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==8597== Command: ./main_test
==8597== 
==8597== Warning: client switching stacks?  SP change: 0x1ffed32100 --> 0x1ffeffec90
==8597==          to suppress, use: --max-stackframe=2935696 or greater
GET TEST EXISTING: OK
GET TEST EMPTY: OK
==8597== Warning: client switching stacks?  SP change: 0x1ffed32070 --> 0x1ffeffec00
==8597==          to suppress, use: --max-stackframe=2935696 or greater
==8597== Warning: client switching stacks?  SP change: 0x1ffed32070 --> 0x1ffeffec00
==8597==          to suppress, use: --max-stackframe=2935696 or greater
==8597==          further instances of this message will not be shown.
SET TEST: OK
DELETE TEST: OK
COMPAT TEST: OK
==8597== 
==8597== HEAP SUMMARY:
==8597==     in use at exit: 16 bytes in 1 blocks
==8597==   total heap usage: 1,119 allocs, 1,118 frees, 108,869,627 bytes allocated
==8597== 
==8597== 16 bytes in 1 blocks are definitely lost in loss record 1 of 1
==8597==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==8597==    by 0x10C864: ID3v2_Tag_set_frame (tag.c:222)
==8597==    by 0x10B566: new_tag_test (set_test.c:262)
==8597==    by 0x10B5EC: set_test_main (set_test.c:281)
==8597==    by 0x10ABBD: main (main_test.c:20)
==8597== 
==8597== LEAK SUMMARY:
==8597==    definitely lost: 16 bytes in 1 blocks
==8597==    indirectly lost: 0 bytes in 0 blocks
==8597==      possibly lost: 0 bytes in 0 blocks
==8597==    still reachable: 0 bytes in 0 blocks
==8597==         suppressed: 0 bytes in 0 blocks
==8597== 
==8597== For lists of detected and suppressed errors, rerun with: -s
==8597== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
Valgrind logs after fix
==9784== Memcheck, a memory error detector
==9784== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9784== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==9784== Command: ./main_test
==9784== 
==9784== Warning: client switching stacks?  SP change: 0x1ffed320f0 --> 0x1ffeffec80
==9784==          to suppress, use: --max-stackframe=2935696 or greater
GET TEST EXISTING: OK
GET TEST EMPTY: OK
==9784== Warning: client switching stacks?  SP change: 0x1ffed32060 --> 0x1ffeffebf0
==9784==          to suppress, use: --max-stackframe=2935696 or greater
==9784== Warning: client switching stacks?  SP change: 0x1ffed32060 --> 0x1ffeffebf0
==9784==          to suppress, use: --max-stackframe=2935696 or greater
==9784==          further instances of this message will not be shown.
SET TEST: OK
DELETE TEST: OK
COMPAT TEST: OK
==9784== 
==9784== HEAP SUMMARY:
==9784==     in use at exit: 0 bytes in 0 blocks
==9784==   total heap usage: 1,119 allocs, 1,119 frees, 108,869,627 bytes allocated
==9784== 
==9784== All heap blocks were freed -- no leaks are possible
==9784== 
==9784== For lists of detected and suppressed errors, rerun with: -s
==9784== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@@ -248,6 +248,31 @@ void edit_test()

void new_tag_test()
{
const char* frame_id = "GEOB";
const char frame_data[8] = {0xD, 0xE, 0xA, 0xD, 0xB, 0xE, 0xE, 0xF};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't just use a string here because I thought it made it clearer that this isn't supposed to be testing text frames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant