[FFmpeg-devel,6/6] avcodec/flicvideo: More strictly check chunk size for FLI_COPY

Submitted by Michael Niedermayer on June 21, 2019, 11:29 p.m.

Details

Message ID 20190621232936.11052-6-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 21, 2019, 11:29 p.m.
Fixes: Timeout (40sec -> 13sec)
Fixes: 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/flicvideo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer July 19, 2019, 11:11 a.m.
On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
> Fixes: Timeout (40sec -> 13sec)
> Fixes: 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/flicvideo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

will apply

[...]
Paul B Mahol July 19, 2019, 1:54 p.m.
On 7/19/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
>> Fixes: Timeout (40sec -> 13sec)
>> Fixes:
>> 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/flicvideo.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> will apply
>

Why? This actually is breaking old code.
Can you please stop committing such kind of patches?
Michael Niedermayer July 19, 2019, 7:46 p.m.
On Fri, Jul 19, 2019 at 03:54:19PM +0200, Paul B Mahol wrote:
> On 7/19/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
> >> Fixes: Timeout (40sec -> 13sec)
> >> Fixes:
> >> 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
> >>
> >> Found-by: continuous fuzzing process
> >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>  libavcodec/flicvideo.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > will apply
> >
> 
> Why? This actually is breaking old code.
> Can you please stop committing such kind of patches?

This patch was on the mailing list since a month, why do you point
out a problem with it only once i say that i intend to apply it ?

either way, ill of course not apply it now that you point to a
problem. But please provide a testcase that this patch breaks

Thanks


[...]
Paul B Mahol July 21, 2019, 9:18 a.m.
On 7/19/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Fri, Jul 19, 2019 at 03:54:19PM +0200, Paul B Mahol wrote:
>> On 7/19/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
>> >> Fixes: Timeout (40sec -> 13sec)
>> >> Fixes:
>> >> 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
>> >>
>> >> Found-by: continuous fuzzing process
>> >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> >> ---
>> >>  libavcodec/flicvideo.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > will apply
>> >
>>
>> Why? This actually is breaking old code.
>> Can you please stop committing such kind of patches?
>
> This patch was on the mailing list since a month, why do you point
> out a problem with it only once i say that i intend to apply it ?
>
> either way, ill of course not apply it now that you point to a
> problem. But please provide a testcase that this patch breaks

Apparently decoder have padding of some kind for this stuff.
So check FLIC video files with different resolutions (the one that are
not same as padded values).

>
> Thanks
>
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> "Nothing to hide" only works if the folks in power share the values of
> you and everyone you know entirely and always will -- Tom Scott
>
>
Reimar Döffinger July 21, 2019, 11:58 a.m.
On 19.07.2019, at 21:46, Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Fri, Jul 19, 2019 at 03:54:19PM +0200, Paul B Mahol wrote:
>> On 7/19/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> On Sat, Jun 22, 2019 at 01:29:36AM +0200, Michael Niedermayer wrote:
>>>> Fixes: Timeout (40sec -> 13sec)
>>>> Fixes:
>>>> 15417/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5679812615602176
>>>> 
>>>> Found-by: continuous fuzzing process
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>> libavcodec/flicvideo.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> will apply
>>> 
>> 
>> Why? This actually is breaking old code.
>> Can you please stop committing such kind of patches?
> 
> This patch was on the mailing list since a month, why do you point
> out a problem with it only once i say that i intend to apply it ?

You are sending an awful lot of these patches.
For my part it is purely random whether and when I manage to look at one even for code I somewhat know.
I imagine a lot of other people are struggling to keep up as well.
If you want better reviews you might want to think about better ways to categorise them to reduce review burden - e.g. by risk level.

Patch hide | download patch | download mbox

diff --git a/libavcodec/flicvideo.c b/libavcodec/flicvideo.c
index 2474a9ca72..209df591cf 100644
--- a/libavcodec/flicvideo.c
+++ b/libavcodec/flicvideo.c
@@ -729,9 +729,9 @@  static int flic_decode_frame_15_16BPP(AVCodecContext *avctx,
         case FLI_COPY:
         case FLI_DTA_COPY:
             /* copy the chunk (uncompressed frame) */
-            if (chunk_size - 6 > (unsigned int)(FFALIGN(s->avctx->width, 2) * s->avctx->height)*2) {
+            if (chunk_size - 6 != (unsigned int)(FFALIGN(s->avctx->width, 2) * s->avctx->height)*2) {
                 av_log(avctx, AV_LOG_ERROR, "In chunk FLI_COPY : source data (%d bytes) " \
-                       "bigger than image, skipping chunk\n", chunk_size - 6);
+                       "different than image, skipping chunk\n", chunk_size - 6);
                 bytestream2_skip(&g2, chunk_size - 6);
             } else {