From f7491f9741a0fa2e623d321882b29a49c35596d8 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 14 Jul 2016 10:06:23 +0200 Subject: [PATCH] stream_decoder: fix memory leak after seek table read error When read_metadata_seektable_() fails, the has_seek_table flag is never set to true, and thus free() is never called. Example valgrind output: 11,185,464 bytes in 1 blocks are definitely lost in loss record 62 of 62 at 0x4C2BC0F: malloc (vg_replace_malloc.c:299) by 0x4C2DE6F: realloc (vg_replace_malloc.c:785) by 0x40A7880: safe_realloc_ (alloc.h:159) by 0x40A7911: safe_realloc_mul_2op_ (alloc.h:205) by 0x40AB6B5: read_metadata_seektable_ (stream_decoder.c:1654) by 0x40AAB2D: read_metadata_ (stream_decoder.c:1422) by 0x40A9C79: FLAC__stream_decoder_process_until_end_of_metadata (stream_decoder.c:1055) It is easy to craft a FLAC file which leaks megabytes of memory on every attempt to open the file. This patch fixes the problem by removing checks which are unnecessary (and harmful). Checking the has_seek_table flag is not enough, as described above. The NULL check is not harmful, but is not helpful either, because free(NULL) is documented to be legal. After running this code block, we're in a well-known safe state, no matter how inconsistent pointer and flag may have been before, for whatever reasons. Signed-off-by: Erik de Castro Lopo --- src/libFLAC/stream_decoder.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libFLAC/stream_decoder.c b/src/libFLAC/stream_decoder.c index 91535ac5..8123c267 100644 --- a/src/libFLAC/stream_decoder.c +++ b/src/libFLAC/stream_decoder.c @@ -643,11 +643,10 @@ FLAC_API FLAC__bool FLAC__stream_decoder_finish(FLAC__StreamDecoder *decoder) */ FLAC__MD5Final(decoder->private_->computed_md5sum, &decoder->private_->md5context); - if(decoder->private_->has_seek_table && 0 != decoder->private_->seek_table.data.seek_table.points) { - free(decoder->private_->seek_table.data.seek_table.points); - decoder->private_->seek_table.data.seek_table.points = 0; - decoder->private_->has_seek_table = false; - } + free(decoder->private_->seek_table.data.seek_table.points); + decoder->private_->seek_table.data.seek_table.points = 0; + decoder->private_->has_seek_table = false; + FLAC__bitreader_free(decoder->private_->input); for(i = 0; i < FLAC__MAX_CHANNELS; i++) { /* WATCHOUT: @@ -977,11 +976,11 @@ FLAC_API FLAC__bool FLAC__stream_decoder_reset(FLAC__StreamDecoder *decoder) decoder->protected_->state = FLAC__STREAM_DECODER_SEARCH_FOR_METADATA; decoder->private_->has_stream_info = false; - if(decoder->private_->has_seek_table && 0 != decoder->private_->seek_table.data.seek_table.points) { - free(decoder->private_->seek_table.data.seek_table.points); - decoder->private_->seek_table.data.seek_table.points = 0; - decoder->private_->has_seek_table = false; - } + + free(decoder->private_->seek_table.data.seek_table.points); + decoder->private_->seek_table.data.seek_table.points = 0; + decoder->private_->has_seek_table = false; + decoder->private_->do_md5_checking = decoder->protected_->md5_checking; /* * This goes in reset() and not flush() because according to the spec, a -- GitLab