From 1619c8b1a1c0a1e2c2d853e6d01671f712466dbb Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 4 Jan 2019 08:38:58 +0000 Subject: [PATCH 1/3] fix memleak in magick6load IM ExceptionInfo were not being freed correctly. This patch adds a small wrapper function and uses it to allocate and free all IM exception objects. Tested with im 6.9 and gm 1.3. See: https://github.com/libvips/lua-vips/issues/24 https://github.com/libvips/libvips/issues/1203 --- ChangeLog | 1 + configure.ac | 10 +++++++ libvips/foreign/magick.c | 52 +++++++++++++++++++++++++++++++++-- libvips/foreign/magick.h | 3 ++ libvips/foreign/magick2vips.c | 24 ++++++++-------- libvips/foreign/magick7load.c | 12 ++++---- libvips/foreign/magicksave.c | 4 +-- 7 files changed, 83 insertions(+), 23 deletions(-) diff --git a/ChangeLog b/ChangeLog index fd154a09..324a00e0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,7 @@ - fix infinite loop for autofit with non-scaleable font - mapim was not offsetting by window offset [erdmann] - better rounding for scale [kleisauke] +- fix a memleak in magick6load [kleisauke] 21/11/18 started 8.7.2 - more info output for temp files to help diagnose problems diff --git a/configure.ac b/configure.ac index 250feba1..f5e15cf2 100644 --- a/configure.ac +++ b/configure.ac @@ -762,6 +762,16 @@ if test x"$magick6" = x"yes"; then LIBS="$save_LIBS" fi +if test x"$magick6" = x"yes"; then + # GM is missing AcquireExceptionInfo + save_LIBS="$LIBS" + LIBS="$LIBS $MAGICK_LIBS" + AC_CHECK_FUNCS(AcquireExceptionInfo, + AC_DEFINE(HAVE_ACQUIREEXCEPTIONINFO,1, + [define if your magick has AcquireExceptionInfo.])) + LIBS="$save_LIBS" +fi + # have flags to turn load and save off independently ... some people will want # save but not load, for example AC_ARG_ENABLE([magickload], diff --git a/libvips/foreign/magick.c b/libvips/foreign/magick.c index 654c6418..65bf9546 100644 --- a/libvips/foreign/magick.c +++ b/libvips/foreign/magick.c @@ -85,6 +85,18 @@ magick_set_property( Image *image, const char *property, const char *value, (void) SetImageProperty( image, property, value, exception ); } +ExceptionInfo * +magick_acquire_exception( void ) +{ + return( AcquireExceptionInfo() ); +} + +void +magick_destroy_exception( ExceptionInfo *exception ) +{ + VIPS_FREEF( DestroyExceptionInfo, exception ); +} + void magick_inherit_exception( ExceptionInfo *exception, Image *image ) { @@ -197,12 +209,48 @@ magick_set_property( Image *image, const char *property, const char *value, #endif /*HAVE_SETIMAGEPROPERTY*/ } +ExceptionInfo * +magick_acquire_exception( void ) +{ + ExceptionInfo *exception; + +#ifdef HAVE_ACQUIREEXCEPTIONINFO + /* IM6+ + */ + exception = AcquireExceptionInfo(); +#else /*!HAVE_ACQUIREEXCEPTIONINFO*/ + /* gm + */ + exception = g_new( ExceptionInfo, 1 ); + GetExceptionInfo( exception ); +#endif /*HAVE_ACQUIREEXCEPTIONINFO*/ + + return( exception ); +} + +void +magick_destroy_exception( ExceptionInfo *exception ) +{ +#ifdef HAVE_ACQUIREEXCEPTIONINFO + /* IM6+ will free the exception in destroy. + */ + VIPS_FREEF( DestroyExceptionInfo, exception ); +#else /*!HAVE_ACQUIREEXCEPTIONINFO*/ + /* gm and very old IM need to free the memory too. + */ + if( exception ) { + DestroyExceptionInfo( exception ); + g_free( exception ); + } +#endif /*HAVE_ACQUIREEXCEPTIONINFO*/ +} + void magick_inherit_exception( ExceptionInfo *exception, Image *image ) { -#ifdef HAVE_INHERITEXCEPTION +#ifdef HAVE_INHERITEXCEPTIONINFO InheritException( exception, &image->exception ); -#endif /*HAVE_INHERITEXCEPTION*/ +#endif /*HAVE_INHERITEXCEPTIONINFO*/ } void diff --git a/libvips/foreign/magick.h b/libvips/foreign/magick.h index e95ea5f2..4cdb8c41 100644 --- a/libvips/foreign/magick.h +++ b/libvips/foreign/magick.h @@ -58,7 +58,10 @@ void magick_set_image_option( ImageInfo *image_info, void magick_set_number_scenes( ImageInfo *image_info, int scene, int number_scenes ); +ExceptionInfo *magick_acquire_exception( void ); +void magick_destroy_exception( ExceptionInfo *exception ); void magick_inherit_exception( ExceptionInfo *exception, Image *image ); + void magick_sniff_bytes( ImageInfo *image_info, const unsigned char *bytes, size_t length ); void magick_sniff_file( ImageInfo *image_info, const char *filename ); diff --git a/libvips/foreign/magick2vips.c b/libvips/foreign/magick2vips.c index 8c22c6c7..f3f311d7 100644 --- a/libvips/foreign/magick2vips.c +++ b/libvips/foreign/magick2vips.c @@ -137,7 +137,7 @@ typedef struct _Read { Image *image; ImageInfo *image_info; - ExceptionInfo exception; + ExceptionInfo *exception; /* Number of pages in image. */ @@ -168,9 +168,7 @@ read_free( Read *read ) VIPS_FREEF( DestroyImageList, read->image ); VIPS_FREEF( DestroyImageInfo, read->image_info ); VIPS_FREE( read->frames ); - if ( (&read->exception)->signature == MagickSignature ) { - DestroyExceptionInfo( &read->exception ); - } + VIPS_FREEF( magick_destroy_exception, read->exception ); VIPS_FREEF( vips_g_mutex_free, read->lock ); } @@ -209,7 +207,7 @@ read_new( const char *filename, VipsImage *im, read->im = im; read->image = NULL; read->image_info = CloneImageInfo( NULL ); - GetExceptionInfo( &read->exception ); + read->exception = magick_acquire_exception(); read->n_pages = 0; read->n_frames = 0; read->frames = NULL; @@ -765,9 +763,9 @@ vips__magick_read( const char *filename, printf( "magick2vips: calling ReadImage() ...\n" ); #endif /*DEBUG*/ - read->image = ReadImage( read->image_info, &read->exception ); + read->image = ReadImage( read->image_info, read->exception ); if( !read->image ) { - magick_vips_error( "magick2vips", &read->exception ); + magick_vips_error( "magick2vips", read->exception ); vips_error( "magick2vips", _( "unable to read file \"%s\"" ), filename ); return( -1 ); @@ -803,9 +801,9 @@ vips__magick_read_header( const char *filename, * but sadly many IM coders do not support ping. The critical one for * us is DICOM. TGA also has issues. */ - read->image = ReadImage( read->image_info, &read->exception ); + read->image = ReadImage( read->image_info, read->exception ); if( !read->image ) { - magick_vips_error( "magick2vips", &read->exception ); + magick_vips_error( "magick2vips", read->exception ); vips_error( "magick2vips", _( "unable to read file \"%s\"" ), filename ); return( -1 ); @@ -845,9 +843,9 @@ vips__magick_read_buffer( const void *buf, const size_t len, #endif /*DEBUG*/ read->image = BlobToImage( read->image_info, - buf, len, &read->exception ); + buf, len, read->exception ); if( !read->image ) { - magick_vips_error( "magick2vips", &read->exception ); + magick_vips_error( "magick2vips", read->exception ); vips_error( "magick2vips", "%s", _( "unable to read buffer" ) ); return( -1 ); } @@ -883,9 +881,9 @@ vips__magick_read_buffer_header( const void *buf, const size_t len, * for us is DICOM. TGA also has issues. */ read->image = BlobToImage( read->image_info, - buf, len, &read->exception ); + buf, len, read->exception ); if( !read->image ) { - magick_vips_error( "magick2vips", &read->exception ); + magick_vips_error( "magick2vips", read->exception ); vips_error( "magick2vips", "%s", _( "unable to ping blob" ) ); return( -1 ); } diff --git a/libvips/foreign/magick7load.c b/libvips/foreign/magick7load.c index 95f518a7..8351c41c 100644 --- a/libvips/foreign/magick7load.c +++ b/libvips/foreign/magick7load.c @@ -279,7 +279,7 @@ vips_foreign_load_magick7_dispose( GObject *gobject ) VIPS_FREEF( DestroyImageInfo, magick7->image_info ); VIPS_FREE( magick7->frames ); VIPS_FREE( magick7->cache_view ); - VIPS_FREEF( DestroyExceptionInfo, magick7->exception ); + VIPS_FREEF( magick_destroy_exception, magick7->exception ); VIPS_FREEF( vips_g_mutex_free, magick7->lock ); G_OBJECT_CLASS( vips_foreign_load_magick7_parent_class )-> @@ -298,7 +298,7 @@ vips_foreign_load_magick7_build( VipsObject *object ) magick_genesis(); magick7->image_info = CloneImageInfo( NULL ); - magick7->exception = AcquireExceptionInfo(); + magick7->exception = magick_acquire_exception(); magick7->lock = vips_g_mutex_new(); if( !magick7->image_info ) @@ -760,14 +760,14 @@ ismagick7( const char *filename ) /* Horribly slow :-( */ image_info = CloneImageInfo( NULL ); - exception = AcquireExceptionInfo(); + exception = magick_acquire_exception(); vips_strncpy( image_info->filename, filename, MagickPathExtent ); magick_sniff_file( image_info, filename ); image = PingImage( image_info, exception ); result = image != NULL; VIPS_FREEF( DestroyImageList, image ); VIPS_FREEF( DestroyImageInfo, image_info ); - VIPS_FREEF( DestroyExceptionInfo, exception ); + VIPS_FREEF( magick_destroy_exception, exception ); return( result ); } @@ -863,13 +863,13 @@ vips_foreign_load_magick7_buffer_is_a_buffer( const void *buf, size_t len ) /* Horribly slow :-( */ image_info = CloneImageInfo( NULL ); - exception = AcquireExceptionInfo(); + exception = magick_acquire_exception(); magick_sniff_bytes( image_info, buf, len ); image = PingBlob( image_info, buf, len, exception ); result = image != NULL; VIPS_FREEF( DestroyImageList, image ); VIPS_FREEF( DestroyImageInfo, image_info ); - VIPS_FREEF( DestroyExceptionInfo, exception ); + VIPS_FREEF( magick_destroy_exception, exception ); return( result ); } diff --git a/libvips/foreign/magicksave.c b/libvips/foreign/magicksave.c index bf1e8e43..2fc9f580 100644 --- a/libvips/foreign/magicksave.c +++ b/libvips/foreign/magicksave.c @@ -81,7 +81,7 @@ vips_foreign_save_magick_dispose( GObject *gobject ) VIPS_FREE( magick->map ); VIPS_FREEF( DestroyImageList, magick->images ); VIPS_FREEF( DestroyImageInfo, magick->image_info ); - VIPS_FREEF( DestroyExceptionInfo, magick->exception ); + VIPS_FREEF( magick_destroy_exception, magick->exception ); G_OBJECT_CLASS( vips_foreign_save_magick_parent_class )-> dispose( gobject ); @@ -218,7 +218,7 @@ vips_foreign_save_magick_build( VipsObject *object ) */ im = save->ready; - magick->exception = AcquireExceptionInfo(); + magick->exception = magick_acquire_exception(); magick->image_info = CloneImageInfo( NULL ); magick->storage_type = UndefinedPixel; From 63c6c7ae3075e6a1b810de4a649c730b62a55587 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 4 Jan 2019 10:34:30 +0000 Subject: [PATCH 2/3] fix small magicksave error copy-paste fail meant that IM exceptions were not inherited correctly see https://github.com/libvips/libvips/commit/1619c8b1a1c0a1e2c2d853e6d01671f712466dbb#commitcomment-31838043 --- ChangeLog | 3 +++ configure.ac | 6 +++--- libvips/foreign/magick.c | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 324a00e0..e80313a8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,6 @@ +4/1/19 started 8.7.4 +- magicksave with magick6 API did not chain exceptions correctly [kleisauke] + 21/11/18 started 8.7.3 - fix infinite loop for autofit with non-scaleable font - mapim was not offsetting by window offset [erdmann] diff --git a/configure.ac b/configure.ac index f5e15cf2..f3c6abc6 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.7.3], [vipsip@jiscmail.ac.uk]) +AC_INIT([vips], [8.7.4], [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], [7]) -m4_define([vips_micro_version], [3]) +m4_define([vips_micro_version], [4]) 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=51 -LIBRARY_REVISION=4 +LIBRARY_REVISION=5 LIBRARY_AGE=9 # patched into include/vips/version.h diff --git a/libvips/foreign/magick.c b/libvips/foreign/magick.c index 65bf9546..49a8aa7b 100644 --- a/libvips/foreign/magick.c +++ b/libvips/foreign/magick.c @@ -248,9 +248,9 @@ magick_destroy_exception( ExceptionInfo *exception ) void magick_inherit_exception( ExceptionInfo *exception, Image *image ) { -#ifdef HAVE_INHERITEXCEPTIONINFO +#ifdef HAVE_INHERITEXCEPTION InheritException( exception, &image->exception ); -#endif /*HAVE_INHERITEXCEPTIONINFO*/ +#endif /*HAVE_INHERITEXCEPTION*/ } void From 5bc342b9b2a9ea053508faa725d55b07a23ef736 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Tue, 8 Jan 2019 09:05:39 +0000 Subject: [PATCH 3/3] revise changelog in magickload fix --- ChangeLog | 2 +- libvips/foreign/magick2vips.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 8d7cd864..a1d8e417 100644 --- a/ChangeLog +++ b/ChangeLog @@ -16,7 +16,7 @@ - add CMYK as a supported colourspace 4/1/19 started 8.7.4 -- magicksave with magick6 API did not chain exceptions correctly [kleisauke] +- fix memory leak in magickload [kleisauke] 21/11/18 started 8.7.3 - fix infinite loop for autofit with non-scaleable font diff --git a/libvips/foreign/magick2vips.c b/libvips/foreign/magick2vips.c index f3f311d7..c6777546 100644 --- a/libvips/foreign/magick2vips.c +++ b/libvips/foreign/magick2vips.c @@ -59,6 +59,9 @@ * - don't use Ping, it's too unreliable * 24/7/18 * - sniff extra filetypes + * 4/1/19 kleisauke + * - we did not chain exceptions correctly, causing a memory leak + * - added wrapper funcs for exception handling */ /*