diff mbox series

[FFmpeg-devel,10/14] h264_sei: use a separate reader for the individual SEI messages

Message ID 20200327125747.13460-10-anton@khirnov.net
State Accepted
Commit 1e9615c5d4d1697816037057133b20da4fd9e57b
Headers show
Series [FFmpeg-devel,01/14] mpeg4videodec: do not copy a range of fields at once | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov March 27, 2020, 12:57 p.m. UTC 
This tells the parsing functions the payload size and prevents them from
overreading.
---
 libavcodec/h264_sei.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

James Almer March 27, 2020, 3:08 p.m. UTC | #1 
On 3/27/2020 9:57 AM, Anton Khirnov wrote:
> This tells the parsing functions the payload size and prevents them from
> overreading.
> ---
>  libavcodec/h264_sei.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> index a565feabe2..32d13985f3 100644
> --- a/libavcodec/h264_sei.c
> +++ b/libavcodec/h264_sei.c
> @@ -407,9 +407,9 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>      int master_ret = 0;
>  
>      while (get_bits_left(gb) > 16 && show_bits(gb, 16)) {
> +        GetBitContext gb_payload;
>          int type = 0;
>          unsigned size = 0;
> -        unsigned next;
>          int ret  = 0;
>  
>          do {
> @@ -429,35 +429,38 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>                     type, 8*size, get_bits_left(gb));
>              return AVERROR_INVALIDDATA;
>          }
> -        next = get_bits_count(gb) + 8 * size;
> +
> +        ret = init_get_bits8(&gb_payload, gb->buffer + get_bits_count(gb) / 8, size);

Could use init_get_bits() instead, since size was already checked above.

> +        if (ret < 0)
> +            return ret;
>  
>          switch (type) {
>          case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
> -            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
> +            ret = decode_picture_timing(&h->picture_timing, &gb_payload, ps, logctx);
>              break;
>          case H264_SEI_TYPE_USER_DATA_REGISTERED:
> -            ret = decode_registered_user_data(h, gb, logctx, size);
> +            ret = decode_registered_user_data(h, &gb_payload, logctx, size);
>              break;
>          case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
> -            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
> +            ret = decode_unregistered_user_data(&h->unregistered, &gb_payload, logctx, size);
>              break;
>          case H264_SEI_TYPE_RECOVERY_POINT:
> -            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
> +            ret = decode_recovery_point(&h->recovery_point, &gb_payload, logctx);
>              break;
>          case H264_SEI_TYPE_BUFFERING_PERIOD:
> -            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
> +            ret = decode_buffering_period(&h->buffering_period, &gb_payload, ps, logctx);
>              break;
>          case H264_SEI_TYPE_FRAME_PACKING:
> -            ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
> +            ret = decode_frame_packing_arrangement(&h->frame_packing, &gb_payload);
>              break;
>          case H264_SEI_TYPE_DISPLAY_ORIENTATION:
> -            ret = decode_display_orientation(&h->display_orientation, gb);
> +            ret = decode_display_orientation(&h->display_orientation, &gb_payload);
>              break;
>          case H264_SEI_TYPE_GREEN_METADATA:
> -            ret = decode_green_metadata(&h->green_metadata, gb);
> +            ret = decode_green_metadata(&h->green_metadata, &gb_payload);
>              break;
>          case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
> -            ret = decode_alternative_transfer(&h->alternative_transfer, gb);
> +            ret = decode_alternative_transfer(&h->alternative_transfer, &gb_payload);
>              break;
>          default:
>              av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
> @@ -467,10 +470,12 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>          if (ret < 0)
>              master_ret = ret;
>  
> -        skip_bits_long(gb, next - get_bits_count(gb));
> +        if (get_bits_left(&gb_payload) < 0) {
> +            av_log(logctx, AV_LOG_WARNING, "SEI type %d overread by %d bits\n",
> +                   type, -get_bits_left(&gb_payload));

Maybe check for explode and abort?

> +        }
>  
> -        // FIXME check bits here
> -        align_get_bits(gb);
> +        skip_bits_long(gb, 8 * size);
>      }
>  
>      return master_ret;
> 

