[FFmpeg-devel] [PATCH 21/50] avformat/matroskadec: use av_packet_alloc() to allocate packets

[Andreas Rheinhardt]

James Almer:
> On 2/8/2021 12:32 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>>    libavformat/matroskadec.c | 17 ++++++++++++-----
>>>>>    1 file changed, 12 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>>>> index 374831baa3..9fad78c78b 100644
>>>>> --- a/libavformat/matroskadec.c
>>>>> +++ b/libavformat/matroskadec.c
>>>>> @@ -365,6 +365,8 @@ typedef struct MatroskaDemuxContext {
>>>>>        /* byte position of the segment inside the stream */
>>>>>        int64_t segment_start;
>>>>>    +    AVPacket *pkt;
>>>>> +
>>>>>        /* the packet queue */
>>>>>        AVPacketList *queue;
>>>>>        AVPacketList *queue_end;
>>>>> @@ -2885,6 +2887,10 @@ static int
>>>>> matroska_read_header(AVFormatContext *s)
>>>>>        }
>>>>>        ebml_free(ebml_syntax, &ebml);
>>>>>    +    matroska->pkt = av_packet_alloc();
>>>>> +    if (!matroska->pkt)
>>>>
>>>> Missing AVERROR(ENOMEM).
>>>
>>> This seems to be a common mistake in this set. Sorry.
>>>
>>
>> Yeah, the failure paths seem untested.
> 
> make fate doesn't test failure paths. I recall someone did some manual
> runs limiting available memory long ago, which detected and removed a
> lot of unchecked av_mallocs, but nothing automated.
> Fuzzing has handled failure path issue detection since then.
> 

No, fuzzing does not simulate low-memory environments (at least ossfuzz
as-is does not). I think it errors out on OOM when one tries to allocate
a lot, but small stuff like AVPackets don't trigger this. I have a long
list of patches in libavcodec where an earlier allocation in an init
function would leak if a subsequent allocation fails; or even something
like 96061c5a4f690c3ab49e4458701bb013fd3dd57f where the close function
assumed that several allocations were successful. These bugs existed for
years and the fuzzer never found them.
This means that all these errors paths need to be manually tested, so
that something that adds error paths (like requiring things to be
allocated) adds a burden on devs (in addition to the runtime overhead).

>>
>>>> (I actually have an idea to completely remove the packet list from
>>>> matroskadec.)
>>>
>>> Should i commit this while you work on that, or just withdraw this
>>> patch? It's not urgent at all, since any deprecation period is 2+ years.
>>> Either option is fine by me.
>>>
>> I don't want to have deprecation warnings.
> 
> Ok.
> 
>>
>>>>
>>>>> +        goto fail;
>>>>> +
>>>>>        /* The next thing is a segment. */
>>>>>        pos = avio_tell(matroska->ctx->pb);
>>>>>        res = ebml_parse(matroska, matroska_segments, matroska);
>>>>> @@ -2947,7 +2953,7 @@ static int matroska_read_header(AVFormatContext
>>>>> *s)
>>>>>                    st->disposition         |=
>>>>> AV_DISPOSITION_ATTACHED_PIC;
>>>>>                    st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
>>>>>    -                av_init_packet(pkt);
>>>>> +                av_packet_unref(pkt);
>>>>>                    pkt->buf          = attachments[j].bin.buf;
>>>>>                    attachments[j].bin.buf = NULL;
>>>>>                    pkt->data         = attachments[j].bin.data;
>>>>> @@ -3180,7 +3186,7 @@ static int
>>>>> matroska_parse_rm_audio(MatroskaDemuxContext *matroska,
>>>>>          while (track->audio.pkt_cnt) {
>>>>>            int ret;
>>>>> -        AVPacket pktl, *pkt = &pktl;
>>>>> +        AVPacket *pkt = matroska->pkt;
>>>>>              ret = av_new_packet(pkt, a);
>>>>>            if (ret < 0) {
>>>>> @@ -3317,7 +3323,7 @@ static int
>>>>> matroska_parse_webvtt(MatroskaDemuxContext *matroska,
>>>>>                                     uint64_t duration,
>>>>>                                     int64_t pos)
>>>>>    {
>>>>> -    AVPacket pktl, *pkt = &pktl;
>>>>> +    AVPacket *pkt = matroska->pkt;
>>>>>        uint8_t *id, *settings, *text, *buf;
>>>>>        int id_len, settings_len, text_len;
>>>>>        uint8_t *p, *q;
>>>>> @@ -3434,7 +3440,7 @@ static int
>>>>> matroska_parse_frame(MatroskaDemuxContext *matroska,
>>>>>    {
>>>>>        uint8_t *pkt_data = data;
>>>>>        int res = 0;
>>>>> -    AVPacket pktl, *pkt = &pktl;
>>>>> +    AVPacket *pkt = matroska->pkt;
>>>>>          if (st->codecpar->codec_id == AV_CODEC_ID_WAVPACK) {
>>>>>            res = matroska_parse_wavpack(track, &pkt_data, &pkt_size);
>>>>> @@ -3464,7 +3470,7 @@ static int
>>>>> matroska_parse_frame(MatroskaDemuxContext *matroska,
>>>>>        if (!pkt_size && !additional_size)
>>>>>            goto no_output;
>>>>>    -    av_init_packet(pkt);
>>>>> +    av_packet_unref(pkt);
>>>>>        if (!buf)
>>>>>            pkt->buf = av_buffer_create(pkt_data, pkt_size +
>>>>> AV_INPUT_BUFFER_PADDING_SIZE,
>>>>>                                        NULL, NULL, 0);
>>>>> @@ -3838,6 +3844,7 @@ static int
>>>>> matroska_read_close(AVFormatContext *s)
>>>>>        int n;
>>>>>          matroska_clear_queue(matroska);
>>>>> +    av_packet_free(&matroska->pkt);
>>>>>          for (n = 0; n < matroska->tracks.nb_elem; n++)
>>>>>            if (tracks[n].type == MATROSKA_TRACK_TYPE_AUDIO)
>>>>>
>>>>