[FFmpeg-devel] [PATCH 2/3] avformat/evcdec: Check that enough data has been read

[Andreas Rheinhardt]

James Almer:
> On 7/6/2023 10:30 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 7/6/2023 10:14 PM, James Almer wrote:
>>>> On 7/6/2023 6:08 PM, Andreas Rheinhardt wrote:
>>>>> Fixes potential use of uninitialized values
>>>>> in evc_read_nal_unit_length().
>>>>>
>>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>>>> ---
>>>>>    libavformat/evcdec.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/libavformat/evcdec.c b/libavformat/evcdec.c
>>>>> index 9886542311..0f464930f7 100644
>>>>> --- a/libavformat/evcdec.c
>>>>> +++ b/libavformat/evcdec.c
>>>>> @@ -162,6 +162,8 @@ static int evc_read_packet(AVFormatContext *s,
>>>>> AVPacket *pkt)
>>>>>            ret = avio_read(s->pb, buf, EVC_NALU_LENGTH_PREFIX_SIZE);
>>>>>            if (ret < 0)
>>>>>                return ret;
>>>>> +        if (ret != EVC_NALU_LENGTH_PREFIX_SIZE)
>>>>> +            return AVERROR_INVALIDDATA;
>>>>
>>>> There's a ffio_ensure_seekback() for EVC_NALU_LENGTH_PREFIX_SIZE bytes
>>>> immediately before the avio_read() call. Shouldn't that be enough to
>>>> guarantee that much can be read?
>>>>
>>>> Also, you can just pass ret to evc_read_nal_unit_length() below
>>>> instead of adding this check here. It will return an error if it's <
>>>> EVC_NALU_LENGTH_PREFIX_SIZE.
>>>
>>> Oh, my bad, i was looking at the function of the same name in
>>> libavcodec/evc_parse.h
>>>
>>> The function in evc.h could be changed to also use the same check as the
>>> one the evc_parse.h version alongside the other change you're doing in
>>> patch 3/3.
>>>
>>
>> These functions already do the same (except for the log message); they
>> both return 0 if not enough data is available. The return value would
>> need to be int64_t if one wanted to return both error codes and lengths
>> via it. I don't really see the advantage of this.
> 
> Fair enough. Then what about ffio_ensure_seekback()? Should that not
> guarantee at least EVC_NALU_LENGTH_PREFIX_SIZE bytes are buffered, thus
> readable and seekable backwards?

No. It is just supposed to ensure that if one reads
EVC_NALU_LENGTH_PREFIX_SIZE bytes, one can seek back and "unread" them.
See also my earlier mail.

- Andreas