Re: [FFmpeg-devel] [PATCH 2/2] avcodec/scpr: Check for min > max in decompress_p()
On Mon, Aug 06, 2018 at 09:05:51AM +0200, Paul B Mahol wrote: > On 8/5/18, Michael Niedermayer wrote: > > On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote: > >> On 8/5/18, Michael Niedermayer wrote: > >> > Fixes: Timeout > >> > Fixes: > >> > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312 > >> > > >> > Found-by: continuous fuzzing process > >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> > Signed-off-by: Michael Niedermayer > >> > --- > >> > libavcodec/scpr.c | 3 +++ > >> > 1 file changed, 3 insertions(+) > >> > > >> > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c > >> > index 72f59d5917..d1e47b09ac 100644 > >> > --- a/libavcodec/scpr.c > >> > +++ b/libavcodec/scpr.c > >> > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx, > >> > if (ret < 0) > >> > return ret; > >> > > >> > +if (min > max) > >> > +return AVERROR_INVALIDDATA; > >> > + > >> > >> Shouldn't this check be actually bellow? > > > > yes, fixed, locally > > > > > >> You sure this does not break valid files? > > > > i found no file that it breaks, beyond this, no iam not sure. > > It mostly based on logic thinking that these would not be ordered the > > other way, as that seems not usefull > > > > Is there some specification or more files i can test ? > > It should be fine. will apply thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB "I am not trying to be anyone's saviour, I'm trying to think about the future and not be sad" - Elon Musk signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/scpr: Check for min > max in decompress_p()
On 8/5/18, Michael Niedermayer wrote: > On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote: >> On 8/5/18, Michael Niedermayer wrote: >> > Fixes: Timeout >> > Fixes: >> > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312 >> > >> > Found-by: continuous fuzzing process >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> > Signed-off-by: Michael Niedermayer >> > --- >> > libavcodec/scpr.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c >> > index 72f59d5917..d1e47b09ac 100644 >> > --- a/libavcodec/scpr.c >> > +++ b/libavcodec/scpr.c >> > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx, >> > if (ret < 0) >> > return ret; >> > >> > +if (min > max) >> > +return AVERROR_INVALIDDATA; >> > + >> >> Shouldn't this check be actually bellow? > > yes, fixed, locally > > >> You sure this does not break valid files? > > i found no file that it breaks, beyond this, no iam not sure. > It mostly based on logic thinking that these would not be ordered the > other way, as that seems not usefull > > Is there some specification or more files i can test ? It should be fine. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/scpr: Check for min > max in decompress_p()
On Sun, Aug 05, 2018 at 10:08:31AM +0200, Paul B Mahol wrote: > On 8/5/18, Michael Niedermayer wrote: > > Fixes: Timeout > > Fixes: > > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer > > --- > > libavcodec/scpr.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c > > index 72f59d5917..d1e47b09ac 100644 > > --- a/libavcodec/scpr.c > > +++ b/libavcodec/scpr.c > > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx, > > if (ret < 0) > > return ret; > > > > +if (min > max) > > +return AVERROR_INVALIDDATA; > > + > > Shouldn't this check be actually bellow? yes, fixed, locally > You sure this does not break valid files? i found no file that it breaks, beyond this, no iam not sure. It mostly based on logic thinking that these would not be ordered the other way, as that seems not usefull Is there some specification or more files i can test ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Rewriting code that is poorly written but fully understood is good. Rewriting code that one doesnt understand is a sign that one is less smart then the original author, trying to rewrite it will not make it better. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/scpr: Check for min > max in decompress_p()
On 8/5/18, Michael Niedermayer wrote: > Fixes: Timeout > Fixes: > 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/scpr.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c > index 72f59d5917..d1e47b09ac 100644 > --- a/libavcodec/scpr.c > +++ b/libavcodec/scpr.c > @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx, > if (ret < 0) > return ret; > > +if (min > max) > +return AVERROR_INVALIDDATA; > + Shouldn't this check be actually bellow? You sure this does not break valid files? > max += temp << 8; > memset(s->blocks, 0, sizeof(*s->blocks) * s->nbcount); > > -- > 2.18.0 > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avcodec/scpr: Check for min > max in decompress_p()
Fixes: Timeout Fixes: 9342/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SCPR_fuzzer-4795990841229312 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/scpr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavcodec/scpr.c b/libavcodec/scpr.c index 72f59d5917..d1e47b09ac 100644 --- a/libavcodec/scpr.c +++ b/libavcodec/scpr.c @@ -525,6 +525,9 @@ static int decompress_p(AVCodecContext *avctx, if (ret < 0) return ret; +if (min > max) +return AVERROR_INVALIDDATA; + max += temp << 8; memset(s->blocks, 0, sizeof(*s->blocks) * s->nbcount); -- 2.18.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
