[FFmpeg-devel] [PATCH 4/7] avformat/vvc: Fix crash on allocation failure, avoid allocations

[Andreas Rheinhardt]

Nuo Mi:
> On Wed, Jun 5, 2024 at 7:41 PM Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com> wrote:
> 
>> This is the VVC version of 8b5d15530127fea54e934043a64653859de07353.
>>
>> (Hint: This ensures that the order of NALU arrays is OPI-VPS-SPS-PPS-
>> Prefix-SEI-Suffix-SEI, regardless of the order in the original
>> extradata. I hope this is right.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>>  libavformat/vvc.c | 169 ++++++++++++++++++++--------------------------
>>  1 file changed, 73 insertions(+), 96 deletions(-)
>>
>> diff --git a/libavformat/vvc.c b/libavformat/vvc.c
>> index 679bb07a4d..819ee02e2c 100644
>> --- a/libavformat/vvc.c
>> +++ b/libavformat/vvc.c
>> @@ -32,6 +32,16 @@
>>  #include "avio_internal.h"
>>  #include "vvc.h"
>>
>> +enum {
>> +    OPI_INDEX,
>> +    VPS_INDEX,
>> +    SPS_INDEX,
>> +    PPS_INDEX,
>> +    SEI_PREFIX_INDEX,
>> +    SEI_SUFFIX_INDEX,
>> +    NB_ARRAYS
>> +};
>> +
>>  typedef struct VVCCNALUnitArray {
>>      uint8_t array_completeness;
>>      uint8_t NAL_unit_type;
>> @@ -67,7 +77,7 @@ typedef struct VVCDecoderConfigurationRecord {
>>      uint16_t max_picture_height;
>>      uint16_t avg_frame_rate;
>>      uint8_t num_of_arrays;
>> -    VVCCNALUnitArray *array;
>> +    VVCCNALUnitArray arrays[NB_ARRAYS];
>>  } VVCDecoderConfigurationRecord;
>>
>>  static void vvcc_update_ptl(VVCDecoderConfigurationRecord *vvcc,
>> @@ -432,32 +442,11 @@ static void nal_unit_parse_header(GetBitContext *gb,
>> uint8_t *nal_type)
>>
>>  static int vvcc_array_add_nal_unit(uint8_t *nal_buf, uint32_t nal_size,
>>                                     uint8_t nal_type, int
>> ps_array_completeness,
>> -                                   VVCDecoderConfigurationRecord *vvcc)
>> +                                   VVCCNALUnitArray *array)
>>  {
>>      int ret;
>> -    uint8_t index;
>>      uint16_t num_nalus;
>> -    VVCCNALUnitArray *array;
>> -
>> -    for (index = 0; index < vvcc->num_of_arrays; index++)
>> -        if (vvcc->array[index].NAL_unit_type == nal_type)
>> -            break;
>> -
>> -    if (index >= vvcc->num_of_arrays) {
>> -        uint8_t i;
>> -
>> -        ret =
>> -            av_reallocp_array(&vvcc->array, index + 1,
>> -                              sizeof(VVCCNALUnitArray));
>> -        if (ret < 0)
>> -            return ret;
>> -
>> -        for (i = vvcc->num_of_arrays; i <= index; i++)
>> -            memset(&vvcc->array[i], 0, sizeof(VVCCNALUnitArray));
>> -        vvcc->num_of_arrays = index + 1;
>> -    }
>>
>> -    array = &vvcc->array[index];
>>      num_nalus = array->num_nalus;
>>
>>      ret = av_reallocp_array(&array->nal_unit, num_nalus + 1,
>> sizeof(uint8_t *));
>> @@ -504,7 +493,8 @@ static int vvcc_array_add_nal_unit(uint8_t *nal_buf,
>> uint32_t nal_size,
>>
>>  static int vvcc_add_nal_unit(uint8_t *nal_buf, uint32_t nal_size,
>>                               int ps_array_completeness,
>> -                             VVCDecoderConfigurationRecord *vvcc)
>> +                             VVCDecoderConfigurationRecord *vvcc,
>> +                             unsigned array_idx)
>>  {
>>      int ret = 0;
>>      GetBitContext gbc;
>> @@ -529,18 +519,15 @@ static int vvcc_add_nal_unit(uint8_t *nal_buf,
>> uint32_t nal_size,
>>       * vvcc. Perhaps the SEI playload type should be checked
>>       * and non-declarative SEI messages discarded?
>>       */
>> -    switch (nal_type) {
>> -    case VVC_OPI_NUT:
>> -    case VVC_VPS_NUT:
>> -    case VVC_SPS_NUT:
>> -    case VVC_PPS_NUT:
>> -    case VVC_PREFIX_SEI_NUT:
>> -    case VVC_SUFFIX_SEI_NUT:
>> -        ret = vvcc_array_add_nal_unit(nal_buf, nal_size, nal_type,
>> -                                      ps_array_completeness, vvcc);
>> -        if (ret < 0)
>> -            goto end;
>> -        else if (nal_type == VVC_VPS_NUT)
>> +    ret = vvcc_array_add_nal_unit(nal_buf, nal_size, nal_type,
>> +                                  ps_array_completeness,
>> +                                  &vvcc->arrays[array_idx]);
>> +    if (ret < 0)
>> +        goto end;
>> +    if (vvcc->arrays[array_idx].num_nalus == 1)
>> +        vvcc->num_of_arrays++;
>> +
>> +        if (nal_type == VVC_VPS_NUT)
>>              ret = vvcc_parse_vps(&gbc, vvcc);
>>          else if (nal_type == VVC_SPS_NUT)
>>              ret = vvcc_parse_sps(&gbc, vvcc);
>> @@ -551,11 +538,6 @@ static int vvcc_add_nal_unit(uint8_t *nal_buf,
>> uint32_t nal_size,
>>          }
>>          if (ret < 0)
>>              goto end;
>> -        break;
>> -    default:
>> -        ret = AVERROR_INVALIDDATA;
>> -        goto end;
>> -    }
>>
>>    end:
>>      av_free(rbsp_buf);
>> @@ -572,22 +554,21 @@ static void vvcc_init(VVCDecoderConfigurationRecord
>> *vvcc)
>>
>>  static void vvcc_close(VVCDecoderConfigurationRecord *vvcc)
>>  {
>> -    uint8_t i;
>> +    for (unsigned i = 0; i < FF_ARRAY_ELEMS(vvcc->arrays); i++) {
>> +        VVCCNALUnitArray *const array = &vvcc->arrays[i];
>>
>> -    for (i = 0; i < vvcc->num_of_arrays; i++) {
>> -        vvcc->array[i].num_nalus = 0;
>> -        av_freep(&vvcc->array[i].nal_unit);
>> -        av_freep(&vvcc->array[i].nal_unit_length);
>> +        array->num_nalus = 0;
>> +        av_freep(&array->nal_unit);
>> +        av_freep(&array->nal_unit_length);
>>      }
>>
>>      vvcc->num_of_arrays = 0;
>> -    av_freep(&vvcc->array);
>>  }
>>
>>  static int vvcc_write(AVIOContext *pb, VVCDecoderConfigurationRecord
>> *vvcc)
>>  {
>>      uint8_t i;
>> -    uint16_t j, vps_count = 0, sps_count = 0, pps_count = 0;
>> +    uint16_t vps_count = 0, sps_count = 0, pps_count = 0;
>>      /*
>>       * It's unclear how to properly compute these fields, so
>>       * let's always set them to values meaning 'unspecified'.
>> @@ -672,40 +653,33 @@ static int vvcc_write(AVIOContext *pb,
>> VVCDecoderConfigurationRecord *vvcc)
>>      av_log(NULL, AV_LOG_TRACE,
>>             "num_of_arrays:                       %" PRIu8 "\n",
>>             vvcc->num_of_arrays);
>> -    for (i = 0; i < vvcc->num_of_arrays; i++) {
>> +    for (unsigned i = 0; i < FF_ARRAY_ELEMS(vvcc->arrays); i++) {
>>
> this will shadow the "uint8_t i"
> 

I just sent a patch for this (and for fixing the comment).

- Andreas