| Message ID | 20190720220856.17516-4-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
Jul 20, 2019, 11:08 PM by michael@niedermayer.cc: > Fixes: Timeout (22 -> 7 sec) > Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/hqx.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > index bc24ba91d1..8639d77a41 100644 > --- a/libavcodec/hqx.c > +++ b/libavcodec/hqx.c > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data, > avctx->height = ctx->height; > avctx->bits_per_raw_sample = 10; > > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size) > + return AVERROR_INVALIDDATA; > + > Not only are you ignoring my and others opinion, not only you still continue sending these awful patches, you've just submitted by far the worst one I've ever seen thinking its okay. Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help. Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck?
> Am 21.07.2019 um 00:36 schrieb Lynne <dev@lynne.ee>: > > Jul 20, 2019, 11:08 PM by michael@niedermayer.cc: > >> Fixes: Timeout (22 -> 7 sec) >> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/hqx.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >> index bc24ba91d1..8639d77a41 100644 >> --- a/libavcodec/hqx.c >> +++ b/libavcodec/hqx.c >> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data, >> avctx->height = ctx->height; >> avctx->bits_per_raw_sample = 10; >> >> + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >> + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size) >> + return AVERROR_INVALIDDATA; >> + >> > > Not only are you ignoring my and others opinion, not only you still continue sending these awful patches, > you've just submitted by far the worst one I've ever seen thinking its okay. > Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help. > Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck? Ad hominem attacks sadly do not count as reviews. Carl Eugen
Jul 21, 2019, 12:31 AM by ceffmpeg@gmail.com: > > >> Am 21.07.2019 um 00:36 schrieb Lynne <dev@lynne.ee>: >> >> Jul 20, 2019, 11:08 PM by michael@niedermayer.cc: >> >>> Fixes: Timeout (22 -> 7 sec) >>> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/hqx.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >>> index bc24ba91d1..8639d77a41 100644 >>> --- a/libavcodec/hqx.c >>> +++ b/libavcodec/hqx.c >>> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data, >>> avctx->height = ctx->height; >>> avctx->bits_per_raw_sample = 10; >>> >>> + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >>> + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size) >>> + return AVERROR_INVALIDDATA; >>> + >>> >> >> Not only are you ignoring my and others opinion, not only you still continue sending these awful patches, >> you've just submitted by far the worst one I've ever seen thinking its okay. >> Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help. >> Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck? >> > > Ad hominem attacks sadly do not count as reviews. > Something like this doesn't deserve anything but the lowest level of criticism. Can't even be called a patch, so it can't have a review in my opinion.
> Something like this doesn't deserve anything but the lowest level of criticism.
But the lowest level of criticism is not allowed here.
Carl Eugen
On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Timeout (22 -> 7 sec) > Fixes: > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/hqx.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > index bc24ba91d1..8639d77a41 100644 > --- a/libavcodec/hqx.c > +++ b/libavcodec/hqx.c > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void > *data, > avctx->height = ctx->height; > avctx->bits_per_raw_sample = 10; > > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * > avpkt->size) > + return AVERROR_INVALIDDATA; Why just this change and not something better? > + > switch (ctx->format) { > case HQX_422: > avctx->pix_fmt = AV_PIX_FMT_YUV422P16; > -- > 2.22.0 > > _______________________________________________ > 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".
On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: > On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: Timeout (22 -> 7 sec) > > Fixes: > > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/hqx.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > > index bc24ba91d1..8639d77a41 100644 > > --- a/libavcodec/hqx.c > > +++ b/libavcodec/hqx.c > > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void > > *data, > > avctx->height = ctx->height; > > avctx->bits_per_raw_sample = 10; > > > > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * > > avpkt->size) > > + return AVERROR_INVALIDDATA; > > Why just this change and not something better? What would you prefer exactly ? Thanks [...]
On 21.07.2019, at 00:36, Lynne <dev@lynne.ee> wrote: > Jul 20, 2019, 11:08 PM by michael@niedermayer.cc: > >> Fixes: Timeout (22 -> 7 sec) >> Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/hqx.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >> index bc24ba91d1..8639d77a41 100644 >> --- a/libavcodec/hqx.c >> +++ b/libavcodec/hqx.c >> @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data, >> avctx->height = ctx->height; >> avctx->bits_per_raw_sample = 10; >> >> + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >> + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size) >> + return AVERROR_INVALIDDATA; >> + >> > > Not only are you ignoring my and others opinion, not only you still continue sending these awful patches, > you've just submitted by far the worst one I've ever seen thinking its okay. > Patches like these motivate developers to not even bother including test samples for new decoders, or even write them. Myself included. Doing exactly the opposite of what this system's meant to help. > Sure, you sent this for review, but how can you even consider this utterly ridiculous hack for a problem that doesn't exist even worthy for review in the first place? Just what the fuck? I kind of understand your point of view, and the fuzzer complaining should not be an excuse to skip writing a commit message with some motivation for example, but I think you are a bit over the top. There is already a discard_damaged_percentage and there is a point that maybe if a packet is obviously too broken to discard it with minimal overhead. Those do not seem like utterly ridiculous ideas as your reply makes them out to me. Nor is the idea of having some hardening against all too easy DoS attacks in some use-cases. Also some value in just having the fuzzer run efficiently (it also discovers quite some real issues). I agree it's hackish, I don't know the code enough to know it's correct, it needs to be properly documented (Michael, please, if you add such non-obvious, and especially heuristic checks, there needs to be some comment telling people the idea behind it and how to easily verify its correctness etc.). With that in mind, can you maybe see why I do think that discussing such patch proposals does have merit? Can we maybe come up with some compromise without being mad at each other? Maybe some of these likely to be more controversial could be submitted as RFC instead of patch first? Best regards, Reimar
On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > Fixes: Timeout (22 -> 7 sec) >> > Fixes: >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >> > >> > Found-by: continuous fuzzing process >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavcodec/hqx.c | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >> > index bc24ba91d1..8639d77a41 100644 >> > --- a/libavcodec/hqx.c >> > +++ b/libavcodec/hqx.c >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, >> > void >> > *data, >> > avctx->height = ctx->height; >> > avctx->bits_per_raw_sample = 10; >> > >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * >> > avpkt->size) >> > + return AVERROR_INVALIDDATA; >> >> Why just this change and not something better? > > What would you prefer exactly ? Something that works with pure black video. > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Modern terrorism, a quick summary: Need oil, start war with country that > has oil, kill hundread thousand in war. Let country fall into chaos, > be surprised about raise of fundamantalists. Drop more bombs, kill more > people, be surprised about them taking revenge and drop even more bombs > and strip your own citizens of their rights and freedoms. to be continued >
On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: > On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: > >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > Fixes: Timeout (22 -> 7 sec) > >> > Fixes: > >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > >> > > >> > Found-by: continuous fuzzing process > >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > libavcodec/hqx.c | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > > >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > >> > index bc24ba91d1..8639d77a41 100644 > >> > --- a/libavcodec/hqx.c > >> > +++ b/libavcodec/hqx.c > >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, > >> > void > >> > *data, > >> > avctx->height = ctx->height; > >> > avctx->bits_per_raw_sample = 10; > >> > > >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * > >> > avpkt->size) > >> > + return AVERROR_INVALIDDATA; > >> > >> Why just this change and not something better? > > > > What would you prefer exactly ? > > Something that works with pure black video. Can you share the failing video file ? I thought theres a minimum size of 1 vlc code (2 bit seem the smallest) per 16x16 block. But quite possibly i might have missed something Thanks [...]
On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > Fixes: Timeout (22 -> 7 sec) >> >> > Fixes: >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >> >> > >> >> > Found-by: continuous fuzzing process >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> > --- >> >> > libavcodec/hqx.c | 4 ++++ >> >> > 1 file changed, 4 insertions(+) >> >> > >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >> >> > index bc24ba91d1..8639d77a41 100644 >> >> > --- a/libavcodec/hqx.c >> >> > +++ b/libavcodec/hqx.c >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext >> >> > *avctx, >> >> > void >> >> > *data, >> >> > avctx->height = ctx->height; >> >> > avctx->bits_per_raw_sample = 10; >> >> > >> >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >> >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * >> >> > avpkt->size) >> >> > + return AVERROR_INVALIDDATA; >> >> >> >> Why just this change and not something better? >> > >> > What would you prefer exactly ? >> >> Something that works with pure black video. > > Can you share the failing video file ? > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest) > per 16x16 block. But quite possibly i might have missed something > This is very disappointing. There is no freely available encoder for HQX. And the one who commits stuff need to make sure it does not introduce regressions. > Thanks > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The educated differ from the uneducated as much as the living from the > dead. -- Aristotle >
On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote: > On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: > >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: > >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> >> > Fixes: Timeout (22 -> 7 sec) > >> >> > Fixes: > >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > >> >> > > >> >> > Found-by: continuous fuzzing process > >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> > --- > >> >> > libavcodec/hqx.c | 4 ++++ > >> >> > 1 file changed, 4 insertions(+) > >> >> > > >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > >> >> > index bc24ba91d1..8639d77a41 100644 > >> >> > --- a/libavcodec/hqx.c > >> >> > +++ b/libavcodec/hqx.c > >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext > >> >> > *avctx, > >> >> > void > >> >> > *data, > >> >> > avctx->height = ctx->height; > >> >> > avctx->bits_per_raw_sample = 10; > >> >> > > >> >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > >> >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * > >> >> > avpkt->size) > >> >> > + return AVERROR_INVALIDDATA; > >> >> > >> >> Why just this change and not something better? > >> > > >> > What would you prefer exactly ? > >> > >> Something that works with pure black video. > > > > Can you share the failing video file ? > > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest) > > per 16x16 block. But quite possibly i might have missed something > > > > This is very disappointing. There is no freely available encoder for HQX. > And the one who commits stuff need to make sure it does not introduce > regressions. The reviewer just has to explain how the problem he speaks of can occur. If we look at decode_slice_thread() each slice reads data from a distinct input area (this is checked to be distinct in the code). So even the same black slice cannot reuse the same representation. decode_slice_thread calls decode_slice which calls decode_func decode_func can be one of 4 implementations each reading at least a minimum of 2 bit. so we have a minimum of 2 bits per macroblock the calls happen at a granularity of 16x16 so theres a minimum of 2 bits per 16x16 block. Its very possible that i have missed some path or case in this analysis. But we wont really move forward based on riddles. If you know of a path/case where a frame can be encoded with less input data just explain that case and ill adjust the patch accordingly. Otherwise the patch is stuck as i dont know what case you speak of PS: also there are no other return pathes in hqx_decode_frame() all returns are error pathes so i dont see any shortcuts for black frames which would bypass the minimum size Thanks [...]
On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote: >> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: >> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> >> >> > Fixes: Timeout (22 -> 7 sec) >> >> >> > Fixes: >> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >> >> >> > >> >> >> > Found-by: continuous fuzzing process >> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> >> >> > --- >> >> >> > libavcodec/hqx.c | 4 ++++ >> >> >> > 1 file changed, 4 insertions(+) >> >> >> > >> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >> >> >> > index bc24ba91d1..8639d77a41 100644 >> >> >> > --- a/libavcodec/hqx.c >> >> >> > +++ b/libavcodec/hqx.c >> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext >> >> >> > *avctx, >> >> >> > void >> >> >> > *data, >> >> >> > avctx->height = ctx->height; >> >> >> > avctx->bits_per_raw_sample = 10; >> >> >> > >> >> >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >> >> >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * >> >> >> > avpkt->size) >> >> >> > + return AVERROR_INVALIDDATA; >> >> >> >> >> >> Why just this change and not something better? >> >> > >> >> > What would you prefer exactly ? >> >> >> >> Something that works with pure black video. >> > >> > Can you share the failing video file ? >> > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest) >> > per 16x16 block. But quite possibly i might have missed something >> > >> >> This is very disappointing. There is no freely available encoder for HQX. >> And the one who commits stuff need to make sure it does not introduce >> regressions. > > The reviewer just has to explain how the problem he speaks of can > occur. No, its other way around. The patch author just has to explain how the problem he tries to solve is valid solution by given patch. > > If we look at decode_slice_thread() each slice reads data from a distinct > input area (this is checked to be distinct in the code). So even the > same black slice cannot reuse the same representation. > > decode_slice_thread calls decode_slice which calls decode_func > decode_func can be one of 4 implementations each reading at least > a minimum of 2 bit. so we have a minimum of 2 bits per macroblock > the calls happen at a granularity of 16x16 so theres a minimum of > 2 bits per 16x16 block. > Its very possible that i have missed some path or case in this > analysis. But we wont really move forward based on riddles. > If you know of a path/case where a frame can be encoded with > less input data just explain that case and ill adjust the > patch accordingly. Otherwise the patch is stuck as i dont > know what case you speak of For start, get a copy of HQX encoder, than we can talk more. > > PS: also there are no other return pathes in hqx_decode_frame() > all returns are error pathes so i dont see any shortcuts for > black frames which would bypass the minimum size > > Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Never trust a computer, one day, it may think you are the virus. -- Compn >
On 7/23/19, Paul B Mahol <onemda@gmail.com> wrote: > On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >> On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote: >>> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: >>> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: >>> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: >>> >> >> > Fixes: Timeout (22 -> 7 sec) >>> >> >> > Fixes: >>> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 >>> >> >> > >>> >> >> > Found-by: continuous fuzzing process >>> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> >> >> > --- >>> >> >> > libavcodec/hqx.c | 4 ++++ >>> >> >> > 1 file changed, 4 insertions(+) >>> >> >> > >>> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c >>> >> >> > index bc24ba91d1..8639d77a41 100644 >>> >> >> > --- a/libavcodec/hqx.c >>> >> >> > +++ b/libavcodec/hqx.c >>> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext >>> >> >> > *avctx, >>> >> >> > void >>> >> >> > *data, >>> >> >> > avctx->height = ctx->height; >>> >> >> > avctx->bits_per_raw_sample = 10; >>> >> >> > >>> >> >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * >>> >> >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * >>> >> >> > avpkt->size) >>> >> >> > + return AVERROR_INVALIDDATA; >>> >> >> >>> >> >> Why just this change and not something better? >>> >> > >>> >> > What would you prefer exactly ? >>> >> >>> >> Something that works with pure black video. >>> > >>> > Can you share the failing video file ? >>> > I thought theres a minimum size of 1 vlc code (2 bit seem the >>> > smallest) >>> > per 16x16 block. But quite possibly i might have missed something >>> > >>> >>> This is very disappointing. There is no freely available encoder for >>> HQX. >>> And the one who commits stuff need to make sure it does not introduce >>> regressions. >> >> The reviewer just has to explain how the problem he speaks of can >> occur. > > No, its other way around. > The patch author just has to explain how the problem he tries to solve > is valid solution by given patch. > >> >> If we look at decode_slice_thread() each slice reads data from a distinct >> input area (this is checked to be distinct in the code). So even the >> same black slice cannot reuse the same representation. >> >> decode_slice_thread calls decode_slice which calls decode_func >> decode_func can be one of 4 implementations each reading at least >> a minimum of 2 bit. so we have a minimum of 2 bits per macroblock >> the calls happen at a granularity of 16x16 so theres a minimum of >> 2 bits per 16x16 block. >> Its very possible that i have missed some path or case in this >> analysis. But we wont really move forward based on riddles. >> If you know of a path/case where a frame can be encoded with >> less input data just explain that case and ill adjust the >> patch accordingly. Otherwise the patch is stuck as i dont >> know what case you speak of > > For start, get a copy of HQX encoder, than we can talk more. Here, I was not lazy so I got one sample for you: http://0x0.st/zppS.avi > >> >> PS: also there are no other return pathes in hqx_decode_frame() >> all returns are error pathes so i dont see any shortcuts for >> black frames which would bypass the minimum size >> >> Thanks >> >> [...] >> -- >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >> >> Never trust a computer, one day, it may think you are the virus. -- Compn >> >
On Tue, Jul 23, 2019 at 09:52:59AM +0200, Paul B Mahol wrote: > On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Jul 23, 2019 at 09:03:32AM +0200, Paul B Mahol wrote: > >> On 7/23/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> > On Mon, Jul 22, 2019 at 08:20:54AM +0200, Paul B Mahol wrote: > >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> >> > On Sun, Jul 21, 2019 at 10:48:26AM +0200, Paul B Mahol wrote: > >> >> >> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote: > >> >> >> > Fixes: Timeout (22 -> 7 sec) > >> >> >> > Fixes: > >> >> >> > 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 > >> >> >> > > >> >> >> > Found-by: continuous fuzzing process > >> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >> >> >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> >> >> > --- > >> >> >> > libavcodec/hqx.c | 4 ++++ > >> >> >> > 1 file changed, 4 insertions(+) > >> >> >> > > >> >> >> > diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c > >> >> >> > index bc24ba91d1..8639d77a41 100644 > >> >> >> > --- a/libavcodec/hqx.c > >> >> >> > +++ b/libavcodec/hqx.c > >> >> >> > @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext > >> >> >> > *avctx, > >> >> >> > void > >> >> >> > *data, > >> >> >> > avctx->height = ctx->height; > >> >> >> > avctx->bits_per_raw_sample = 10; > >> >> >> > > >> >> >> > + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * > >> >> >> > + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * > >> >> >> > avpkt->size) > >> >> >> > + return AVERROR_INVALIDDATA; > >> >> >> > >> >> >> Why just this change and not something better? > >> >> > > >> >> > What would you prefer exactly ? > >> >> > >> >> Something that works with pure black video. > >> > > >> > Can you share the failing video file ? > >> > I thought theres a minimum size of 1 vlc code (2 bit seem the smallest) > >> > per 16x16 block. But quite possibly i might have missed something > >> > > >> > >> This is very disappointing. There is no freely available encoder for HQX. > >> And the one who commits stuff need to make sure it does not introduce > >> regressions. > > > > The reviewer just has to explain how the problem he speaks of can > > occur. > > No, its other way around. > The patch author just has to explain how the problem he tries to solve > is valid solution by given patch. I have explained that in the very mail you just replied to. anyway, no problem ill find a way to encode a pure black hqx video or will otherwise do a more complete proof of correctness. (or of course if an issue is found by doing so improve the patch) I just thought if you know of a issue and told me what it is exactly that would be much simpler and quicker. Thanks [...]
diff --git a/libavcodec/hqx.c b/libavcodec/hqx.c index bc24ba91d1..8639d77a41 100644 --- a/libavcodec/hqx.c +++ b/libavcodec/hqx.c @@ -471,6 +471,10 @@ static int hqx_decode_frame(AVCodecContext *avctx, void *data, avctx->height = ctx->height; avctx->bits_per_raw_sample = 10; + if (avctx->coded_width / 16 * (avctx->coded_height / 16) * + (100 - avctx->discard_damaged_percentage) / 100 > 8LL * avpkt->size) + return AVERROR_INVALIDDATA; + switch (ctx->format) { case HQX_422: avctx->pix_fmt = AV_PIX_FMT_YUV422P16;
Fixes: Timeout (22 -> 7 sec) Fixes: 15173/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HQX_fuzzer-5662556846292992 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hqx.c | 4 ++++ 1 file changed, 4 insertions(+)