larsbs / id3v2lib

id3v2lib is a library written in C to read and edit id3 tags from mp3 files.
BSD 2-Clause "Simplified" License
128 stars 44 forks source link

Fix memory leak when freeing unknown frame types. #56

Open sizeak opened 4 months ago

sizeak commented 4 months ago

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== Memcheck, a memory error detector ==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) ```