From 66b6000933397806113816e5bcaa6e7ca641305d Mon Sep 17 00:00:00 2001 From: Simon Jackson Date: Wed, 24 Apr 2024 17:06:17 +0000 Subject: [PATCH 1/2] Add new test demonstrating memory leak. --- include/modules/tag.h | 10 ++++++++ src/modules/tag.c | 58 +++++++++++++++++------------------------- test/assertion_utils.c | 15 +++++++++++ test/assertion_utils.h | 1 + test/set_test.c | 25 ++++++++++++++++++ 5 files changed, 74 insertions(+), 35 deletions(-) diff --git a/include/modules/tag.h b/include/modules/tag.h index fb78694..c3a9f69 100644 --- a/include/modules/tag.h +++ b/include/modules/tag.h @@ -62,6 +62,16 @@ ID3v2_FrameList* ID3v2_Tag_get_apic_frames(ID3v2_Tag* tag); /** * Setter functions */ +typedef struct _ID3v2_FrameInput +{ + const char* id; + const char* flags; + const char* data; + const int size; +} ID3v2_FrameInput; + +void ID3v2_Tag_set_frame(ID3v2_Tag* tag, const ID3v2_FrameInput* input); + typedef struct _ID3v2_TextFrameInput { const char* id; diff --git a/src/modules/tag.c b/src/modules/tag.c index 5350666..8b55ba8 100644 --- a/src/modules/tag.c +++ b/src/modules/tag.c @@ -198,12 +198,8 @@ ID3v2_FrameList* ID3v2_Tag_get_apic_frames(ID3v2_Tag* tag) /** * Setter functions */ -void ID3v2_Tag_set_text_frame(ID3v2_Tag* tag, ID3v2_TextFrameInput* input) +void ID3v2_Tag_set_frame_internal(ID3v2_Tag* tag, ID3v2_Frame* new_frame, ID3v2_Frame* existing_frame) { - ID3v2_TextFrame* new_frame = TextFrame_new(input->id, input->flags, input->text); - ID3v2_TextFrame* existing_frame = - (ID3v2_TextFrame*) FrameList_get_frame_by_id(tag->frames, input->id); - if (existing_frame == NULL) { FrameList_add_frame(tag->frames, (ID3v2_Frame*) new_frame); @@ -221,6 +217,26 @@ void ID3v2_Tag_set_text_frame(ID3v2_Tag* tag, ID3v2_TextFrameInput* input) } } +void ID3v2_Tag_set_frame(ID3v2_Tag* tag, const ID3v2_FrameInput* input) +{ + ID3v2_Frame* new_frame = (ID3v2_Frame*) malloc(sizeof(ID3v2_Frame)); + new_frame->header = FrameHeader_new(input->id, input->flags, input->size); + new_frame->data = (char*) malloc(input->size * sizeof(char)); + memcpy(new_frame->data, input->data, input->size); + + ID3v2_Frame* existing_frame = FrameList_get_frame_by_id(tag->frames, input->id); + ID3v2_Tag_set_frame_internal(tag, new_frame, existing_frame); +} + +void ID3v2_Tag_set_text_frame(ID3v2_Tag* tag, ID3v2_TextFrameInput* input) +{ + ID3v2_TextFrame* new_frame = TextFrame_new(input->id, input->flags, input->text); + ID3v2_TextFrame* existing_frame = + (ID3v2_TextFrame*) FrameList_get_frame_by_id(tag->frames, input->id); + + ID3v2_Tag_set_frame_internal(tag, (ID3v2_Frame*)new_frame, (ID3v2_Frame*)existing_frame); +} + void ID3v2_Tag_set_artist(ID3v2_Tag* tag, const char* artist) { ID3v2_Tag_set_text_frame( @@ -338,21 +354,7 @@ void ID3v2_Tag_set_comment_frame(ID3v2_Tag* tag, ID3v2_CommentFrameInput* input) CommentFrame_new(input->flags, input->language, input->short_description, input->comment); ID3v2_CommentFrame* existing_frame = ID3v2_Tag_get_comment_frame(tag); - if (existing_frame == NULL) - { - FrameList_add_frame(tag->frames, (ID3v2_Frame*) new_frame); - tag->header->tag_size += new_frame->header->size; - } - else - { - FrameList_replace_frame( - tag->frames, - (ID3v2_Frame*) existing_frame, - (ID3v2_Frame*) new_frame - ); - tag->header->tag_size += (new_frame->header->size - existing_frame->header->size); - ID3v2_Frame_free((ID3v2_Frame*) existing_frame); - } + ID3v2_Tag_set_frame_internal(tag, (ID3v2_Frame*)new_frame, (ID3v2_Frame*)existing_frame); } void ID3v2_Tag_add_comment_frame(ID3v2_Tag* tag, ID3v2_CommentFrameInput* input) @@ -391,21 +393,7 @@ void ID3v2_Tag_set_apic_frame(ID3v2_Tag* tag, ID3v2_ApicFrameInput* input) ); ID3v2_ApicFrame* existing_frame = ID3v2_Tag_get_album_cover_frame(tag); - if (existing_frame == NULL) - { - FrameList_add_frame(tag->frames, (ID3v2_Frame*) new_frame); - tag->header->tag_size += new_frame->header->size; - } - else - { - FrameList_replace_frame( - tag->frames, - (ID3v2_Frame*) existing_frame, - (ID3v2_Frame*) new_frame - ); - tag->header->tag_size += (new_frame->header->size - existing_frame->header->size); - ID3v2_Frame_free((ID3v2_Frame*) existing_frame); - } + ID3v2_Tag_set_frame_internal(tag, (ID3v2_Frame*)new_frame, (ID3v2_Frame*)existing_frame); } void ID3v2_Tag_add_apic_frame(ID3v2_Tag* tag, ID3v2_ApicFrameInput* input) diff --git a/test/assertion_utils.c b/test/assertion_utils.c index c00184c..718b4e6 100644 --- a/test/assertion_utils.c +++ b/test/assertion_utils.c @@ -24,6 +24,21 @@ void assert_frame_header(ID3v2_Frame* frame, Frame_header_assertion comparison) assert(memcmp(frame->header->flags, comparison.flags, ID3v2_FRAME_HEADER_FLAGS_LENGTH) == 0); } +void assert_frame(ID3v2_Frame* frame, ID3v2_FrameInput* comparison) { + // Header + assert_frame_header( + frame, + (Frame_header_assertion){ + .id = comparison->id, + .size = comparison->size, + .flags = comparison->flags, + } + ); + + // Data + assert(memcmp(frame->data, comparison->data, comparison->size) == 0); +} + void assert_text_frame(ID3v2_TextFrame* frame, ID3v2_TextFrameInput* comparison) { const char encoding = has_bom(comparison->text) ? ID3v2_ENCODING_UNICODE : ID3v2_ENCODING_ISO; diff --git a/test/assertion_utils.h b/test/assertion_utils.h index b9c7e27..9edfecd 100644 --- a/test/assertion_utils.h +++ b/test/assertion_utils.h @@ -19,6 +19,7 @@ typedef struct _Frame_header_assertion const char* flags; } Frame_header_assertion; +void assert_frame(ID3v2_Frame* frame, ID3v2_FrameInput* comparison); void assert_frame_header(ID3v2_Frame* frame, Frame_header_assertion comparison); void assert_text_frame(ID3v2_TextFrame* frame, ID3v2_TextFrameInput* comparison); void assert_comment_frame(ID3v2_CommentFrame* frame, ID3v2_CommentFrameInput* comparison); diff --git a/test/set_test.c b/test/set_test.c index dab7d22..001af7d 100644 --- a/test/set_test.c +++ b/test/set_test.c @@ -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}; + + // set a new, non-standard frame + ID3v2_Tag* tag = ID3v2_Tag_new_empty(); + ID3v2_FrameInput new_frame = { + .id = frame_id, + .flags = "\0\0", + .data = &frame_data[0], + .size = 8 + }; + ID3v2_Tag_set_frame(tag, &new_frame); + + // verify the frame + assert_frame( + ID3v2_Tag_get_frame(tag, frame_id), + &(ID3v2_FrameInput){ + .id = frame_id, + .flags = "\0\0", + .data = &frame_data[0], + .size = 8, + } + ); + + ID3v2_Tag_free(tag); } void set_test_main() From d4034f119e61209dcf26601f0fc9bdf5aff95a44 Mon Sep 17 00:00:00 2001 From: Simon Jackson Date: Thu, 25 Apr 2024 12:31:01 +0000 Subject: [PATCH 2/2] Still free the frame if the type is unknown. --- src/modules/frame.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/modules/frame.c b/src/modules/frame.c index 29d775e..cc0800b 100644 --- a/src/modules/frame.c +++ b/src/modules/frame.c @@ -104,5 +104,6 @@ void ID3v2_Frame_free(ID3v2_Frame* frame) // Unknown frame id, naively try our best to free it free(frame->header); free(frame->data); + free(frame); } }