diff mbox series

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

Message ID DB6PR0101MB22144042E48FA4A5086D58648FB29@DB6PR0101MB2214.eurprd01.prod.exchangelabs.com
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/h2645_parse: Only trim RBSP trailing padding if it exists | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Andreas Rheinhardt June 22, 2022, 10:53 a.m. UTC
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@outlook.com>
---
 libavcodec/h2645_parse.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Andreas Rheinhardt June 24, 2022, 9:53 a.m. UTC | #1
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@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
diff mbox series

Patch

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;