[FFmpeg-devel] [PATCH 2/3] avformat/vividas: Check allocation for success

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Zane van Iperen:
>> On Thu, 6 Aug 2020 01:33:57 +0200
>> "Andreas Rheinhardt" <andreas.rheinhardt at gmail.com> wrote:
>>
>>>
>>>  static void load_sb_block(AVFormatContext *s, VividasDemuxContext *viv, unsigned expected_size)
>>> @@ -608,7 +606,7 @@ static int viv_read_header(AVFormatContext *s)
>>>      ret = track_index(viv, s, buf, v);
>>>      av_free(buf);
>>>      if (ret < 0)
>>> -        return ret;
>>> +        goto fail;
>>>
>>>      viv->sb_offset = avio_tell(pb);
>>>      if (viv->n_sb_blocks > 0) {
>>> @@ -619,6 +617,9 @@ static int viv_read_header(AVFormatContext *s)
>>>      }
>>>
>>>      return 0;
>>> +fail:
>>> +    av_freep(&viv->sb_blocks);
>>> +    return ret;
>>>  }
>>
>> Nit: Considering there's only one `goto fail`, wouldn't it be better to
>> just av_freep and return immediately instead?
>>
> This patch is designed so that this demuxer can easily be converted to
> automatically call read_close on read_header failure (see [1] for
> details, in fact all bugs in this patchset have been found when working
> on this goal (I already have 61 demuxers now (17 more to go in
> libavformat plus all the demuxers in libavdevice)). Doing cleanup the
> way I do it here implies that the patch that marks this demuxer as
> compatible with automatic freeing can simply remove the whole fail part
> above and replace "goto fail" with return ret again. But if I cleaned up
> in place, I would have to touch the "if (ret < 0)" line now and either
> unnecessarily touch it again when the cleanup will be made automatic or
> keep the unnecessary {} in "if (ret < 0) {\n return ret;\n}".
> 
> - Andreas
> 
[1] of course refers to
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-July/266575.html. Forgot
to add it.

- Andreas