[FFmpeg-devel] [PATCH 6/6] avformat/flvdec: Check for EOF in loop in flv_data_packet()

[Anton Khirnov]

Quoting Michael Niedermayer (2021-01-26 17:42:27)
> On Sun, Jan 24, 2021 at 02:17:05PM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-01-23 23:10:56)
> > > Fixes: Timeout
> > > Fixes: 29656/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-5840098987999232
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavformat/flvdec.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> > > index 4bc5e15dd2..02797e1fba 100644
> > > --- a/libavformat/flvdec.c
> > > +++ b/libavformat/flvdec.c
> > > @@ -909,6 +909,11 @@ static int flv_data_packet(AVFormatContext *s, AVPacket *pkt,
> > >  
> > >      while (array || (ret = amf_get_string(pb, buf, sizeof(buf))) > 0) {
> > >          AMFDataType type = avio_r8(pb);
> > > +        if (avio_feof(pb)) {
> > > +            ret = AVERROR_INVALIDDATA;
> > > +            goto skip;
> > > +        }
> > > +
> > >          if (type == AMF_DATA_TYPE_STRING && (array || !strcmp(buf, "text"))) {
> > >              length = avio_rb16(pb);
> > >              ret    = av_get_packet(pb, pkt, length);
> > > -- 
> > > 2.17.1
> > 
> > IMO it would be cleaner to make amf_get_string() check the return value
> > of avio_read() and return an error on short reads.
> 
> will do but that does not fix the issue as array is set and
> amf_get_string() does not run in that case.

That makes me wonder whether it would not be better to make
av_get_packet() return an error on short reads. Is there any use case
in lavf where we would want to return a partial packet?

-- 
Anton Khirnov