[FFmpeg-devel] [PATCH v1] libavcodec/flac_parser: Validate subframe zero bit and type

[Mattias Wadman]

On Wed, Oct 13, 2021 at 6:15 PM Mattias Wadman <mattias.wadman at gmail.com>
wrote:

> Reduces the risk of finding false frames that happens to have valid values
> and CRC.
>
> Fixes ticket #9185 ffmpeg flac decoder incorrectly finds junk frame
> https://trac.ffmpeg.org/ticket/9185
> ---
>  libavcodec/flac_parser.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index d3d9c889a1..2c550507fc 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -96,8 +96,34 @@ static int frame_header_is_valid(AVCodecContext *avctx,
> const uint8_t *buf,
>                                   FLACFrameInfo *fi)
>  {
>      GetBitContext gb;
> -    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8);
> -    return !ff_flac_decode_frame_header(avctx, &gb, fi, 127);
> +    uint8_t subframe_type;
> +
> +    // header plus one byte from first subframe
> +    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE * 8 + 8);
> +    if (ff_flac_decode_frame_header(avctx, &gb, fi, 127)) {
> +        return 0;
> +    }
> +    // subframe zero bit
> +    if (get_bits1(&gb) != 0) {
> +        return 0;
> +    }
> +    // subframe type
> +    // 000000 : SUBFRAME_CONSTANT
> +    // 000001 : SUBFRAME_VERBATIM
> +    // 00001x : reserved
> +    // 0001xx : reserved
> +    // 001xxx : if(xxx <= 4) SUBFRAME_FIXED, xxx=order ; else reserved
> +    // 01xxxx : reserved
> +    // 1xxxxx : SUBFRAME_LPC, xxxxx=order-1
> +    subframe_type = get_bits(&gb, 6);
> +    if (!(subframe_type == 0 ||
> +          subframe_type == 1 ||
> +          ((subframe_type >= 8) && (subframe_type <= 12)) ||
> +          (subframe_type >= 32))) {
> +        return 0;
> +    }
> +
> +    return 1;
>  }
>
>  /**
> --
> 2.29.2
>
>
Ping

Maybe I should give some more context. I originally reported
https://trac.ffmpeg.org/ticket/9185 after seeing decode errors 1-2 times a
day in a system that decodes tens of thousands of FLAC files per day. All
of the failing ones I've looked at decodes fine with the FLAC reference
decoder and nearly all of them are encoded using "Switch Audio File
Converter", but I've managed to make ffmpeg's FLAC encoder produce valid
files that it fails on also.

With the patch I haven't seen any errors for the last 2 weeks and I've also
gone through older files and all have decoded fine.

I've looked into solving this without adding more validation in the flac
parser but failed. I can't see how to know the frame size without more or
less decoding it.

-Mattias