From ac2fb4a823d30b2e1c540c837e4377e6b8ccb58f Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sun, 6 Nov 2011 09:58:02 +0000 Subject: [PATCH] fixed a fd leak image rewind was not closing the fd --- TODO | 11 +++---- libvips/iofuncs/image.c | 67 ++++++++++++++++++++++------------------ libvips/iofuncs/memory.c | 9 +++++- 3 files changed, 49 insertions(+), 38 deletions(-) diff --git a/TODO b/TODO index df0edc34..e2e21343 100644 --- a/TODO +++ b/TODO @@ -1,12 +1,9 @@ -- try +- try: - $ vips copy babe.png x.v - tracked memory: 0 allocations totalling 0 bytes - tracked memory: high-water mark 36.09 KB - tracked files: 1 open - $ + $ vips vips copy babe.png x.v + GLib-GObject-WARNING **: invalid cast from `VipsFormatVips' to + `VipsOperation' - leaking a tracked somewhere diff --git a/libvips/iofuncs/image.c b/libvips/iofuncs/image.c index 446c705f..039a8795 100644 --- a/libvips/iofuncs/image.c +++ b/libvips/iofuncs/image.c @@ -195,41 +195,11 @@ vips_image_finalize( GObject *gobject ) */ vips__link_break_all( image ); - /* Any file mapping? - */ - if( image->baseaddr ) { - /* MMAP file. - */ - VIPS_DEBUG_MSG( "vips_image_finalize: unmapping file\n" ); - - vips__munmap( image->baseaddr, image->length ); - image->baseaddr = NULL; - image->length = 0; - - /* This must have been a pointer to the mmap region, rather - * than a setbuf. - */ - image->data = NULL; - } - if( image->time ) { VIPS_FREEF( g_timer_destroy, image->time->start ); VIPS_FREE( image->time ); } - /* Is there a file descriptor? - */ - if( image->fd != -1 ) { - VIPS_DEBUG_MSG( "vips_image_finalize: closing output file\n" ); - - if( image->dtype == VIPS_IMAGE_OPENOUT ) - (void) vips__writehist( image ); - if( vips_tracked_close( image->fd ) == -1 ) - vips_error( "VipsImage", - "%s", _( "unable to close fd" ) ); - image->fd = -1; - } - /* Any image data? */ if( image->data ) { @@ -267,10 +237,47 @@ vips_image_finalize( GObject *gobject ) static void vips_image_dispose( GObject *gobject ) { + VipsImage *image = VIPS_IMAGE( gobject ); + VIPS_DEBUG_MSG( "vips_image_dispose: %p\n", gobject ); vips_object_preclose( VIPS_OBJECT( gobject ) ); + /* We have to junk the fd in dispose, since we run this for rewind and + * we must close and reopen the file when we switch from write to + * read. + */ + + /* Any file mapping? + */ + if( image->baseaddr ) { + /* MMAP file. + */ + VIPS_DEBUG_MSG( "vips_image_finalize: unmapping file\n" ); + + vips__munmap( image->baseaddr, image->length ); + image->baseaddr = NULL; + image->length = 0; + + /* This must have been a pointer to the mmap region, rather + * than a setbuf. + */ + image->data = NULL; + } + + /* Is there a file descriptor? + */ + if( image->fd != -1 ) { + VIPS_DEBUG_MSG( "vips_image_finalize: closing output file\n" ); + + if( image->dtype == VIPS_IMAGE_OPENOUT ) + (void) vips__writehist( image ); + if( vips_tracked_close( image->fd ) == -1 ) + vips_error( "VipsImage", + "%s", _( "unable to close fd" ) ); + image->fd = -1; + } + G_OBJECT_CLASS( vips_image_parent_class )->dispose( gobject ); } diff --git a/libvips/iofuncs/memory.c b/libvips/iofuncs/memory.c index 0ccd90d3..2627a7a9 100644 --- a/libvips/iofuncs/memory.c +++ b/libvips/iofuncs/memory.c @@ -99,7 +99,7 @@ static int vips_tracked_allocs = 0; static size_t vips_tracked_mem = 0; -static size_t vips_tracked_files = 0; +static int vips_tracked_files = 0; static size_t vips_tracked_mem_highwater = 0; static GMutex *vips_tracked_mutex = NULL; @@ -352,6 +352,10 @@ vips_tracked_open( const char *pathname, int flags, ... ) g_mutex_lock( vips_tracked_mutex ); vips_tracked_files += 1; +#ifdef DEBUG + printf( "vips_tracked_open: %s = %d (%d)\n", + pathname, fd, vips_tracked_files ); +#endif /*DEBUG*/ g_mutex_unlock( vips_tracked_mutex ); @@ -381,6 +385,9 @@ vips_tracked_close( int fd ) g_assert( vips_tracked_files > 0 ); vips_tracked_files -= 1; +#ifdef DEBUG + printf( "vips_tracked_close: %d (%d)\n", fd, vips_tracked_files ); +#endif /*DEBUG*/ g_mutex_unlock( vips_tracked_mutex );