From a988b9b0259e7d0b1090913087369dd504cd0f45 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 18 Jul 2009 22:45:56 +0200 Subject: ape: check the tag size (fixes integer underflow) The expression "tagLen - size > 0" may result in an integer underflow and a buffer overflow, when "size" is larger than "tagLen". "size" is read from the input file, and must not be trusted. This patch changes the expression to "tagLen > size", which is a lot safer. --- src/tag_ape.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/tag_ape.c b/src/tag_ape.c index d1249fcb2..0d504dc7d 100644 --- a/src/tag_ape.c +++ b/src/tag_ape.c @@ -112,7 +112,7 @@ tag_ape_load(const char *file) /* get the key */ key = p; - while (tagLen - size > 0 && *p != '\0') { + while (tagLen > size && *p != '\0') { p++; tagLen--; } -- cgit v1.2.3 From e3ff0ab6d1f378aec9b98fe930ca42d1f428409e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Jul 2009 17:37:02 +0200 Subject: tag_ape: removed redundant length check Extend the tagLen check after reading it. Removed the second (redundant) check after the subtraction. --- src/tag_ape.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/tag_ape.c b/src/tag_ape.c index 0d504dc7d..ef921141b 100644 --- a/src/tag_ape.c +++ b/src/tag_ape.c @@ -22,6 +22,7 @@ #include +#include #include struct tag * @@ -86,15 +87,15 @@ tag_ape_load(const char *file) /* find beginning of ape tag */ tagLen = GUINT32_FROM_LE(footer.length); - if (tagLen < sizeof(footer)) + if (tagLen <= sizeof(footer) + 10) goto fail; if (fseek(fp, size - tagLen, SEEK_SET)) goto fail; /* read tag into buffer */ tagLen -= sizeof(footer); - if (tagLen <= 0) - goto fail; + assert(tagLen > 10); + buffer = g_malloc(tagLen); if (fread(buffer, 1, tagLen, fp) != tagLen) goto fail; -- cgit v1.2.3 From 0ce727d5d459c2319edc507eb2e71af8a1c9d5dc Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sun, 19 Jul 2009 17:38:46 +0200 Subject: ape: added protection against large memory allocations The function tag_ape_load() retrieves a 32 bit unsigned integer from the input file, and passes it to g_malloc(). This is dangerous, and may be used for a denial of service attack on MPD. --- src/tag_ape.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src') diff --git a/src/tag_ape.c b/src/tag_ape.c index ef921141b..7cbf32208 100644 --- a/src/tag_ape.c +++ b/src/tag_ape.c @@ -89,6 +89,9 @@ tag_ape_load(const char *file) tagLen = GUINT32_FROM_LE(footer.length); if (tagLen <= sizeof(footer) + 10) goto fail; + if (tagLen > 1024 * 1024) + /* refuse to load more than one megabyte of tag data */ + goto fail; if (fseek(fp, size - tagLen, SEEK_SET)) goto fail; -- cgit v1.2.3 From 322ef3cb805dacff84aea1e18a840e2bbf8cc881 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 22 Jul 2009 12:57:03 +0200 Subject: mad: skip ID3 frames when libid3tag is disabled When libid3tag is disabled, the libmad decoder plugin is unable to identify ID3 frames. If the file starts with an (unidentified) ID3 frame, it assumes that the file is not a valid MP3 song. This patch solves this by adding minimal stubs for the ID3 functions. --- src/decoder/mad_plugin.c | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/decoder/mad_plugin.c b/src/decoder/mad_plugin.c index c6b9d32d3..1ef7183fa 100644 --- a/src/decoder/mad_plugin.c +++ b/src/decoder/mad_plugin.c @@ -350,11 +350,11 @@ parse_id3_replay_gain_info(struct id3_tag *tag) } #endif -#ifdef HAVE_ID3TAG static void mp3_parse_id3(struct mp3_data *data, size_t tagsize, struct tag **mpd_tag, struct replay_gain_info **replay_gain_info_r) { +#ifdef HAVE_ID3TAG struct id3_tag *id3_tag = NULL; id3_length_t count; id3_byte_t const *id3_data; @@ -418,8 +418,34 @@ static void mp3_parse_id3(struct mp3_data *data, size_t tagsize, id3_tag_delete(id3_tag); g_free(allocated); -} +#else /* !HAVE_ID3TAG */ + (void)mpd_tag; + (void)replay_gain_info_r; + + /* This code is enabled when libid3tag is disabled. Instead + of parsing the ID3 frame, it just skips it. */ + + mad_stream_skip(&data->stream, tagsize); #endif +} + +#ifndef HAVE_ID3TAG +/** + * This function emulates libid3tag when it is disabled. Instead of + * doing a real analyzation of the frame, it just checks whether the + * frame begins with the string "ID3". If so, it returns the full + * length. + */ +static signed long +id3_tag_query(const void *p0, size_t length) +{ + const char *p = p0; + + return length > 3 && memcmp(p, "ID3", 3) == 0 + ? length + : 0; +} +#endif /* !HAVE_ID3TAG */ static enum mp3_action decode_next_frame_header(struct mp3_data *data, G_GNUC_UNUSED struct tag **tag, @@ -433,7 +459,6 @@ decode_next_frame_header(struct mp3_data *data, G_GNUC_UNUSED struct tag **tag, return DECODE_BREAK; } if (mad_header_decode(&data->frame.header, &data->stream)) { -#ifdef HAVE_ID3TAG if ((data->stream).error == MAD_ERROR_LOSTSYNC && (data->stream).this_frame) { signed long tagsize = id3_tag_query((data->stream). @@ -454,7 +479,6 @@ decode_next_frame_header(struct mp3_data *data, G_GNUC_UNUSED struct tag **tag, return DECODE_CONT; } } -#endif if (MAD_RECOVERABLE((data->stream).error)) { return DECODE_SKIP; } else { @@ -493,7 +517,6 @@ decodeNextFrame(struct mp3_data *data) return DECODE_BREAK; } if (mad_frame_decode(&data->frame, &data->stream)) { -#ifdef HAVE_ID3TAG if ((data->stream).error == MAD_ERROR_LOSTSYNC) { signed long tagsize = id3_tag_query((data->stream). this_frame, @@ -506,7 +529,6 @@ decodeNextFrame(struct mp3_data *data) return DECODE_CONT; } } -#endif if (MAD_RECOVERABLE((data->stream).error)) { return DECODE_SKIP; } else { -- cgit v1.2.3 From 8e2d9879962775bb29aa4d7556666b2216353bbe Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 22 Jul 2009 13:31:43 +0200 Subject: decoder/flac: removed misplaced authorship comment This belongs into "git annotate" or AUTHORS. --- src/decoder/_flac_common.c | 1 - 1 file changed, 1 deletion(-) (limited to 'src') diff --git a/src/decoder/_flac_common.c b/src/decoder/_flac_common.c index 713dfe9b2..a6c3a2134 100644 --- a/src/decoder/_flac_common.c +++ b/src/decoder/_flac_common.c @@ -67,7 +67,6 @@ flac_find_float_comment(const FLAC__StreamMetadata *block, return false; } -/* replaygain stuff by AliasMrJones */ static void flac_parse_replay_gain(const FLAC__StreamMetadata *block, struct flac_data *data) -- cgit v1.2.3 From cf1fd2b0da4be4675ddb16c5421d949a0e923b20 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 22 Jul 2009 13:31:46 +0200 Subject: decoder/flac: return early from flac_find_float_comment() When one metadata check fails, return quickly. This removes 2 levels of indent. --- src/decoder/_flac_common.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) (limited to 'src') diff --git a/src/decoder/_flac_common.c b/src/decoder/_flac_common.c index a6c3a2134..a450b6886 100644 --- a/src/decoder/_flac_common.c +++ b/src/decoder/_flac_common.c @@ -44,27 +44,28 @@ static bool flac_find_float_comment(const FLAC__StreamMetadata *block, const char *cmnt, float *fl) { - int offset = - FLAC__metadata_object_vorbiscomment_find_entry_from(block, 0, cmnt); - - if (offset >= 0) { - size_t pos = strlen(cmnt) + 1; /* 1 is for '=' */ - int len = block->data.vorbis_comment.comments[offset].length - - pos; - if (len > 0) { - unsigned char tmp; - unsigned char *p = &(block->data.vorbis_comment. - comments[offset].entry[pos]); - tmp = p[len]; - p[len] = '\0'; - *fl = (float)atof((char *)p); - p[len] = tmp; - - return true; - } - } - - return false; + int offset; + size_t pos; + int len; + unsigned char tmp, *p; + + offset = FLAC__metadata_object_vorbiscomment_find_entry_from(block, 0, + cmnt); + if (offset < 0) + return false; + + pos = strlen(cmnt) + 1; /* 1 is for '=' */ + len = block->data.vorbis_comment.comments[offset].length - pos; + if (len <= 0) + return false; + + p = &block->data.vorbis_comment.comments[offset].entry[pos]; + tmp = p[len]; + p[len] = '\0'; + *fl = (float)atof((char *)p); + p[len] = tmp; + + return true; } static void -- cgit v1.2.3 From 47ed89bd4c6499d475d5f16cb89d7be95763670c Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 22 Jul 2009 13:31:48 +0200 Subject: decoder/flac: parse all replaygain tags The FLAC replaygain parser used the "||" operator. This made the code stop after the first value which was found. --- src/decoder/_flac_common.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) (limited to 'src') diff --git a/src/decoder/_flac_common.c b/src/decoder/_flac_common.c index a450b6886..9e2d9f437 100644 --- a/src/decoder/_flac_common.c +++ b/src/decoder/_flac_common.c @@ -40,9 +40,9 @@ flac_data_init(struct flac_data *data, struct decoder * decoder, data->tag = NULL; } -static bool +static void flac_find_float_comment(const FLAC__StreamMetadata *block, - const char *cmnt, float *fl) + const char *cmnt, float *fl, bool *found_r) { int offset; size_t pos; @@ -52,12 +52,12 @@ flac_find_float_comment(const FLAC__StreamMetadata *block, offset = FLAC__metadata_object_vorbiscomment_find_entry_from(block, 0, cmnt); if (offset < 0) - return false; + return; pos = strlen(cmnt) + 1; /* 1 is for '=' */ len = block->data.vorbis_comment.comments[offset].length - pos; if (len <= 0) - return false; + return; p = &block->data.vorbis_comment.comments[offset].entry[pos]; tmp = p[len]; @@ -65,28 +65,32 @@ flac_find_float_comment(const FLAC__StreamMetadata *block, *fl = (float)atof((char *)p); p[len] = tmp; - return true; + *found_r = true; } static void flac_parse_replay_gain(const FLAC__StreamMetadata *block, struct flac_data *data) { - bool found; + bool found = false; if (data->replay_gain_info) replay_gain_info_free(data->replay_gain_info); data->replay_gain_info = replay_gain_info_new(); - found = flac_find_float_comment(block, "replaygain_album_gain", - &data->replay_gain_info->tuples[REPLAY_GAIN_ALBUM].gain) || - flac_find_float_comment(block, "replaygain_album_peak", - &data->replay_gain_info->tuples[REPLAY_GAIN_ALBUM].peak) || - flac_find_float_comment(block, "replaygain_track_gain", - &data->replay_gain_info->tuples[REPLAY_GAIN_TRACK].gain) || - flac_find_float_comment(block, "replaygain_track_peak", - &data->replay_gain_info->tuples[REPLAY_GAIN_TRACK].peak); + flac_find_float_comment(block, "replaygain_album_gain", + &data->replay_gain_info->tuples[REPLAY_GAIN_ALBUM].gain, + &found); + flac_find_float_comment(block, "replaygain_album_peak", + &data->replay_gain_info->tuples[REPLAY_GAIN_ALBUM].peak, + &found); + flac_find_float_comment(block, "replaygain_track_gain", + &data->replay_gain_info->tuples[REPLAY_GAIN_TRACK].gain, + &found); + flac_find_float_comment(block, "replaygain_track_peak", + &data->replay_gain_info->tuples[REPLAY_GAIN_TRACK].peak, + &found); if (!found) { replay_gain_info_free(data->replay_gain_info); -- cgit v1.2.3 From f4b39bc263b46685742ad120ac9a02d4ba022532 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 22 Jul 2009 13:34:33 +0200 Subject: decoder/flac: fixed indentation of flac_comment_value() --- src/decoder/_flac_common.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/decoder/_flac_common.c b/src/decoder/_flac_common.c index 9e2d9f437..ae7d039ce 100644 --- a/src/decoder/_flac_common.c +++ b/src/decoder/_flac_common.c @@ -110,25 +110,27 @@ flac_comment_value(const FLAC__StreamMetadata_VorbisComment_Entry *entry, size_t char_tnum_length = 0; const char *comment = (const char*)entry->entry; - if (entry->length > name_length && - g_ascii_strncasecmp(comment, name, name_length) == 0) { - if (char_tnum != NULL) { - char_tnum_length = strlen(char_tnum); - if (entry->length > name_length + char_tnum_length + 2 && - comment[name_length] == '[' && - g_ascii_strncasecmp(comment + name_length + 1, - char_tnum, char_tnum_length) == 0 && - comment[name_length + char_tnum_length + 1] == ']') - name_length = name_length + char_tnum_length + 2; - else if (entry->length > name_length + char_tnum_length && - g_ascii_strncasecmp(comment + name_length, - char_tnum, char_tnum_length) == 0) - name_length = name_length + char_tnum_length; - } - if (comment[name_length] == '=') { - *length_r = entry->length - name_length - 1; - return comment + name_length + 1; - } + if (entry->length <= name_length || + g_ascii_strncasecmp(comment, name, name_length) != 0) + return NULL; + + if (char_tnum != NULL) { + char_tnum_length = strlen(char_tnum); + if (entry->length > name_length + char_tnum_length + 2 && + comment[name_length] == '[' && + g_ascii_strncasecmp(comment + name_length + 1, + char_tnum, char_tnum_length) == 0 && + comment[name_length + char_tnum_length + 1] == ']') + name_length = name_length + char_tnum_length + 2; + else if (entry->length > name_length + char_tnum_length && + g_ascii_strncasecmp(comment + name_length, + char_tnum, char_tnum_length) == 0) + name_length = name_length + char_tnum_length; + } + + if (comment[name_length] == '=') { + *length_r = entry->length - name_length - 1; + return comment + name_length + 1; } return NULL; -- cgit v1.2.3 From e44f31391234607ce0e95d69903142e71d61c813 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:51:35 +0200 Subject: update: free empty path string (memleak) When you pass an empty string to directory_update_init(), it was not freed by update_task(). --- src/update.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/update.c b/src/update.c index 1088f5338..9a2a2ceb9 100644 --- a/src/update.c +++ b/src/update.c @@ -767,7 +767,6 @@ static void * update_task(void *_path) { if (_path != NULL && !isRootDirectory(_path)) { updatePath((char *)_path); - g_free(_path); } else { struct directory *directory = db_get_root(); struct stat st; @@ -776,6 +775,8 @@ static void * update_task(void *_path) updateDirectory(directory, &st); } + g_free(_path); + if (modified || !db_exists()) db_save(); -- cgit v1.2.3 From 1c4f407a6db4c4795bbbc354f5cf311762fb8e33 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:51:42 +0200 Subject: decoder/flac: don't allocate cuesheet twice (memleak) The function flac_cue_track() first calls FLAC__metadata_object_new(), then overwrites this pointer with FLAC__metadata_get_cuesheet(). This allocate two FLAC__StreamMetadata objects, but the first pointer is lost, and never freed. --- src/decoder/_flac_common.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/decoder/_flac_common.c b/src/decoder/_flac_common.c index ae7d039ce..e096750f3 100644 --- a/src/decoder/_flac_common.c +++ b/src/decoder/_flac_common.c @@ -377,13 +377,15 @@ char* flac_cue_track( const char* pathname, const unsigned int tnum) { - FLAC__StreamMetadata* cs = FLAC__metadata_object_new(FLAC__METADATA_TYPE_CUESHEET); + FLAC__bool success; + FLAC__StreamMetadata* cs; - FLAC__metadata_get_cuesheet(pathname, &cs); - - if (cs == NULL) + success = FLAC__metadata_get_cuesheet(pathname, &cs); + if (!success) return NULL; + assert(cs != NULL); + if (cs->data.cue_sheet.num_tracks <= 1) { FLAC__metadata_object_delete(cs); -- cgit v1.2.3 From 5d6f7803e1059f527a70e541ed3945c19bd78c90 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:51:51 +0200 Subject: update: free temporary string in container scan (memleak) The return value of map_directory_child_fs() must be freed. --- src/update.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/update.c b/src/update.c index 9a2a2ceb9..bdf84ce36 100644 --- a/src/update.c +++ b/src/update.c @@ -430,7 +430,7 @@ update_container_file( struct directory* directory, { char* vtrack = NULL; unsigned int tnum = 0; - const char* pathname = map_directory_child_fs(directory, name); + char* pathname = map_directory_child_fs(directory, name); struct directory* contdir = dirvec_find(&directory->children, name); // directory exists already @@ -446,8 +446,10 @@ update_container_file( struct directory* directory, modified = true; } - else + else { + g_free(pathname); return true; + } } contdir = make_subdir(directory, name); @@ -473,6 +475,8 @@ update_container_file( struct directory* directory, g_free(vtrack); } + g_free(pathname); + if (tnum == 1) { delete_directory(contdir); -- cgit v1.2.3 From 7dddd9beda2bb0505758bb6a32cae6feb3215733 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:52:00 +0200 Subject: directory: free empty directories after removing them (memleak) dirvec_delete() does not free the object, we have to call directory_free() afterwards. --- src/directory.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/directory.c b/src/directory.c index 85c24fd04..ef8c038a3 100644 --- a/src/directory.c +++ b/src/directory.c @@ -73,9 +73,14 @@ directory_prune_empty(struct directory *directory) struct dirvec *dv = &directory->children; for (i = dv->nr; --i >= 0; ) { - directory_prune_empty(dv->base[i]); - if (directory_is_empty(dv->base[i])) - dirvec_delete(dv, dv->base[i]); + struct directory *child = dv->base[i]; + + directory_prune_empty(child); + + if (directory_is_empty(child)) { + dirvec_delete(dv, child); + directory_free(child); + } } if (!dv->nr) dirvec_destroy(dv); -- cgit v1.2.3 From 7133f560ec24c90671a40c9f9bc9cea6eb31cc17 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:52:12 +0200 Subject: output: fixed shout stuck pause bug Explicitly make the output thread leave the ao_pause() loop. This patch is a workaround, and the "pause" flag is not managed in a thread-safe way, but that's good enough for now. --- src/output_control.c | 11 +++++++++++ src/output_internal.h | 6 ++++++ src/output_thread.c | 3 +++ 3 files changed, 20 insertions(+) (limited to 'src') diff --git a/src/output_control.c b/src/output_control.c index eac9bdfcb..16c0dbb75 100644 --- a/src/output_control.c +++ b/src/output_control.c @@ -76,6 +76,17 @@ audio_output_open(struct audio_output *ao, audio_format_equals(audio_format, &ao->in_audio_format)) { assert(ao->pipe == mp); + if (ao->pause) { + /* unpause with the CANCEL command; this is a + hack, but suits well for forcing the thread + to leave the ao_pause() thread, and we need + to flush the device buffer anyway */ + + /* we're not using audio_output_cancel() here, + because that function is asynchronous */ + ao_command(ao, AO_COMMAND_CANCEL); + } + return true; } diff --git a/src/output_internal.h b/src/output_internal.h index 362d24947..72596c1c3 100644 --- a/src/output_internal.h +++ b/src/output_internal.h @@ -80,6 +80,12 @@ struct audio_output { */ bool open; + /** + * Is the device paused? i.e. the output thread is in the + * ao_pause() loop. + */ + bool pause; + /** * If not NULL, the device has failed, and this timer is used * to estimate how long it should stay disabled (unless diff --git a/src/output_thread.c b/src/output_thread.c index d414ba8d5..785ac808f 100644 --- a/src/output_thread.c +++ b/src/output_thread.c @@ -165,6 +165,7 @@ static void ao_pause(struct audio_output *ao) bool ret; ao_plugin_cancel(ao->plugin, ao->data); + ao->pause = true; ao_command_finished(ao); do { @@ -174,6 +175,8 @@ static void ao_pause(struct audio_output *ao) break; } } while (ao->command == AO_COMMAND_NONE); + + ao->pause = false; } static gpointer audio_output_task(gpointer arg) -- cgit v1.2.3 From f38ce5408b5d0126b8cfe730c91d5203ee59a987 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 14 Aug 2009 11:52:36 +0200 Subject: output/shout: minimize the unpause latency During the pause loop, manually sleep for 500ms if shout_delay() returns a value greater than that. Don't exhaust libshout's buffer. --- src/output/shout_plugin.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src') diff --git a/src/output/shout_plugin.c b/src/output/shout_plugin.c index 8e091679e..4412d26ff 100644 --- a/src/output/shout_plugin.c +++ b/src/output/shout_plugin.c @@ -448,8 +448,15 @@ my_shout_play(void *data, const void *chunk, size_t size, GError **error) static bool my_shout_pause(void *data) { + struct shout_data *sd = (struct shout_data *)data; static const char silence[1020]; + if (shout_delay(sd->shout_conn) > 500) { + /* cap the latency for unpause */ + g_usleep(500000); + return true; + } + return my_shout_play(data, silence, sizeof(silence), NULL); } -- cgit v1.2.3