| Message ID | 20221122225652.1341-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/pictordec: Check that the image fits in the input | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, Nov 22, 2022 at 11:56:51PM +0100, Michael Niedermayer wrote: > Fixes: Timeout > Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/pictordec.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c > index 71bad40a0a..09229b94fd 100644 > --- a/libavcodec/pictordec.c > +++ b/libavcodec/pictordec.c > @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, > > if (av_image_check_size(s->width, s->height, 0, avctx) < 0) > return -1; > + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) > + return AVERROR_INVALIDDATA; how did you arrive at this formula? -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
t On Fri, Nov 25, 2022 at 05:45:29PM +1100, Peter Ross wrote: > On Tue, Nov 22, 2022 at 11:56:51PM +0100, Michael Niedermayer wrote: > > Fixes: Timeout > > Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/pictordec.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c > > index 71bad40a0a..09229b94fd 100644 > > --- a/libavcodec/pictordec.c > > +++ b/libavcodec/pictordec.c > > @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, > > > > if (av_image_check_size(s->width, s->height, 0, avctx) < 0) > > return -1; > > + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) > > + return AVERROR_INVALIDDATA; > > how did you arrive at this formula? There are 2 coding modes, RLE and RAW I assume usable raw images will need around W*H and thus more than RLE RLE codes the most compressed runs by 1 byte for val (=marker) 1 byte run (=0) 2 bytes run 1 byte val thats 5 bytes and the maximum run we can code is 65535 The RLE decoder loop exits before applying the last RLE run and then there is a seperate piece of code after it that fills the last color to the end. Iam not sure why its done like that way but if i remove that mid exit the seperate code piece becomes unused for all images i have so it seems all RLE images are always fully coded with no special case at the end. Based on this iam guesing that my formula is correct for undamaged images but of course i could find one tomorrow that exploits the special end handling and breaks this formula and of course its very possible that i missed some other thing that changes this limit thx [...]
Quoting Michael Niedermayer (2022-11-25 14:38:46) > t On Fri, Nov 25, 2022 at 05:45:29PM +1100, Peter Ross wrote: > > On Tue, Nov 22, 2022 at 11:56:51PM +0100, Michael Niedermayer wrote: > > > Fixes: Timeout > > > Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavcodec/pictordec.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c > > > index 71bad40a0a..09229b94fd 100644 > > > --- a/libavcodec/pictordec.c > > > +++ b/libavcodec/pictordec.c > > > @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, > > > > > > if (av_image_check_size(s->width, s->height, 0, avctx) < 0) > > > return -1; > > > + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) > > > + return AVERROR_INVALIDDATA; > > > > how did you arrive at this formula? > > There are 2 coding modes, RLE and RAW > I assume usable raw images will need around W*H and thus more than RLE > RLE codes the most compressed runs by > 1 byte for val (=marker) > 1 byte run (=0) > 2 bytes run > 1 byte val > thats 5 bytes and the maximum run we can code is 65535 > > The RLE decoder loop exits before applying the last RLE run and then > there is a seperate piece of code after it that fills the last color to > the end. Iam not sure why its done like that way but if i remove that > mid exit the seperate code piece becomes unused for all images i have > so it seems all RLE images are always fully coded with no special case > at the end. > Based on this iam guesing that my formula is correct for undamaged images > but of course i could find one tomorrow that exploits the special end > handling and breaks this formula > and of course its very possible that i missed some other thing that changes > this limit This should be written in a comment above the code then, otherwise all future readers will be completely clueless.
On Fri, Nov 25, 2022 at 03:11:32PM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2022-11-25 14:38:46) > > t On Fri, Nov 25, 2022 at 05:45:29PM +1100, Peter Ross wrote: > > > On Tue, Nov 22, 2022 at 11:56:51PM +0100, Michael Niedermayer wrote: > > > > Fixes: Timeout > > > > Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/pictordec.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c > > > > index 71bad40a0a..09229b94fd 100644 > > > > --- a/libavcodec/pictordec.c > > > > +++ b/libavcodec/pictordec.c > > > > @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, > > > > > > > > if (av_image_check_size(s->width, s->height, 0, avctx) < 0) > > > > return -1; > > > > + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) > > > > + return AVERROR_INVALIDDATA; > > > > > > how did you arrive at this formula? > > > > There are 2 coding modes, RLE and RAW > > I assume usable raw images will need around W*H and thus more than RLE > > RLE codes the most compressed runs by > > 1 byte for val (=marker) > > 1 byte run (=0) > > 2 bytes run > > 1 byte val > > thats 5 bytes and the maximum run we can code is 65535 > > > > The RLE decoder loop exits before applying the last RLE run and then > > there is a seperate piece of code after it that fills the last color to > > the end. Iam not sure why its done like that way but if i remove that > > mid exit the seperate code piece becomes unused for all images i have > > so it seems all RLE images are always fully coded with no special case > > at the end. > > Based on this iam guesing that my formula is correct for undamaged images > > but of course i could find one tomorrow that exploits the special end > > handling and breaks this formula > > and of course its very possible that i missed some other thing that changes > > this limit > > This should be written in a comment above the code then, otherwise all > future readers will be completely clueless. good idea. ok, patch looks good. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
On Sat, Nov 26, 2022 at 10:42:30AM +1100, Peter Ross wrote: > On Fri, Nov 25, 2022 at 03:11:32PM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2022-11-25 14:38:46) > > > t On Fri, Nov 25, 2022 at 05:45:29PM +1100, Peter Ross wrote: > > > > On Tue, Nov 22, 2022 at 11:56:51PM +0100, Michael Niedermayer wrote: > > > > > Fixes: Timeout > > > > > Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavcodec/pictordec.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c > > > > > index 71bad40a0a..09229b94fd 100644 > > > > > --- a/libavcodec/pictordec.c > > > > > +++ b/libavcodec/pictordec.c > > > > > @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, > > > > > > > > > > if (av_image_check_size(s->width, s->height, 0, avctx) < 0) > > > > > return -1; > > > > > + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) > > > > > + return AVERROR_INVALIDDATA; > > > > > > > > how did you arrive at this formula? > > > > > > There are 2 coding modes, RLE and RAW > > > I assume usable raw images will need around W*H and thus more than RLE > > > RLE codes the most compressed runs by > > > 1 byte for val (=marker) > > > 1 byte run (=0) > > > 2 bytes run > > > 1 byte val > > > thats 5 bytes and the maximum run we can code is 65535 > > > > > > The RLE decoder loop exits before applying the last RLE run and then > > > there is a seperate piece of code after it that fills the last color to > > > the end. Iam not sure why its done like that way but if i remove that > > > mid exit the seperate code piece becomes unused for all images i have > > > so it seems all RLE images are always fully coded with no special case > > > at the end. > > > Based on this iam guesing that my formula is correct for undamaged images > > > but of course i could find one tomorrow that exploits the special end > > > handling and breaks this formula > > > and of course its very possible that i missed some other thing that changes > > > this limit > > > > This should be written in a comment above the code then, otherwise all > > future readers will be completely clueless. > > good idea. ok, patch looks good. ok will apply, i need to reword this comment a bit as the code changed already and the mid exit no longer exists thx [...]
diff --git a/libavcodec/pictordec.c b/libavcodec/pictordec.c index 71bad40a0a..09229b94fd 100644 --- a/libavcodec/pictordec.c +++ b/libavcodec/pictordec.c @@ -162,6 +162,9 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *frame, if (av_image_check_size(s->width, s->height, 0, avctx) < 0) return -1; + if (bytestream2_get_bytes_left(&s->g) < s->width * s->height / 65536 * 5) + return AVERROR_INVALIDDATA; + if (s->width != avctx->width || s->height != avctx->height) { ret = ff_set_dimensions(avctx, s->width, s->height); if (ret < 0)
Fixes: Timeout Fixes: 53438/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PICTOR_fuzzer-5458939919859712 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpe Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/pictordec.c | 3 +++ 1 file changed, 3 insertions(+)