[FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix type of value_len

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Thursday, 30 September 2021 04:59
> To: ffmpeg-devel at ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v5 3/7] libavformat/asfdec: Fix type
> of value_len
> 
> The value_len is an uint32 not an int32 per spec. That
> value must not be truncated, neither by casting to int, nor by any
> conditional checks, because at the end of get_tag, this value is
> needed to move forward in parsing. When the len value gets
> modified, the parsing may break.
> 
> Signed-off-by: softworkz <softworkz at hotmail.com>
> ---
> v5: Split into pieces as requested
> 
>  libavformat/asfdec_f.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> index 076b5ab147..d017fae019 100644
> --- a/libavformat/asfdec_f.c
> +++ b/libavformat/asfdec_f.c
> @@ -218,7 +218,7 @@ static uint64_t get_value(AVIOContext *pb, int
> type, int type2_size)
>      }
>  }
> 
> -static void get_tag(AVFormatContext *s, const char *key, int type,
> int len, int type2_size)
> +static void get_tag(AVFormatContext *s, const char *key, int type,
> uint32_t len, int type2_size)
>  {
>      ASFContext *asf = s->priv_data;
>      char *value = NULL;
> @@ -528,7 +528,7 @@ static int
> asf_read_ext_stream_properties(AVFormatContext *s, int64_t size)
>  static int asf_read_content_desc(AVFormatContext *s, int64_t size)
>  {
>      AVIOContext *pb = s->pb;
> -    int len1, len2, len3, len4, len5;
> +    uint32_t len1, len2, len3, len4, len5;
> 
>      len1 = avio_rl16(pb);
>      len2 = avio_rl16(pb);
> @@ -602,25 +602,23 @@ static int asf_read_metadata(AVFormatContext
> *s, int64_t size)
>  {
>      AVIOContext *pb = s->pb;
>      ASFContext *asf = s->priv_data;
> -    int n, stream_num, name_len_utf16, name_len_utf8, value_len;
> +    int n, name_len_utf8;
> +    uint16_t stream_num, name_len_utf16, value_type;
> +    uint32_t value_len;
>      int ret, i;
>      n = avio_rl16(pb);
> 
>      for (i = 0; i < n; i++) {
>          uint8_t *name;
> -        int value_type;
> 
>          avio_rl16(pb);  // lang_list_index
> -        stream_num = avio_rl16(pb);
> -        name_len_utf16 = avio_rl16(pb);
> -        value_type = avio_rl16(pb); /* value_type */
> -        value_len  = avio_rl32(pb);
> +        stream_num     = (uint16_t)avio_rl16(pb);
> +        name_len_utf16 = (uint16_t)avio_rl16(pb);
> +        value_type     = (uint16_t)avio_rl16(pb); /* value_type */
> +        value_len      = avio_rl32(pb);
> 
> -        if (value_len < 0 || value_len > UINT16_MAX)
> -            return AVERROR_INVALIDDATA;

The previous two lines, which were added about a year ago, are actually 
the origin of this patchset:

A while ago after rebasing, I noticed that album image extraction from 
.wma didn't work anymore in certain cases, namely: 
image attachments > 32kB

I found those two lines and wondered, checked against the ASF spec and 
noticed the other flaws.

Regarding those two lines, I would suppose that they might have slipped in 
accidentally - at least the check against UINT_16_MAX.


Kind regards,
softworkz