diff mbox series

[FFmpeg-devel,4/4] avcodec/tiff: Use ff_set_dimensions() for setting up mjpeg context dimensions

Message ID 20211217215155.19805-4-michael@niedermayer.cc
State Accepted
Commit cfa1f0e214d07f0fdc027f2ec760eb9fd3fac85e
Headers show
Series [FFmpeg-devel,1/4] avcodec/ass: Faster ff_ass_add_rect() | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Michael Niedermayer Dec. 17, 2021, 9:51 p.m. UTC 
Fixes: OOM
Fixes: 42263/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-5653333619113984

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

Comments

Anton Khirnov Dec. 20, 2021, 9:51 a.m. UTC | #1 
Quoting Michael Niedermayer (2021-12-17 22:51:55)
> Fixes: OOM

This is very non-obvious and could use more explanation.
Michael Niedermayer Dec. 20, 2021, 10:50 a.m. UTC | #2 
On Mon, Dec 20, 2021 at 10:51:56AM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-12-17 22:51:55)
> > Fixes: OOM
> 
> This is very non-obvious and could use more explanation.

I guess its obvious to me as ive seen this bug more than once
the problem is that directly setting width/height leaves
coded_w/h unset, then something sets coded_w/h and next time
width/height is set again it is unrelated to the still set
coded_w/h and theres a FFMAX() between coded_w and width
so the image allocated is much bigger
ff_set_dimension() breaks this chain by setting both width and
coded_width 

should i add this long explanation above to the commit message
or something shorter like
"sets coded_width / coded_height too to keep them consistent with
 width / height"
 
thx

[...]
Anton Khirnov Dec. 20, 2021, 2:32 p.m. UTC | #3 
Quoting Michael Niedermayer (2021-12-20 11:50:21)
> On Mon, Dec 20, 2021 at 10:51:56AM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-12-17 22:51:55)
> > > Fixes: OOM
> > 
> > This is very non-obvious and could use more explanation.
> 
> I guess its obvious to me as ive seen this bug more than once
> the problem is that directly setting width/height leaves
> coded_w/h unset, then something sets coded_w/h and next time
> width/height is set again it is unrelated to the still set
> coded_w/h and theres a FFMAX() between coded_w and width
> so the image allocated is much bigger
> ff_set_dimension() breaks this chain by setting both width and
> coded_width 
> 
> should i add this long explanation above to the commit message
> or something shorter like
> "sets coded_width / coded_height too to keep them consistent with
>  width / height"

Either is fine with me.
Michael Niedermayer Dec. 23, 2021, 1:56 p.m. UTC | #4 
On Mon, Dec 20, 2021 at 03:32:05PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-12-20 11:50:21)
> > On Mon, Dec 20, 2021 at 10:51:56AM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2021-12-17 22:51:55)
> > > > Fixes: OOM
> > > 
> > > This is very non-obvious and could use more explanation.
> > 
> > I guess its obvious to me as ive seen this bug more than once
> > the problem is that directly setting width/height leaves
> > coded_w/h unset, then something sets coded_w/h and next time
> > width/height is set again it is unrelated to the still set
> > coded_w/h and theres a FFMAX() between coded_w and width
> > so the image allocated is much bigger
> > ff_set_dimension() breaks this chain by setting both width and
> > coded_width 
> > 
> > should i add this long explanation above to the commit message
> > or something shorter like
> > "sets coded_width / coded_height too to keep them consistent with
> >  width / height"
> 
> Either is fine with me.

ok will use the shorter and apply the 2 tiff patches

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index 9af602eef70..60773d59ed0 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -724,13 +724,14 @@  static int dng_decode_jpeg(AVCodecContext *avctx, AVFrame *frame,
 static int dng_decode_strip(AVCodecContext *avctx, AVFrame *frame)
 {
     TiffContext *s = avctx->priv_data;
+    int ret = ff_set_dimensions(s->avctx_mjpeg, s->width, s->height);
+
+    if (ret < 0)
+        return ret;
 
     s->jpgframe->width  = s->width;
     s->jpgframe->height = s->height;
 
-    s->avctx_mjpeg->width = s->width;
-    s->avctx_mjpeg->height = s->height;
-
     return dng_decode_jpeg(avctx, frame, s->stripsize, 0, 0, s->width, s->height);
 }
 
@@ -971,14 +972,14 @@  static int dng_decode_tiles(AVCodecContext *avctx, AVFrame *frame,
     int has_width_leftover, has_height_leftover;
     int tile_x = 0, tile_y = 0;
     int pos_x = 0, pos_y = 0;
-    int ret;
+    int ret = ff_set_dimensions(s->avctx_mjpeg, s->tile_width, s->tile_length);
+
+    if (ret < 0)
+        return ret;
 
     s->jpgframe->width  = s->tile_width;
     s->jpgframe->height = s->tile_length;
 
-    s->avctx_mjpeg->width = s->tile_width;
-    s->avctx_mjpeg->height = s->tile_length;
-
     has_width_leftover = (s->width % s->tile_width != 0);
     has_height_leftover = (s->height % s->tile_length != 0);