LGTM
Anton Khirnov April 7, 2020, 1:57 p.m. UTC | #2 
Quoting James Almer (2020-03-27 16:08:11)
> On 3/27/2020 9:57 AM, Anton Khirnov wrote:
> > This tells the parsing functions the payload size and prevents them from
> > overreading.
> > ---
> >  libavcodec/h264_sei.c | 33 +++++++++++++++++++--------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
> > index a565feabe2..32d13985f3 100644
> > --- a/libavcodec/h264_sei.c
> > +++ b/libavcodec/h264_sei.c
> > @@ -407,9 +407,9 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> >      int master_ret = 0;
> >  
> >      while (get_bits_left(gb) > 16 && show_bits(gb, 16)) {
> > +        GetBitContext gb_payload;
> >          int type = 0;
> >          unsigned size = 0;
> > -        unsigned next;
> >          int ret  = 0;
> >  
> >          do {
> > @@ -429,35 +429,38 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> >                     type, 8*size, get_bits_left(gb));
> >              return AVERROR_INVALIDDATA;
> >          }
> > -        next = get_bits_count(gb) + 8 * size;
> > +
> > +        ret = init_get_bits8(&gb_payload, gb->buffer + get_bits_count(gb) / 8, size);
> 
> Could use init_get_bits() instead, since size was already checked above.

ok, done locally

