diff mbox series

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

Message ID 20210204191005.48190-22-jamrial@gmail.com
State New
Headers show
Series deprecate av_init_packet() and sizeof(AVPacket) as part of the ABI
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Feb. 4, 2021, 7:09 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroskadec.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Andreas Rheinhardt Feb. 8, 2021, 3:22 p.m. UTC | #1
James Almer:
> Signed-off-by: James Almer <jamrial@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).
(I actually have an idea to completely remove the packet list from
matroskadec.)

> +        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)
>
James Almer Feb. 8, 2021, 3:27 p.m. UTC | #2
On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@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.

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

> 
>> +        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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt Feb. 8, 2021, 3:32 p.m. UTC | #3
James Almer:
> On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer <jamrial@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.

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

>>
>>> +        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@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Feb. 8, 2021, 3:37 p.m. UTC | #4
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@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@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt Feb. 8, 2021, 3:57 p.m. UTC | #5
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@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)
>>>>>
>>>>
James Almer Feb. 8, 2021, 4:03 p.m. UTC | #6
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
James Almer Feb. 8, 2021, 4:24 p.m. UTC | #7
On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@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).

I think at this point i can just return ENOMEM without jumping to fail.
Andreas Rheinhardt Feb. 8, 2021, 4:44 p.m. UTC | #8
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@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.

Then I don't get the point of you mentioning fuzzing as handling failure
path issues in your reply to my comment about untested allocation error
paths.

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

Static analyzers do, but they typically only find the easy ones. E.g.
Coverity would probably have found the memleak in avpriv_dv_init_demux
that your patch introduced, but not the memleaks in the dv demuxer
(because in the latter case, the packets are still accessible via the
still existing DVDemuxContext and it doesn't know that they will not be
freed).
(And of course it should be mentioned that not all parts of FFmpeg are
systematically fuzzed: Encoders, muxers and filters are 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)
>>>>>>>
>>>>>>
Andreas Rheinhardt Feb. 8, 2021, 4:47 p.m. UTC | #9
James Almer:
> On 2/8/2021 12:22 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer <jamrial@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).
> 
> I think at this point i can just return ENOMEM without jumping to fail.

Yes.

- Andreas
James Almer Feb. 8, 2021, 4:51 p.m. UTC | #10
On 2/8/2021 1:44 PM, Andreas Rheinhardt wrote:
> 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@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.
> 
> Then I don't get the point of you mentioning fuzzing as handling failure
> path issues in your reply to my comment about untested allocation error
> paths.

If you look at the above exchange, you started with the general 
statement "failure paths seem untested", and i continued from there.
Apologies if it was confusing.
Andreas Rheinhardt Feb. 8, 2021, 4:58 p.m. UTC | #11
James Almer:
> On 2/8/2021 1:44 PM, Andreas Rheinhardt wrote:
>> 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@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.
>>
>> Then I don't get the point of you mentioning fuzzing as handling failure
>> path issues in your reply to my comment about untested allocation error
>> paths.
> 
> If you look at the above exchange, you started with the general
> statement "failure paths seem untested", and i continued from there.
> Apologies if it was confusing.

I was actually thinking about the failure paths added by your set (after
all, you started the above exchange with a comment about your set); but
yes, the issue is more widespread, unfortunately.
Apologies for being so confusing.

- Andreas
diff mbox series

Patch

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)
+        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)