Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Thu, Aug 10, 2023 at 05:58:24PM +0200, Paul B Mahol wrote:
> On Thu, Aug 10, 2023 at 5:46 PM Michael Niedermayer
> wrote:
>
> > On Thu, Aug 10, 2023 at 12:12:51PM +0200, Paul B Mahol wrote:
> > > On Thu, Aug 10, 2023 at 11:34 AM Michael Niedermayer <
> > mich...@niedermayer.cc>
> > > wrote:
> > >
> > > > On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote:
> > > > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer <
> > > > mich...@niedermayer.cc>
> > > > > wrote:
> > > > >
> > > > > > Hi Paul
> > > > > >
> > > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote:
> > > > > > > This is not correct, and please stop writing such patches.
> > Thanks.
> > > > > >
> > > > > > If there is a problem in the bugfix, please explain what the
> > problem
> > > > is.
> > > > > > If you do not like the specific fix, you can fix it differently
> > too or
> > > > > > tell me what you prefer.
> > > > > > Simply saying "no" with no explanation repeatedly is rude
> > > > > >
> > > > >
> > > > > Patch breaks valid files.
> > > >
> > > > Does the patch break files you create intentionally or files
> > > > pre-existing ?
> > > > The check can fail if 2 data segments overlap, one can craft
> > > > a file with that. The previous patches are implemented differently
> > > > and dont have that behavior, you rejected them too and at the time
> > > > you did call them "hacky" and did not mention that they break anything
> > > > and also ignored all further questions
> > > >
> > > > I just implemented this one differently as the other way was rejected
> > > > by you with no comment
> > > >
> > > > Also please provide the files this breaks so the issue can be
> > > > fixed
> > > >
> > > >
> > > Why not same thing for AV1 codec?
> > > Just reduce max resolutions for mv30 to 32x32 and be done.
> >
> > Limiting the resolution to max 32x32 would break real samples
> > for example V-codecs/mv30.avi
> >
> > if you suggest to limit it only for the fuzzer, well, that would not
> > fix the timeout outside the fuzzer.
> > For some decoders limiting the resolution in the fuzzer is the only
> > practical
> > option. But for mv30 this timeout really occurs because the input is not
> > checked/validated.
> >
>
> But mv30 bitstream is not so trivial and can not be checked that easily
> with your patches.
for decode_intra the loop reads from mgb look like this:
for (int y = 0; y < avctx->height; y += 16) {
...
for (int x = 0; x < avctx->width; x += 16) {
...
for (int b = 0; b < 6; b++) {
int mode = get_bits_le(, 2);
...
}
}
the only way to escape this loop is through error exit
so 2*6* number of 16x16 blocks is read unconditionally
and its read from a place that has s->mode_size * 8 bits
alternatively get_bits_left() can be used
for decode inter:
there is this at the begin
mask = *gb;
skip_bits_long(gb, mask_size * 8);
mgb = *gb;
skip_bits_long(gb, s->mode_size * 8);
so we have at minimum (mask_size + s->mode_size) * 8 bits
It does seem reasonable to me to check the input to contain enough bits
for what is consumed
If it breaks some real files, i very much would like to know what these
files are doing
>
> Also whole fuzzing idea of your work is flawed.
what do you mean by "whole fuzzing idea" ?
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Thu, Aug 10, 2023 at 5:46 PM Michael Niedermayer wrote: > On Thu, Aug 10, 2023 at 12:12:51PM +0200, Paul B Mahol wrote: > > On Thu, Aug 10, 2023 at 11:34 AM Michael Niedermayer < > mich...@niedermayer.cc> > > wrote: > > > > > On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote: > > > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer < > > > mich...@niedermayer.cc> > > > > wrote: > > > > > > > > > Hi Paul > > > > > > > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > > > > > > This is not correct, and please stop writing such patches. > Thanks. > > > > > > > > > > If there is a problem in the bugfix, please explain what the > problem > > > is. > > > > > If you do not like the specific fix, you can fix it differently > too or > > > > > tell me what you prefer. > > > > > Simply saying "no" with no explanation repeatedly is rude > > > > > > > > > > > > > Patch breaks valid files. > > > > > > Does the patch break files you create intentionally or files > > > pre-existing ? > > > The check can fail if 2 data segments overlap, one can craft > > > a file with that. The previous patches are implemented differently > > > and dont have that behavior, you rejected them too and at the time > > > you did call them "hacky" and did not mention that they break anything > > > and also ignored all further questions > > > > > > I just implemented this one differently as the other way was rejected > > > by you with no comment > > > > > > Also please provide the files this breaks so the issue can be > > > fixed > > > > > > > > Why not same thing for AV1 codec? > > Just reduce max resolutions for mv30 to 32x32 and be done. > > Limiting the resolution to max 32x32 would break real samples > for example V-codecs/mv30.avi > > if you suggest to limit it only for the fuzzer, well, that would not > fix the timeout outside the fuzzer. > For some decoders limiting the resolution in the fuzzer is the only > practical > option. But for mv30 this timeout really occurs because the input is not > checked/validated. > But mv30 bitstream is not so trivial and can not be checked that easily with your patches. Also whole fuzzing idea of your work is flawed. > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In a rich man's house there is no place to spit but his face. > -- Diogenes of Sinope > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Thu, Aug 10, 2023 at 12:12:51PM +0200, Paul B Mahol wrote: > On Thu, Aug 10, 2023 at 11:34 AM Michael Niedermayer > wrote: > > > On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote: > > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer < > > mich...@niedermayer.cc> > > > wrote: > > > > > > > Hi Paul > > > > > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > > > > > This is not correct, and please stop writing such patches. Thanks. > > > > > > > > If there is a problem in the bugfix, please explain what the problem > > is. > > > > If you do not like the specific fix, you can fix it differently too or > > > > tell me what you prefer. > > > > Simply saying "no" with no explanation repeatedly is rude > > > > > > > > > > Patch breaks valid files. > > > > Does the patch break files you create intentionally or files > > pre-existing ? > > The check can fail if 2 data segments overlap, one can craft > > a file with that. The previous patches are implemented differently > > and dont have that behavior, you rejected them too and at the time > > you did call them "hacky" and did not mention that they break anything > > and also ignored all further questions > > > > I just implemented this one differently as the other way was rejected > > by you with no comment > > > > Also please provide the files this breaks so the issue can be > > fixed > > > > > Why not same thing for AV1 codec? > Just reduce max resolutions for mv30 to 32x32 and be done. Limiting the resolution to max 32x32 would break real samples for example V-codecs/mv30.avi if you suggest to limit it only for the fuzzer, well, that would not fix the timeout outside the fuzzer. For some decoders limiting the resolution in the fuzzer is the only practical option. But for mv30 this timeout really occurs because the input is not checked/validated. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Thu, Aug 10, 2023 at 11:34 AM Michael Niedermayer wrote: > On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote: > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer < > mich...@niedermayer.cc> > > wrote: > > > > > Hi Paul > > > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > > > > This is not correct, and please stop writing such patches. Thanks. > > > > > > If there is a problem in the bugfix, please explain what the problem > is. > > > If you do not like the specific fix, you can fix it differently too or > > > tell me what you prefer. > > > Simply saying "no" with no explanation repeatedly is rude > > > > > > > Patch breaks valid files. > > Does the patch break files you create intentionally or files > pre-existing ? > The check can fail if 2 data segments overlap, one can craft > a file with that. The previous patches are implemented differently > and dont have that behavior, you rejected them too and at the time > you did call them "hacky" and did not mention that they break anything > and also ignored all further questions > > I just implemented this one differently as the other way was rejected > by you with no comment > > Also please provide the files this breaks so the issue can be > fixed > > Why not same thing for AV1 codec? Just reduce max resolutions for mv30 to 32x32 and be done. > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Dictatorship naturally arises out of democracy, and the most aggravated > form of tyranny and slavery out of the most extreme liberty. -- Plato > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Wed, Aug 09, 2023 at 11:20:43PM +0200, Paul B Mahol wrote: > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer > wrote: > > > Hi Paul > > > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > > > This is not correct, and please stop writing such patches. Thanks. > > > > If there is a problem in the bugfix, please explain what the problem is. > > If you do not like the specific fix, you can fix it differently too or > > tell me what you prefer. > > Simply saying "no" with no explanation repeatedly is rude > > > > Patch breaks valid files. Does the patch break files you create intentionally or files pre-existing ? The check can fail if 2 data segments overlap, one can craft a file with that. The previous patches are implemented differently and dont have that behavior, you rejected them too and at the time you did call them "hacky" and did not mention that they break anything and also ignored all further questions I just implemented this one differently as the other way was rejected by you with no comment Also please provide the files this breaks so the issue can be fixed thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Wed, Aug 9, 2023 at 11:15 PM James Almer wrote: > On 8/9/2023 6:20 PM, Paul B Mahol wrote: > > On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer < > mich...@niedermayer.cc> > > wrote: > > > >> Hi Paul > >> > >> On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > >>> This is not correct, and please stop writing such patches. Thanks. > >> > >> If there is a problem in the bugfix, please explain what the problem is. > >> If you do not like the specific fix, you can fix it differently too or > >> tell me what you prefer. > >> Simply saying "no" with no explanation repeatedly is rude > >> > > > > Patch breaks valid files. > > Can those files be added to FATE alongside a test? > Sure, once SDR files have been removed. > > > > > > >> > >> thank you > >> > >> [...] > >> -- > >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > >> > >> I have often repented speaking, but never of holding my tongue. > >> -- Xenocrates > >> ___ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >> > >> To unsubscribe, visit link above, or email > >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > >> > > ___ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On 8/9/2023 6:20 PM, Paul B Mahol wrote: On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer wrote: Hi Paul On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: This is not correct, and please stop writing such patches. Thanks. If there is a problem in the bugfix, please explain what the problem is. If you do not like the specific fix, you can fix it differently too or tell me what you prefer. Simply saying "no" with no explanation repeatedly is rude Patch breaks valid files. Can those files be added to FATE alongside a test? thank you [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
On Wed, Aug 9, 2023 at 9:30 PM Michael Niedermayer wrote: > Hi Paul > > On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > > This is not correct, and please stop writing such patches. Thanks. > > If there is a problem in the bugfix, please explain what the problem is. > If you do not like the specific fix, you can fix it differently too or > tell me what you prefer. > Simply saying "no" with no explanation repeatedly is rude > Patch breaks valid files. > > thank you > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > I have often repented speaking, but never of holding my tongue. > -- Xenocrates > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
Hi Paul On Wed, Aug 09, 2023 at 08:53:03PM +0200, Paul B Mahol wrote: > This is not correct, and please stop writing such patches. Thanks. If there is a problem in the bugfix, please explain what the problem is. If you do not like the specific fix, you can fix it differently too or tell me what you prefer. Simply saying "no" with no explanation repeatedly is rude thank you [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
This is not correct, and please stop writing such patches. Thanks. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v2] avcodec/mv30: Check the input length before allocation
Fixes: Timeout Fixes: 60867/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MV30_fuzzer-6381933108527104 Fixes: 30147/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MV30_fuzzer-5549246684200960 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/mv30.c | 5 + 1 file changed, 5 insertions(+) diff --git a/libavcodec/mv30.c b/libavcodec/mv30.c index 0b19534b00..addf188c92 100644 --- a/libavcodec/mv30.c +++ b/libavcodec/mv30.c @@ -411,6 +411,8 @@ static int decode_intra(AVCodecContext *avctx, GetBitContext *gb, AVFrame *frame mgb = *gb; if (get_bits_left(gb) < s->mode_size * 8) return AVERROR_INVALIDDATA; +if (s->mode_size * 8 < (avctx->height + 15)/16 * ((avctx->width + 15)/16) * 12) +return AVERROR_INVALIDDATA; skip_bits_long(gb, s->mode_size * 8); @@ -476,6 +478,9 @@ static int decode_inter(AVCodecContext *avctx, GetBitContext *gb, int ret, cnt = 0; int flags = 0; +if (get_bits_left(gb) < (mask_size + s->mode_size) * 8) +return AVERROR_INVALIDDATA; + if ((ret = ff_get_buffer(avctx, frame, AV_GET_BUFFER_FLAG_REF)) < 0) return ret; -- 2.17.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
