From e4db74746a195b4688b5c40d0cb2da5abccc224a Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 31 Jan 2020 15:25:05 +0000 Subject: [PATCH 1/2] fix a deadlock with --vips-leak We were usingh a global lock for metadata changes, but some functions triggered from callbacks in the metadata hash table could also attempt to acquire the same mutex, leading to deadlock. This patch gives metadata change it's own lock. Thanks DarthSim. See https://github.com/libvips/libvips/issues/1542 --- ChangeLog | 3 +++ configure.ac | 6 ++--- libvips/include/vips/private.h | 2 ++ libvips/iofuncs/header.c | 43 +++++++++++++++++----------------- libvips/iofuncs/init.c | 1 + 5 files changed, 31 insertions(+), 24 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9c168622..40a955be 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,6 @@ +31/1/19 started 8.9.2 +- fix a deadlock with --vips-leak [DarthSim] + 20/6/19 started 8.9.1 - don't use the new source loaders for new_from_file or new_from_buffer, it will break the loader priority system diff --git a/configure.ac b/configure.ac index 43d0c85a..98173c27 100644 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ # also update the version number in the m4 macros below -AC_INIT([vips], [8.9.1], [vipsip@jiscmail.ac.uk]) +AC_INIT([vips], [8.9.2], [vipsip@jiscmail.ac.uk]) # required for gobject-introspection AC_PREREQ(2.62) @@ -18,7 +18,7 @@ AC_CONFIG_MACRO_DIR([m4]) # user-visible library versioning m4_define([vips_major_version], [8]) m4_define([vips_minor_version], [9]) -m4_define([vips_micro_version], [1]) +m4_define([vips_micro_version], [2]) m4_define([vips_version], [vips_major_version.vips_minor_version.vips_micro_version]) @@ -38,7 +38,7 @@ VIPS_VERSION_STRING=$VIPS_VERSION-`date -u -r ChangeLog` # binary interface changes not backwards compatible?: reset age to 0 LIBRARY_CURRENT=54 -LIBRARY_REVISION=1 +LIBRARY_REVISION=2 LIBRARY_AGE=12 # patched into include/vips/version.h diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index e28735fc..8716cf36 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -192,6 +192,8 @@ int vips__view_image( struct _VipsImage *image ); */ extern int _vips__argument_id; +void vips__meta_init( void ); + #ifdef __cplusplus } #endif /*__cplusplus*/ diff --git a/libvips/iofuncs/header.c b/libvips/iofuncs/header.c index 6a6347a3..dba1c767 100644 --- a/libvips/iofuncs/header.c +++ b/libvips/iofuncs/header.c @@ -36,6 +36,8 @@ * - add vips_image_get_n_pages() * 20/6/19 * - add vips_image_get/set_array_int() + * 31/1/19 + * - lock for metadata changes */ /* @@ -130,6 +132,11 @@ * these types, it can be copied between images efficiently. */ +/* Use in various small places where we need a mutex and it's not worth + * making a private one. + */ +static GMutex *vips__meta_lock = NULL; + /* We have to keep the gtype as a string, since we statically init this. */ typedef struct _HeaderField { @@ -934,10 +941,10 @@ meta_cp( VipsImage *dst, const VipsImage *src ) /* We lock with vips_image_set() to stop races in highly- * threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); vips_slist_map2( src->meta_traverse, (VipsSListMap2Fn) meta_cp_field, dst, NULL ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); } return( 0 ); @@ -1025,13 +1032,6 @@ vips_image_set( VipsImage *image, const char *name, GValue *value ) g_assert( name ); g_assert( value ); - /* If this image is shared, block metadata changes. - */ - if( G_OBJECT( image )->ref_count > 1 ) { - g_warning( "can't set metadata \"%s\" on shared image", name ); - return; - } - meta_init( image ); /* We lock between modifying metadata and copying metadata between @@ -1041,9 +1041,9 @@ vips_image_set( VipsImage *image, const char *name, GValue *value ) * metadata copy on another -- this can lead to crashes in * highly-threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); (void) meta_new( image, name, value ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); /* If we're setting an EXIF data block, we need to automatically expand * out all the tags. This will set things like xres/yres too. @@ -1240,14 +1240,6 @@ vips_image_remove( VipsImage *image, const char *name ) result = FALSE; - /* If this image is shared, block metadata changes. - */ - if( G_OBJECT( image )->ref_count > 1 ) { - g_warning( "can't remove metadata \"%s\" on shared image", - name ); - return( result ); - } - if( image->meta ) { /* We lock between modifying metadata and copying metadata * between images, see meta_cp(). @@ -1256,9 +1248,9 @@ vips_image_remove( VipsImage *image, const char *name ) * racing with metadata copy on another -- this can lead to * crashes in highly-threaded applications. */ - g_mutex_lock( vips__global_lock ); + g_mutex_lock( vips__meta_lock ); result = g_hash_table_remove( image->meta, name ); - g_mutex_unlock( vips__global_lock ); + g_mutex_unlock( vips__meta_lock ); } return( result ); @@ -2027,3 +2019,12 @@ vips_image_get_history( VipsImage *image ) return( image->Hist ? image->Hist : "" ); } + +/* Called during vips_init(). + */ +void +vips__meta_init( void ) +{ + if( !vips__meta_lock ) + vips__meta_lock = vips_g_mutex_new(); +} diff --git a/libvips/iofuncs/init.c b/libvips/iofuncs/init.c index bd5a90cc..20ed972d 100644 --- a/libvips/iofuncs/init.c +++ b/libvips/iofuncs/init.c @@ -382,6 +382,7 @@ vips_init( const char *argv0 ) vips__threadpool_init(); vips__buffer_init(); + vips__meta_init(); /* This does an unsynchronised static hash table init on first call -- * we have to make sure we do this single-threaded. See: From 8a21f6ea52ed3d2aac68b05f4e502a66d266c153 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 31 Jan 2020 15:51:44 +0000 Subject: [PATCH 2/2] fix gif rendering for "waterfall.gif" This GIF has dispose set to DISPOSAL_UNSPECIFIED and seems to mean transparent. This patch makes gifload use DISPOSAL_UNSPECIFIED as well as _DO_NOT to mean reuse previous frame. Thanks DarthSim. See https://github.com/libvips/libvips/issues/1543 --- ChangeLog | 1 + libvips/foreign/gifload.c | 30 +++++++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 40a955be..1f8ddd3c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 31/1/19 started 8.9.2 - fix a deadlock with --vips-leak [DarthSim] +- better gifload behaviour for DISPOSAL_UNSPECIFIED [DarthSim] 20/6/19 started 8.9.1 - don't use the new source loaders for new_from_file or new_from_buffer, it diff --git a/libvips/foreign/gifload.c b/libvips/foreign/gifload.c index 0ff2273b..367c07d2 100644 --- a/libvips/foreign/gifload.c +++ b/libvips/foreign/gifload.c @@ -34,6 +34,9 @@ * - check image and frame bounds, since giflib does not * 1/9/19 * - improve early close again + * 31/1/20 + * - treat DISPOSAL_UNSPECIFIED as _DO_NOT, since that's what many GIFs + * in the wild appear to do */ /* @@ -852,8 +855,12 @@ vips_foreign_load_gif_render_line( VipsForeignLoadGif *gif, /* In DISPOSE_DO_NOT mode, the previous frame shows * through (ie. we do nothing). In all other modes, * it's just transparent. + * + * Many GIFs use DISPOSAL_UNSPECIFIED to mean DO_NOT, + * so use that for previous frame as well. */ - if( gif->dispose != DISPOSE_DO_NOT ) + if( gif->dispose != DISPOSE_DO_NOT && + gif->dispose != DISPOSAL_UNSPECIFIED ) iq[x] = 0; } else @@ -976,6 +983,20 @@ vips_foreign_load_gif_render( VipsForeignLoadGif *gif ) return( 0 ); } +#ifdef VIPS_DEBUG +static const char * +dispose2str( int dispose ) +{ + switch( dispose ) { + case DISPOSAL_UNSPECIFIED: return( "DISPOSAL_UNSPECIFIED" ); + case DISPOSE_DO_NOT: return( "DISPOSE_DO_NOT" ); + case DISPOSE_BACKGROUND: return( "DISPOSE_BACKGROUND" ); + case DISPOSE_PREVIOUS: return( "DISPOSE_PREVIOUS" ); + default: return( "" ); + } +} +#endif /*VIPS_DEBUG*/ + static int vips_foreign_load_gif_extension( VipsForeignLoadGif *gif ) { @@ -998,15 +1019,18 @@ vips_foreign_load_gif_extension( VipsForeignLoadGif *gif ) * is being set. */ gif->transparency = -1; - if( extension[1] & 0x1 ) + if( extension[1] & 0x1 ) { gif->transparency = extension[4]; + VIPS_DEBUG_MSG( "vips_foreign_load_gif_extension: " + "transparency = %d\n", gif->transparency ); + } /* Set the current dispose mode. This is read during frame load * to set the meaning of background and transparent pixels. */ gif->dispose = (extension[1] >> 2) & 0x7; VIPS_DEBUG_MSG( "vips_foreign_load_gif_extension: " - "dispose = %d\n", gif->dispose ); + "dispose = %s\n", dispose2str( gif->dispose ) ); } while( extension != NULL )