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

[James Almer]

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.