Victor Vasiliev <[email protected]> writes:
> ---
> libavformat/wav.c | 70
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 69 insertions(+), 1 deletions(-)
In addition to what has already been said:
> +static int wav_parse_info_tags(AVFormatContext *s, int64_t size,
> AVStream **stream)
This line and a few others are mangled by pasting into gmail.
If using gmail, please send patches as attachments.
> +{
> + int64_t start, end, cur;
> + AVIOContext *pb = s->pb;
> +
> + start = avio_tell(pb);
> + end = start + size;
The size should possibly be sanity checked somehow.
> + while ( (cur = avio_tell(pb)) < end) {
Please, no spaces immediately inside parens, here or elsewhere.
> + unsigned int chunk_code;
chunk_code is used in a manner requiring it to be exactly 32 bits wide,
so it would be better to declare it as uint32_t. This does make it not
match the next_tag() prototype, which also really ought to be changed to
take a pointer to uint32_t. Changing the two places it is currently used
is not much effort.
> + int64_t chunk_size;
> + char *key, *value;
> +
> + chunk_size = next_tag(pb, &chunk_code);
> + if (chunk_size > end || end - chunk_size < cur) {
This is not a sufficient check since 'end' is based on untrusted data.
> + av_log(s, AV_LOG_ERROR, "too big INFO subchunk\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + key = av_malloc(4 + 1);
> + value = av_malloc(chunk_size + 1);
chunk_size should be sanity checked here to ensure adding 1 doesn't make
it overflow. Since the total size is also untrusted, comparing against
the end does not guarantee safety.
It might even be a good idea to set a fairly low limit and consider
anything above it an error (due to corrupt or malicious file).
> + if (!key || !value) {
> + av_log(s, AV_LOG_ERROR, "out of memory, unable to read
> INFO tag\n");
> + return AVERROR(ENOMEM);
> + }
> +
> +#ifdef HAVE_BIG_ENDIAN
> + chunk_code = av_bswap32(chunk_code);
> +#endif
av_le2ne32(chunk_code)
> + snprintf( key, 5, "%4.4s", &chunk_code );
Is this guaranteed not to mess up if the code contains non-printable
characters?
> + if (avio_read(pb, value, chunk_size) != chunk_size) {
> + av_freep(key);
> + av_freep(value);
> + av_log(s, AV_LOG_ERROR, "premature end of file while
> reading INFO tag\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + key[4] = '\0';
snprintf() will have already 0-terminated the string.
> + value[chunk_size] = '\0';
I prefer to use a plain, unadorned 0 in cases like this. The quotes and
backslash add little value (both have type int).
> + av_dict_set(&s->metadata, key, value, AV_DICT_DONT_STRDUP_VAL);
> + }
This will leak the memory for key; av_dict_set() makes a copy by default.
Simpler would be to declare it as char key[5], drop the malloc here, and
let av_dict_set() do what it does.
> + ff_metadata_conv_ctx(s, NULL, wav_info_tag_conv);
> +
> + return 0;
> +}
> +
> /* wav input */
> static int wav_read_header(AVFormatContext *s,
> AVFormatParameters *ap)
> @@ -385,7 +445,7 @@ static int wav_read_header(AVFormatContext *s,
> int64_t size, av_uninit(data_size);
> int64_t sample_count=0;
> int rf64;
> - unsigned int tag;
> + unsigned int tag, list_type;
> AVIOContext *pb = s->pb;
> AVStream *st;
> WAVContext *wav = s->priv_data;
> @@ -467,6 +527,14 @@ static int wav_read_header(AVFormatContext *s,
> if ((ret = wav_parse_bext_tag(s, size)) < 0)
> return ret;
> break;
> + case MKTAG('L', 'I', 'S', 'T'):
size should be checked to be at least 4 here.
> + list_type = avio_rl32(pb);
> + switch (list_type) {
> + case MKTAG('I', 'N', 'F', 'O'):
> + if ((ret = wav_parse_info_tags(s, size - 4, &st)) < 0)
> + return ret;
> + }
> + break;
> }
>
> /* seek to next tag unless we know that we'll run into EOF */
> --
> 1.7.5.4
>
--
Måns Rullgård
[email protected]
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
