Erik de Castro Lopo
2014-Sep-26 12:21 UTC
[flac-dev] Patch to improve malformed vorbiscomment handling
Janne Hyv?rinen wrote:> Patch v2, now handles more malformed cases. Original patch was for a > file for which I had a sample from a user but this allows handling some > manually broken test cases.Err, I'm getting warning messages on that patch: CC metadata_iterators.lo metadata_iterators.c: In function ?read_metadata_block_data_vorbis_comment_cb_?: metadata_iterators.c:2238:12: error: comparison between ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? [-Werror=enum-compare] if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) goto skip; ^ metadata_iterators.c:2256:13: error: comparison between ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? [-Werror=enum-compare] if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) { ^ Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Janne Hyvärinen
2014-Sep-26 12:47 UTC
[flac-dev] Patch to improve malformed vorbiscomment handling
On 26.9.2014 15:21, Erik de Castro Lopo wrote:> Janne Hyv?rinen wrote: > >> Patch v2, now handles more malformed cases. Original patch was for a >> file for which I had a sample from a user but this allows handling some >> manually broken test cases. > > Err, I'm getting warning messages on that patch: > > CC metadata_iterators.lo > metadata_iterators.c: In function ?read_metadata_block_data_vorbis_comment_cb_?: > metadata_iterators.c:2238:12: error: comparison between ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? [-Werror=enum-compare] > if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) goto skip; > ^ > metadata_iterators.c:2256:13: error: comparison between ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? [-Werror=enum-compare] > if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) { > ^ > > > > ErikDarn, MSVC didn't warn anything and I didn't realize there are so many BAD_METADATA statuses. I picked the first one autocomplete offered. One thing I don't like is that these don't return any warning messages to tools. But I didn't see a way for the library to do that. I see only fatal errors. -------------- next part -------------- diff --git a/src/libFLAC/metadata_iterators.c b/src/libFLAC/metadata_iterators.c index d50df39..52ce7de 100644 --- a/src/libFLAC/metadata_iterators.c +++ b/src/libFLAC/metadata_iterators.c @@ -77,8 +77,8 @@ static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_streaminfo_c static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_padding_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Seek seek_cb, FLAC__StreamMetadata_Padding *block, unsigned block_length); static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_application_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_Application *block, unsigned block_length); static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_seektable_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_SeekTable *block, unsigned block_length); -static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_entry_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment_Entry *entry); -static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment *block); +static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_entry_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment_Entry *entry, unsigned max_length); +static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__IOCallback_Seek seek_cb, FLAC__StreamMetadata_VorbisComment *block, unsigned block_length); static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_cuesheet_track_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_CueSheet_Track *track); static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_cuesheet_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_CueSheet *block); static FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_picture_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_Picture *block); @@ -2091,7 +2091,7 @@ FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_cb_(FLAC__IOHandle case FLAC__METADATA_TYPE_SEEKTABLE: return read_metadata_block_data_seektable_cb_(handle, read_cb, &block->data.seek_table, block->length); case FLAC__METADATA_TYPE_VORBIS_COMMENT: - return read_metadata_block_data_vorbis_comment_cb_(handle, read_cb, &block->data.vorbis_comment); + return read_metadata_block_data_vorbis_comment_cb_(handle, read_cb, seek_cb, &block->data.vorbis_comment, block->length); case FLAC__METADATA_TYPE_CUESHEET: return read_metadata_block_data_cuesheet_cb_(handle, read_cb, &block->data.cue_sheet); case FLAC__METADATA_TYPE_PICTURE: @@ -2189,16 +2189,21 @@ FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_seektable_cb_(FLAC_ return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK; } -FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_entry_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment_Entry *entry) +FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_entry_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment_Entry *entry, unsigned max_length) { const unsigned entry_length_len = FLAC__STREAM_METADATA_VORBIS_COMMENT_ENTRY_LENGTH_LEN / 8; FLAC__byte buffer[4]; /* magic number is asserted below */ FLAC__ASSERT(FLAC__STREAM_METADATA_VORBIS_COMMENT_ENTRY_LENGTH_LEN / 8 == sizeof(buffer)); + if(max_length < entry_length_len) return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_BAD_METADATA; else max_length -= entry_length_len; if(read_cb(buffer, 1, entry_length_len, handle) != entry_length_len) return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_READ_ERROR; entry->length = unpack_uint32_little_endian_(buffer, entry_length_len); + if(max_length < entry->length) { + entry->length = 0; + return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_BAD_METADATA; + } else max_length -= entry->length; if(0 != entry->entry) free(entry->entry); @@ -2219,7 +2224,7 @@ FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_entr return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK; } -FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__StreamMetadata_VorbisComment *block) +FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_(FLAC__IOHandle handle, FLAC__IOCallback_Read read_cb, FLAC__IOCallback_Seek seek_cb, FLAC__StreamMetadata_VorbisComment *block, unsigned block_length) { unsigned i; FLAC__Metadata_SimpleIteratorStatus status; @@ -2228,9 +2233,13 @@ FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_( FLAC__ASSERT(FLAC__STREAM_METADATA_VORBIS_COMMENT_NUM_COMMENTS_LEN / 8 == sizeof(buffer)); - if(FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK != (status = read_metadata_block_data_vorbis_comment_entry_cb_(handle, read_cb, &(block->vendor_string)))) - return status; + status = read_metadata_block_data_vorbis_comment_entry_cb_(handle, read_cb, &(block->vendor_string), block_length); + if(block_length >= 4) block_length -= 4; + if(status == FLAC__METADATA_SIMPLE_ITERATOR_STATUS_BAD_METADATA) goto skip; + else if(status != FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK) return status; + block_length -= block->vendor_string.length; + if(block_length < num_comments_len) goto skip; else block_length -= num_comments_len; if(read_cb(buffer, 1, num_comments_len, handle) != num_comments_len) return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_READ_ERROR; block->num_comments = unpack_uint32_little_endian_(buffer, num_comments_len); @@ -2242,8 +2251,21 @@ FLAC__Metadata_SimpleIteratorStatus read_metadata_block_data_vorbis_comment_cb_( return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_MEMORY_ALLOCATION_ERROR; for(i = 0; i < block->num_comments; i++) { - if(FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK != (status = read_metadata_block_data_vorbis_comment_entry_cb_(handle, read_cb, block->comments + i))) - return status; + status = read_metadata_block_data_vorbis_comment_entry_cb_(handle, read_cb, block->comments + i, block_length); + if(block_length >= 4) block_length -= 4; + if(status == FLAC__METADATA_SIMPLE_ITERATOR_STATUS_BAD_METADATA) { + block->num_comments = i; + goto skip; + } + else if(status != FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK) return status; + block_length -= block->comments[i].length; + } + + skip: + if(block_length > 0) { + /* bad metadata */ + if(0 != seek_cb(handle, block_length, SEEK_CUR)) + return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_SEEK_ERROR; } return FLAC__METADATA_SIMPLE_ITERATOR_STATUS_OK;
Janne Hyvärinen
2014-Sep-26 12:51 UTC
[flac-dev] Patch to improve malformed vorbiscomment handling
Sorry for spamming but in the haste I forgot the stream_decoder part out. On 26.9.2014 15:47, Janne Hyv?rinen wrote:> On 26.9.2014 15:21, Erik de Castro Lopo wrote: >> Janne Hyv?rinen wrote: >> >>> Patch v2, now handles more malformed cases. Original patch was for a >>> file for which I had a sample from a user but this allows handling some >>> manually broken test cases. >> >> Err, I'm getting warning messages on that patch: >> >> CC metadata_iterators.lo >> metadata_iterators.c: In function >> ?read_metadata_block_data_vorbis_comment_cb_?: >> metadata_iterators.c:2238:12: error: comparison between >> ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? >> [-Werror=enum-compare] >> if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) goto skip; >> ^ >> metadata_iterators.c:2256:13: error: comparison between >> ?FLAC__Metadata_SimpleIteratorStatus? and ?enum <anonymous>? >> [-Werror=enum-compare] >> if(status == FLAC__METADATA_CHAIN_STATUS_BAD_METADATA) { >> ^ >> >> >> >> Erik > > Darn, MSVC didn't warn anything and I didn't realize there are so many > BAD_METADATA statuses. I picked the first one autocomplete offered. > > One thing I don't like is that these don't return any warning messages > to tools. But I didn't see a way for the library to do that. I see > only fatal errors. > > > > _______________________________________________ > flac-dev mailing list > flac-dev at xiph.org > http://lists.xiph.org/mailman/listinfo/flac-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.xiph.org/pipermail/flac-dev/attachments/20140926/92f868f2/attachment.htm -------------- next part -------------- A non-text attachment was scrubbed... Name: improve_malformed_comment_handling4.zip Type: application/x-zip-compressed Size: 2176 bytes Desc: not available Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20140926/92f868f2/attachment.bin