diff mbox series

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

Message ID 20210123221056.3366-6-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/6] avutil/common: Add FFABSU() for a signed -> unsigned ABS
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer Jan. 23, 2021, 10:10 p.m. UTC
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@niedermayer.cc>
---
 libavformat/flvdec.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Anton Khirnov Jan. 24, 2021, 1:17 p.m. UTC | #1
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@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.
Michael Niedermayer Jan. 26, 2021, 4:42 p.m. UTC | #2
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@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.

thx

[...]
Anton Khirnov Jan. 27, 2021, 9:36 a.m. UTC | #3
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@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?
diff mbox series

Patch

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);