From e4d38d1d3e0cf43b2f7b6596f0aa70ebb8410577 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 2 Jul 2020 16:41:37 +0100 Subject: [PATCH] clip out of bounds GIF images against the canvas Some malformed GIFs have images which lie outside or partly outside the canvas. With this patch, these frames are clipped and rendered. Previously, these GIFs were rejected. See https://github.com/libvips/libvips/issues/1701 --- ChangeLog | 1 + README.md | 45 ----------- libvips/foreign/gifload.c | 152 ++++++++++++++++++++++---------------- 3 files changed, 90 insertions(+), 108 deletions(-) diff --git a/ChangeLog b/ChangeLog index c6c33b3b..1f136816 100644 --- a/ChangeLog +++ b/ChangeLog @@ -39,6 +39,7 @@ - rework the final bits of vips7 for vips8 [kleisauke] - --disable-deprecated now works [kleisauke] - vipsheader allows "stdin" as a filename +- gifload clips out of bounds images against the canvas 24/4/20 started 8.9.3 - better iiif tile naming [IllyaMoskvin] diff --git a/README.md b/README.md index 89ae80a5..2e48c31f 100644 --- a/README.md +++ b/README.md @@ -4,51 +4,6 @@ [![Fuzzing Status](https://oss-fuzz-build-logs.storage.googleapis.com/badges/libvips.svg)](https://bugs.chromium.org/p/oss-fuzz/issues/list?sort=-opened&can=2&q=proj:libvips) [![Coverity Status](https://scan.coverity.com/projects/6503/badge.svg)](https://scan.coverity.com/projects/jcupitt-libvips) -# This branch - -Is for experiemtning with [libspng](https://github.com/randy408/libspng). - -## Notes - -Build libspng: - -``` -cd libspng -meson build --prefix=/home/john/vips --libdir=/home/john/vips/lib \ - --buildtype=release -cd build -ninja -ninja install -``` - -Installs `spng.pc`. - -Sample code: - -https://github.com/randy408/libspng/blob/master/examples/example.c - -libspng benchmark: - -``` -$ time vips avg wtc.png -117.065766 - -real 0m2.972s -user 0m3.376s -sys 0m0.197s -``` - -And for libpng: - -``` -$ time vips avg wtc.png -117.065766 - -real 0m3.816s -user 0m4.177s -sys 0m0.221s -``` - # Introduction libvips is a [demand-driven, horizontally diff --git a/libvips/foreign/gifload.c b/libvips/foreign/gifload.c index 412b2f31..724ade75 100644 --- a/libvips/foreign/gifload.c +++ b/libvips/foreign/gifload.c @@ -39,6 +39,8 @@ * - add gifload_source * 5/2/20 alon-ne * - fix DISPOSE_BACKGROUND and DISPOSE_PREVIOUS + * 2/7/20 + * - clip out of bounds images against canvas */ /* @@ -177,7 +179,8 @@ typedef struct _VipsForeignLoadGif { */ VipsImage *frame; - /* A scratch buffer the size of frame, used for rendering. + /* A scratch buffer the size of the largest Image inside the GIF. We + * decompress lines from the GIF to this. */ VipsImage *scratch; @@ -189,8 +192,10 @@ typedef struct _VipsForeignLoadGif { */ int current_page; - /* Decompress lines of the gif file to here. + /* Decompress lines of the gif file to here. This is large enough to + * hold one line of the widest sub-image in the GIF. */ + int max_image_width; GifPixelType *line; /* The current dispose method. @@ -400,6 +405,7 @@ vips_foreign_load_gif_open_giflib( VipsForeignLoadGif *gif ) gif->eof = FALSE; gif->current_page = 0; + gif->max_image_width = 0; return( 0 ); } @@ -478,8 +484,10 @@ vips_foreign_load_gif_ext_next( VipsForeignLoadGif *gif, return( -1 ); } +#ifdef DEBUG_VERBOSE if( *extension ) - VIPS_DEBUG_MSG( "gifload: EXTENSION_NEXT\n" ); + printf( "gifload: EXTENSION_NEXT\n" ); +#endif /*DEBUG_VERBOSE*/ return( 0 ); } @@ -493,8 +501,10 @@ vips_foreign_load_gif_code_next( VipsForeignLoadGif *gif, return( -1 ); } +#ifdef DEBUG_VERBOSE if( *extension ) - VIPS_DEBUG_MSG( "gifload: CODE_NEXT\n" ); + printf( "gifload: CODE_NEXT\n" ); +#endif /*DEBUG_VERBOSE*/ return( 0 ); } @@ -510,26 +520,33 @@ vips_foreign_load_gif_scan_image( VipsForeignLoadGif *gif ) ColorMapObject *map; GifByteType *extension; - if( DGifGetImageDesc( gif->file ) == GIF_ERROR ) { + if( DGifGetImageDesc( file ) == GIF_ERROR ) { vips_foreign_load_gif_error( gif ); return( -1 ); } - /* Check that the frame looks sane. Perhaps giflib checks - * this for us. + VIPS_DEBUG_MSG( "vips_foreign_load_gif_scan_image: " + "frame of %dx%d pixels at %dx%d\n", + file->Image.Width, file->Image.Height, + file->Image.Left, file->Image.Top ); + + /* giflib does no checking of image dimensions, not even for 0. */ - if( file->Image.Left < 0 || - file->Image.Width < 1 || - file->Image.Width > 10000 || - file->Image.Left + file->Image.Width > file->SWidth || - file->Image.Top < 0 || - file->Image.Height < 1 || - file->Image.Height > 10000 || - file->Image.Top + file->Image.Height > file->SHeight ) { - vips_error( class->nickname, "%s", _( "bad frame size" ) ); + if( file->Image.Width <= 0 || + file->Image.Width > VIPS_MAX_COORD || + file->Image.Height <= 0 || + file->Image.Height > VIPS_MAX_COORD ) { + vips_error( class->nickname, + "%s", _( "image size out of bounds" ) ); return( -1 ); } + /* We need to find the max scanline size inside the GIF + * so we can allocate the decompress buffer. + */ + gif->max_image_width = VIPS_MAX( gif->max_image_width, + file->Image.Width ); + /* Test for a non-greyscale colourmap for this frame. */ map = file->Image.ColorMap ? file->Image.ColorMap : file->SColorMap; @@ -713,7 +730,7 @@ vips_foreign_load_gif_set_header( VipsForeignLoadGif *gif, VipsImage *image ) } /* Attempt to quickly scan a GIF and discover what we need for our header. We - * need to scan the whole file to get n_pages, transparency and colour. + * need to scan the whole file to get n_pages, transparency, colour etc. * * Don't flag errors during header scan. Many GIFs do not follow spec. */ @@ -769,6 +786,12 @@ vips_foreign_load_gif_scan( VipsForeignLoadGif *gif ) return( -1 ); } + if( gif->max_image_width <= 0 || + gif->max_image_width > VIPS_MAX_COORD ) { + vips_error( class->nickname, "%s", _( "bad image size" ) ); + return( -1 ); + } + return( 0 ); } @@ -801,9 +824,9 @@ vips_foreign_load_gif_header( VipsForeignLoad *load ) /* Allocate a line buffer now that we have the GIF width. */ - if( !(gif->line = - VIPS_ARRAY( NULL, gif->file->SWidth, GifPixelType )) || - vips_foreign_load_gif_scan( gif ) || + if( vips_foreign_load_gif_scan( gif ) || + !(gif->line = VIPS_ARRAY( NULL, + gif->max_image_width, GifPixelType )) || vips_foreign_load_gif_set_header( gif, load->out ) ) { (void) vips_foreign_load_gif_close_giflib( gif ); @@ -844,19 +867,46 @@ vips_foreign_load_gif_build_cmap( VipsForeignLoadGif *gif ) } } +/* Paint line y from the image left/top/width/height into scratch, clipping as + * we go. + */ static void vips_foreign_load_gif_render_line( VipsForeignLoadGif *gif, - int width, VipsPel * restrict dst, VipsPel * restrict src ) + int y, + int left, int top, int width, int height, + VipsPel *line ) { - guint32 * restrict idst = (guint32 *) dst; + VipsRect canvas; + VipsRect row; + VipsRect overlap; - int x; + /* Many GIFs have frames which lie outside the canvas. We have to + * clip. + */ + canvas.left = 0; + canvas.top = 0; + canvas.width = gif->file->SWidth; + canvas.height = gif->file->SHeight; + row.left = left; + row.top = top + y; + row.width = width; + row.height = height; + vips_rect_intersectrect( &canvas, &row, &overlap ); - for( x = 0; x < width; x++ ) { - VipsPel v = src[x]; + if( !vips_rect_isempty( &overlap ) ) { + VipsPel *dst = VIPS_IMAGE_ADDR( gif->scratch, + overlap.left, overlap.top ); + guint32 * restrict idst = (guint32 *) dst; + VipsPel * restrict src = line + overlap.left - row.left; - if( v != gif->transparent_index ) - idst[x] = gif->cmap[v]; + int x; + + for( x = 0; x < overlap.width; x++ ) { + VipsPel v = src[x]; + + if( v != gif->transparent_index ) + idst[x] = gif->cmap[v]; + } } } @@ -873,32 +923,6 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) return( -1 ); } - /* giflib does not check that the Left / Top / Width / Height for this - * Image is inside the canvas. - * - * We could clip against the canvas, but for now, just ignore out of - * bounds frames. Watch for int overflow too. - */ - if( file->Image.Left < 0 || - file->Image.Left > VIPS_MAX_COORD || - file->Image.Width <= 0 || - file->Image.Width > VIPS_MAX_COORD || - file->Image.Left + file->Image.Width > file->SWidth || - file->Image.Top < 0 || - file->Image.Top > VIPS_MAX_COORD || - file->Image.Height <= 0 || - file->Image.Height > VIPS_MAX_COORD || - file->Image.Top + file->Image.Height > file->SHeight ) { - VIPS_DEBUG_MSG( "vips_foreign_load_gif_render: " - "out of bounds frame of %d x %d pixels at %d x %d\n", - file->Image.Width, file->Image.Height, - file->Image.Left, file->Image.Top ); - - /* Don't flag an error -- many GIFs have this problem. - */ - return( 0 ); - } - /* Update the colour map for this frame. */ vips_foreign_load_gif_build_cmap( gif ); @@ -925,9 +949,6 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) for( y = InterlacedOffset[i]; y < file->Image.Height; y += InterlacedJumps[i] ) { - VipsPel *dst = VIPS_IMAGE_ADDR( gif->scratch, - file->Image.Left, file->Image.Top + y ); - if( DGifGetLine( gif->file, gif->line, file->Image.Width ) == GIF_ERROR ) { @@ -935,8 +956,12 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) return( -1 ); } - vips_foreign_load_gif_render_line( gif, - file->Image.Width, dst, gif->line ); + vips_foreign_load_gif_render_line( gif, y, + file->Image.Left, + file->Image.Top, + file->Image.Width, + file->Image.Height, + gif->line ); } } } @@ -949,17 +974,18 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) file->Image.Left, file->Image.Top ); for( y = 0; y < file->Image.Height; y++ ) { - VipsPel *dst = VIPS_IMAGE_ADDR( gif->scratch, - file->Image.Left, file->Image.Top + y ); - if( DGifGetLine( gif->file, gif->line, file->Image.Width ) == GIF_ERROR ) { vips_foreign_load_gif_error( gif ); return( -1 ); } - vips_foreign_load_gif_render_line( gif, - file->Image.Width, dst, gif->line ); + vips_foreign_load_gif_render_line( gif, y, + file->Image.Left, + file->Image.Top, + file->Image.Width, + file->Image.Height, + gif->line ); } }