Skip to content

Commit

Permalink
fix memleak in magick6load
Browse files Browse the repository at this point in the history
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:

libvips/lua-vips#24

#1203
  • Loading branch information
jcupitt committed Jan 4, 2019
1 parent 9d66420 commit 1619c8b
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 23 deletions.
1 change: 1 addition & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
52 changes: 50 additions & 2 deletions libvips/foreign/magick.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
{
Expand Down Expand Up @@ -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

This comment has been minimized.

Copy link
@kleisauke

kleisauke Jan 4, 2019

Member

HAVE_INHERITEXCEPTIONINFO -> HAVE_INHERITEXCEPTION?

This comment has been minimized.

Copy link
@jcupitt

jcupitt Jan 4, 2019

Author Member

Ooops! Stupid fat fingers.

It seems very minor, so let's leave that for 8.7.4.

InheritException( exception, &image->exception );
#endif /*HAVE_INHERITEXCEPTION*/
#endif /*HAVE_INHERITEXCEPTIONINFO*/
}

void
Expand Down
3 changes: 3 additions & 0 deletions libvips/foreign/magick.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
24 changes: 11 additions & 13 deletions libvips/foreign/magick2vips.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ typedef struct _Read {

Image *image;
ImageInfo *image_info;
ExceptionInfo exception;
ExceptionInfo *exception;

/* Number of pages in image.
*/
Expand Down Expand Up @@ -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 );
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 );
}
Expand Down Expand Up @@ -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 );
}
Expand Down
12 changes: 6 additions & 6 deletions libvips/foreign/magick7load.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 )->
Expand All @@ -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 )
Expand Down Expand Up @@ -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 );
}
Expand Down Expand Up @@ -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 );
}
Expand Down
4 changes: 2 additions & 2 deletions libvips/foreign/magicksave.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 1619c8b

Please sign in to comment.