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

[James Almer <jamrial@gmail.com>]

On 7/6/2023 10:24 PM, Andreas Rheinhardt wrote:
> James Almer:
>> 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@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?
>>
> 
> ffio_ensure_seekback() ensures that the read buffer is big enough so
> that reading EVC_NALU_LENGTH_PREFIX_SIZE bytes does not lead to a reset
> of the buffer; it does not imply that the buffer already contains
> EVC_NALU_LENGTH_PREFIX_SIZE bytes. In fact, there is not a single
> codepath in ffio_ensure_seekback() that actually reads further input.
> (If EVC_NALU_LENGTH_PREFIX_SIZE bytes are not available in the buffer,
> then the buf_size <= s->buffer_size codepath will likely be taken in the
> non-seekable case (in the seekable case, ffio_ensure_seekback() does
> even less, namely nothing).)
> 
>> 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.
>>
> 
> It will actually return 0 which the caller will transform into an error.
> I do not want to rely on this behaviour.

Ok, patch LGTM then.

> (Why did you add two inline functions of the same name in different evc
> headers?)

If i recall correctly, because the probe function must not call av_log.

> 
>>>              nalu_size = evc_read_nal_unit_length(buf,
>>> EVC_NALU_LENGTH_PREFIX_SIZE);
>>>            if (!nalu_size || nalu_size > INT_MAX)
> 
> _______________________________________________
> 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".