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

[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.

> 
>>> (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)
>>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>