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

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