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

[James Almer]

On 2/8/2021 12:57 PM, Andreas Rheinhardt wrote:
> 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 know, which is why i said failure path issues, not OOM specifically.
Most failure paths it triggers are due to invalid data. But outside of 
fuzzing, nothing tests any kind of failure path (Maybe static 
analyzers?). All FATE tests do is ensure working scenarios keep working.

> 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)
>>>>>>
>>>>>
> _______________________________________________
> 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".
>