> 
> > +        if (ret < 0)
> > +            return ret;
> >  
> >          switch (type) {
> >          case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
> > -            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
> > +            ret = decode_picture_timing(&h->picture_timing, &gb_payload, ps, logctx);
> >              break;
> >          case H264_SEI_TYPE_USER_DATA_REGISTERED:
> > -            ret = decode_registered_user_data(h, gb, logctx, size);
> > +            ret = decode_registered_user_data(h, &gb_payload, logctx, size);
> >              break;
> >          case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
> > -            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
> > +            ret = decode_unregistered_user_data(&h->unregistered, &gb_payload, logctx, size);
> >              break;
> >          case H264_SEI_TYPE_RECOVERY_POINT:
> > -            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
> > +            ret = decode_recovery_point(&h->recovery_point, &gb_payload, logctx);
> >              break;
> >          case H264_SEI_TYPE_BUFFERING_PERIOD:
> > -            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
> > +            ret = decode_buffering_period(&h->buffering_period, &gb_payload, ps, logctx);
> >              break;
> >          case H264_SEI_TYPE_FRAME_PACKING:
> > -            ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
> > +            ret = decode_frame_packing_arrangement(&h->frame_packing, &gb_payload);
> >              break;
> >          case H264_SEI_TYPE_DISPLAY_ORIENTATION:
> > -            ret = decode_display_orientation(&h->display_orientation, gb);
> > +            ret = decode_display_orientation(&h->display_orientation, &gb_payload);
> >              break;
> >          case H264_SEI_TYPE_GREEN_METADATA:
> > -            ret = decode_green_metadata(&h->green_metadata, gb);
> > +            ret = decode_green_metadata(&h->green_metadata, &gb_payload);
> >              break;
> >          case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
> > -            ret = decode_alternative_transfer(&h->alternative_transfer, gb);
> > +            ret = decode_alternative_transfer(&h->alternative_transfer, &gb_payload);
> >              break;
> >          default:
> >              av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
> > @@ -467,10 +470,12 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
> >          if (ret < 0)
> >              master_ret = ret;
> >  
> > -        skip_bits_long(gb, next - get_bits_count(gb));
> > +        if (get_bits_left(&gb_payload) < 0) {
> > +            av_log(logctx, AV_LOG_WARNING, "SEI type %d overread by %d bits\n",
> > +                   type, -get_bits_left(&gb_payload));
> 
> Maybe check for explode and abort?

We don't have access to explode, so it'd have to be passed in
explicitly. That seemed like too much effort to be worth it.
James Almer April 7, 2020, 1:59 p.m. UTC | #3 
On 4/7/2020 10:57 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-03-27 16:08:11)
>> On 3/27/2020 9:57 AM, Anton Khirnov wrote:
>>> This tells the parsing functions the payload size and prevents them from
>>> overreading.
>>> ---
>>>  libavcodec/h264_sei.c | 33 +++++++++++++++++++--------------
>>>  1 file changed, 19 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
>>> index a565feabe2..32d13985f3 100644
>>> --- a/libavcodec/h264_sei.c
>>> +++ b/libavcodec/h264_sei.c
>>> @@ -407,9 +407,9 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>>>      int master_ret = 0;
>>>  
>>>      while (get_bits_left(gb) > 16 && show_bits(gb, 16)) {
>>> +        GetBitContext gb_payload;
>>>          int type = 0;
>>>          unsigned size = 0;
>>> -        unsigned next;
>>>          int ret  = 0;
>>>  
>>>          do {
>>> @@ -429,35 +429,38 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>>>                     type, 8*size, get_bits_left(gb));
>>>              return AVERROR_INVALIDDATA;
>>>          }
>>> -        next = get_bits_count(gb) + 8 * size;
>>> +
>>> +        ret = init_get_bits8(&gb_payload, gb->buffer + get_bits_count(gb) / 8, size);
>>
>> Could use init_get_bits() instead, since size was already checked above.
> 
> ok, done locally
> 
>>
>>> +        if (ret < 0)
>>> +            return ret;
>>>  
>>>          switch (type) {
>>>          case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
>>> -            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
>>> +            ret = decode_picture_timing(&h->picture_timing, &gb_payload, ps, logctx);
>>>              break;
>>>          case H264_SEI_TYPE_USER_DATA_REGISTERED:
>>> -            ret = decode_registered_user_data(h, gb, logctx, size);
>>> +            ret = decode_registered_user_data(h, &gb_payload, logctx, size);
>>>              break;
>>>          case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
>>> -            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
>>> +            ret = decode_unregistered_user_data(&h->unregistered, &gb_payload, logctx, size);
>>>              break;
>>>          case H264_SEI_TYPE_RECOVERY_POINT:
>>> -            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
>>> +            ret = decode_recovery_point(&h->recovery_point, &gb_payload, logctx);
>>>              break;
>>>          case H264_SEI_TYPE_BUFFERING_PERIOD:
>>> -            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
>>> +            ret = decode_buffering_period(&h->buffering_period, &gb_payload, ps, logctx);
>>>              break;
>>>          case H264_SEI_TYPE_FRAME_PACKING:
>>> -            ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
>>> +            ret = decode_frame_packing_arrangement(&h->frame_packing, &gb_payload);
>>>              break;
>>>          case H264_SEI_TYPE_DISPLAY_ORIENTATION:
>>> -            ret = decode_display_orientation(&h->display_orientation, gb);
>>> +            ret = decode_display_orientation(&h->display_orientation, &gb_payload);
>>>              break;
>>>          case H264_SEI_TYPE_GREEN_METADATA:
>>> -            ret = decode_green_metadata(&h->green_metadata, gb);
>>> +            ret = decode_green_metadata(&h->green_metadata, &gb_payload);
>>>              break;
>>>          case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
>>> -            ret = decode_alternative_transfer(&h->alternative_transfer, gb);
>>> +            ret = decode_alternative_transfer(&h->alternative_transfer, &gb_payload);
>>>              break;
>>>          default:
>>>              av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
>>> @@ -467,10 +470,12 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
>>>          if (ret < 0)
>>>              master_ret = ret;
>>>  
>>> -        skip_bits_long(gb, next - get_bits_count(gb));
>>> +        if (get_bits_left(&gb_payload) < 0) {
>>> +            av_log(logctx, AV_LOG_WARNING, "SEI type %d overread by %d bits\n",
>>> +                   type, -get_bits_left(&gb_payload));
>>
>> Maybe check for explode and abort?
> 
> We don't have access to explode, so it'd have to be passed in
> explicitly. That seemed like too much effort to be worth it.

Ah, true. LGTM then.
diff mbox series

Patch

diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c
index a565feabe2..32d13985f3 100644
--- a/libavcodec/h264_sei.c
+++ b/libavcodec/h264_sei.c
@@ -407,9 +407,9 @@  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
     int master_ret = 0;
 
     while (get_bits_left(gb) > 16 && show_bits(gb, 16)) {
+        GetBitContext gb_payload;
         int type = 0;
         unsigned size = 0;
-        unsigned next;
         int ret  = 0;
 
         do {
@@ -429,35 +429,38 @@  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
                    type, 8*size, get_bits_left(gb));
             return AVERROR_INVALIDDATA;
         }
-        next = get_bits_count(gb) + 8 * size;
+
+        ret = init_get_bits8(&gb_payload, gb->buffer + get_bits_count(gb) / 8, size);
+        if (ret < 0)
+            return ret;
 
         switch (type) {
         case H264_SEI_TYPE_PIC_TIMING: // Picture timing SEI
-            ret = decode_picture_timing(&h->picture_timing, gb, ps, logctx);
+            ret = decode_picture_timing(&h->picture_timing, &gb_payload, ps, logctx);
             break;
         case H264_SEI_TYPE_USER_DATA_REGISTERED:
-            ret = decode_registered_user_data(h, gb, logctx, size);
+            ret = decode_registered_user_data(h, &gb_payload, logctx, size);
             break;
         case H264_SEI_TYPE_USER_DATA_UNREGISTERED:
-            ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size);
+            ret = decode_unregistered_user_data(&h->unregistered, &gb_payload, logctx, size);
             break;
         case H264_SEI_TYPE_RECOVERY_POINT:
-            ret = decode_recovery_point(&h->recovery_point, gb, logctx);
+            ret = decode_recovery_point(&h->recovery_point, &gb_payload, logctx);
             break;
         case H264_SEI_TYPE_BUFFERING_PERIOD:
-            ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
+            ret = decode_buffering_period(&h->buffering_period, &gb_payload, ps, logctx);
             break;
         case H264_SEI_TYPE_FRAME_PACKING:
-            ret = decode_frame_packing_arrangement(&h->frame_packing, gb);
+            ret = decode_frame_packing_arrangement(&h->frame_packing, &gb_payload);
             break;
         case H264_SEI_TYPE_DISPLAY_ORIENTATION:
-            ret = decode_display_orientation(&h->display_orientation, gb);
+            ret = decode_display_orientation(&h->display_orientation, &gb_payload);
             break;
         case H264_SEI_TYPE_GREEN_METADATA:
-            ret = decode_green_metadata(&h->green_metadata, gb);
+            ret = decode_green_metadata(&h->green_metadata, &gb_payload);
             break;
         case H264_SEI_TYPE_ALTERNATIVE_TRANSFER:
-            ret = decode_alternative_transfer(&h->alternative_transfer, gb);
+            ret = decode_alternative_transfer(&h->alternative_transfer, &gb_payload);
             break;
         default:
             av_log(logctx, AV_LOG_DEBUG, "unknown SEI type %d\n", type);
@@ -467,10 +470,12 @@  int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb,
         if (ret < 0)
             master_ret = ret;
 
-        skip_bits_long(gb, next - get_bits_count(gb));
+        if (get_bits_left(&gb_payload) < 0) {
+            av_log(logctx, AV_LOG_WARNING, "SEI type %d overread by %d bits\n",
+                   type, -get_bits_left(&gb_payload));
+        }
 
-        // FIXME check bits here
-        align_get_bits(gb);
+        skip_bits_long(gb, 8 * size);
     }
 
     return master_ret;