[FFmpeg-devel] [PATCH] avcodec/h2645_parse: Only trim RBSP trailing padding if it exists

[Andreas Rheinhardt]

Andreas Rheinhardt:
> It does not exist for NALUs for which the SODB is empty;
> it also does not exist for NALUs for which not even
> the complete header is present. The former category contains
> end of sequence and end of bitstream units. The latter category
> consists of one-byte HEVC units (the ordinary H.264 header is only
> one byte long).
> This commit therefore stops stripping RBSP trailing padding
> from the former type of unit and discards the latter type of unit
> altogether.
> 
> This also fixes an assertion failure: Before this commit, a one-byte
> HEVC NALU from an ISOBMFF packet could pass all the checks in
> hevc_parse_nal_header() (because the first byte of the size field
> of the next unit is mistaken as containing the temporal_id);
> yet because the trailing padding bits were stripped, its actually
> had a size of less than eight bits; because h2645_parse.c uses
> the checked bitstream reader, the get_bits_count() of the GetBitContext
> is not 16 in this case; it is not even a multiple of eight
> and this can trigger an assert in ff_hevc_decode_nal_sei().
> 
> Fixes: Assertion failure
> Fixes: 46662/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-4947860854013952
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>  libavcodec/h2645_parse.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index 03780680c6..dca91b24f3 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -259,10 +259,10 @@ static const char *h264_nal_unit_name(int nal_type)
>      return h264_nal_type_name[nal_type];
>  }
>  
> -static int get_bit_length(H2645NAL *nal, int skip_trailing_zeros)
> +static int get_bit_length(H2645NAL *nal, int min_size, int skip_trailing_zeros)
>  {
>      int size = nal->size;
> -    int v;
> +    int trailing_padding = 0;
>  
>      while (skip_trailing_zeros && size > 0 && nal->data[size - 1] == 0)
>          size--;
> @@ -270,18 +270,23 @@ static int get_bit_length(H2645NAL *nal, int skip_trailing_zeros)
>      if (!size)
>          return 0;
>  
> -    v = nal->data[size - 1];
> +    if (size <= min_size) {
> +        if (nal->size < min_size)
> +            return AVERROR_INVALIDDATA;
> +        size = min_size;
> +    } else {
> +        int v = nal->data[size - 1];
> +        /* remove the stop bit and following trailing zeros,
> +        * or nothing for damaged bitstreams */
> +        if (v)
> +            trailing_padding = ff_ctz(v) + 1;
> +    }
>  
>      if (size > INT_MAX / 8)
>          return AVERROR(ERANGE);
>      size *= 8;
>  
> -    /* remove the stop bit and following trailing zeros,
> -     * or nothing for damaged bitstreams */
> -    if (v)
> -        size -= ff_ctz(v) + 1;
> -
> -    return size;
> +    return size - trailing_padding;
>  }
>  
>  /**
> @@ -491,7 +496,8 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>              bytestream2_peek_be32(&bc) == 0x000001E0)
>              skip_trailing_zeros = 0;
>  
> -        nal->size_bits = get_bit_length(nal, skip_trailing_zeros);
> +        nal->size_bits = get_bit_length(nal, 1 + (codec_id == AV_CODEC_ID_HEVC),
> +                                        skip_trailing_zeros);
>  
>          if (nal->size <= 0 || nal->size_bits <= 0)
>              continue;

Given that Michael has confirmed that it works (see
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297978.html) I will
apply this tonight unless there are objections.

- Andreas