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

[James Almer]

On 9/29/2021 11:58 PM, Soft Works wrote:
> 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)

There's an av_assert0() in this function to ensure len will never be 
bigger than (INT_MAX - 22) / 2. And len is used as argument for 
avio_read(), which takes an int, not an unsigned int. So this change is 
not ok.

>   {
>       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;
> -
> -        name_len_utf8 = 2*name_len_utf16 + 1;
> -        name          = av_malloc(name_len_utf8);
> +        name_len_utf8  = 2 * name_len_utf16 + 1;
> +        name           = av_malloc(name_len_utf8);

Unrelated cosmetics.

>           if (!name)
>               return AVERROR(ENOMEM);
>   
>