From 1142d83adb9497cf8ca3fc34e0d3c4b77ec2a4f5 Mon Sep 17 00:00:00 2001 From: IllusionMan1212 Date: Wed, 25 Sep 2024 04:13:37 +0200 Subject: [PATCH] finally understood why the use-after-free bug happened --- TODO.md | 5 ++++- example/main_linux.odin | 12 +----------- flac.odin | 11 ----------- internal.odin | 2 ++ 4 files changed, 7 insertions(+), 23 deletions(-) diff --git a/TODO.md b/TODO.md index 4ebae00..aca50fa 100644 --- a/TODO.md +++ b/TODO.md @@ -6,6 +6,7 @@ Files:- Playback issues in the example program that may relate to the decoder:- 23 - Crunchy loud noise mixed with the actual audio data +uncommon/02 -> 6 channel audio might be incorrect (not a decoder issue afaik) uncommon/04 -> We get a bits per sample mismatch uncommon/10 -> We error with Invalid Signature. I assume for these two I should look for the frame sync bits. But maybe my decoder should reject these. idk. uncommon/11 -> We error with Invalid Signature @@ -34,8 +35,10 @@ Things:- [ ] Speed optimizations [ ] Seeking [ ] Fuzzing to test the stability of the code -[ ] Check if the use-after-free bug happens on Windows, and if so, if the address sanitizer will show us the full stacktrace +[x] Check if the use-after-free bug happens on Windows, and if so, if the address sanitizer will show us the full stacktrace instead of skipping some procs. + This was caused by `read_data()` and the tee reader writing into the buffer and us using some memory that was freed while the buffer + resized. Solution is to use buffer data BEFORE calling `read_data()` [ ] Encoder (low priority for now) diff --git a/example/main_linux.odin b/example/main_linux.odin index 854ba2e..540523d 100644 --- a/example/main_linux.odin +++ b/example/main_linux.odin @@ -54,14 +54,10 @@ main :: proc() { // TODO: channel mapping is incorrect for anything with more than 2 channels. channels := flac_data.metadata.channels - chmap := alsa.pcm_get_chmap(handle) - - fmt.println(chmap) - md5_ctx: md5.Context md5.init(&md5_ctx) for { - frame, err := flac.read_next_frame(reader, flac_data) + frame, err := flac.read_next_frame(reader, flac_data, allocator) if err != nil { if err == .EOF { break @@ -83,12 +79,6 @@ main :: proc() { } } - fmt.println("Channels:", frame.channels) - - if frame.channels == 6 { - fmt.println("Sample:", len(frame.samples)) - } - // TODO: resampling // 8 bps audio diff --git a/flac.odin b/flac.odin index de5b6f2..4460074 100644 --- a/flac.odin +++ b/flac.odin @@ -591,10 +591,6 @@ read_next_frame :: proc(r: ^Reader, flac: ^Flac, allocator := context.allocator) tee_r: io.Tee_Reader crc_buffer: Buffer - // NOTE: using the default heap allocator here causes a use-after-free bug on files subset/05, subset/06, and subset/25 - // No idea what the cause of the bug is. - // - // Resolved without understanding the root issue. See below code for a note on this. buffer_init_allocator(&crc_buffer, 0, 128) defer buffer_destroy(&crc_buffer) @@ -806,13 +802,6 @@ read_next_frame :: proc(r: ^Reader, flac: ^Flac, allocator := context.allocator) // Frame Footer // crc16_bytes := buffer_to_bytes(&crc_buffer) - // NOTE: The order of these two calls is important. If we're using the default heap allocator - // for crc_buffer and we call read_data before calculate_crc16 then we run into a memory bug. - // I'm still not sure the root cause of the bug but it disappears when they're in this order. - // - // In the below cases the bug doesn't happen and the order doesn't matter. - // When using the temp allocator or an arena allocator. - // When using the reader from the arguments instead of the tee reader. calculated_frame_crc := calculate_crc16(crc16_bytes) frame_crc := u16(read_data(r, u16be) or_return) diff --git a/internal.odin b/internal.odin index f503bcf..3520433 100644 --- a/internal.odin +++ b/internal.odin @@ -6,6 +6,7 @@ import "core:crypto/legacy/md5" FLAC_MAGIC :: 'f' << 24 | 'L' << 16 | 'a' << 8 | 'C' //odinfmt: disable +@(rodata) crc8_lookup := [256]u8 { 0x00, 0x07, 0x0E, 0x09, 0x1C, 0x1B, 0x12, 0x15, 0x38, 0x3F, 0x36, 0x31, 0x24, 0x23, 0x2A, 0x2D, @@ -41,6 +42,7 @@ crc8_lookup := [256]u8 { 0xE6, 0xE1, 0xE8, 0xEF, 0xFA, 0xFD, 0xF4, 0xF3, } +@(rodata) crc16_lookup := [256]u16 { 0x0000, 0x8005, 0x800F, 0x000A, 0x801B, 0x001E, 0x0014, 0x8011, 0x8033, 0x0036, 0x003C, 0x8039, 0x0028, 0x802D, 0x8027, 0x0022,