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

[Andreas Rheinhardt]

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