From 4611651d90d5afe3f8628be2735a1d35be32f3b2 Mon Sep 17 00:00:00 2001 From: Kleis Auke Wolthuizen Date: Sun, 27 Nov 2022 16:43:35 +0100 Subject: [PATCH] nsgifload: avoid minimise after mapping (#3189) * nsgifload: avoid minimise after mapping Not reliable on Windows. * nsgifload: prefer use of `VIPS_FREEF` macro * Improve `test_descriptors.c` * Only build `test_descriptors` when targeting Linux --- libvips/foreign/heifload.c | 2 +- libvips/foreign/nsgifload.c | 22 +++--- libvips/foreign/webp2vips.c | 8 ++- libvips/iofuncs/mapfile.c | 1 + libvips/iofuncs/source.c | 5 -- libvips/meson.build | 4 +- meson.build | 2 +- test/meson.build | 31 +++++---- test/test_descriptors.c | 130 ++++++++++++++++++++++++++---------- test/test_descriptors.sh | 3 +- 10 files changed, 131 insertions(+), 77 deletions(-) diff --git a/libvips/foreign/heifload.c b/libvips/foreign/heifload.c index 83aed76d..0cf71ecf 100644 --- a/libvips/foreign/heifload.c +++ b/libvips/foreign/heifload.c @@ -1027,7 +1027,7 @@ vips_foreign_load_heif_load( VipsForeignLoad *load ) if( vips_foreign_load_heif_set_header( heif, t[0] ) ) return( -1 ); - /* CLose input immediately at end of read. + /* Close input immediately at end of read. */ g_signal_connect( t[0], "minimise", G_CALLBACK( vips_foreign_load_heif_minimise ), heif ); diff --git a/libvips/foreign/nsgifload.c b/libvips/foreign/nsgifload.c index d5d9075b..852ca092 100644 --- a/libvips/foreign/nsgifload.c +++ b/libvips/foreign/nsgifload.c @@ -3,9 +3,11 @@ * 6/10/18 * - from gifload.c * 3/3/22 tlsa - * - update libnsgif API - *9/5/22 - - attach GIF palette as metadata + * - update libnsgif API + * 9/5/22 + * - attach GIF palette as metadata + * 26/11/22 kleisauke + * - avoid minimise after mapping -- not reliable on Win32 */ /* @@ -160,9 +162,7 @@ vips_foreign_load_nsgif_dispose( GObject *gobject ) VIPS_DEBUG_MSG( "vips_foreign_load_nsgif_dispose:\n" ); - if( gif->anim ) { - nsgif_destroy( gif->anim ); - } + VIPS_FREEF( nsgif_destroy, gif->anim ); VIPS_UNREF( gif->source ); VIPS_FREE( gif->delay ); @@ -336,8 +336,6 @@ vips_foreign_load_nsgif_set_header( VipsForeignLoadNsgif *gif, * * Don't flag any errors unless we have to: we want to work for corrupt or * malformed GIFs. - * - * Close as soon as we can to free up the fd. */ static int vips_foreign_load_nsgif_header( VipsForeignLoad *load ) @@ -352,12 +350,10 @@ vips_foreign_load_nsgif_header( VipsForeignLoad *load ) VIPS_DEBUG_MSG( "vips_foreign_load_nsgif_header:\n" ); - /* We map in the image, then minimise to close any underlying file - * object. This won't unmap. + /* Map the whole source into memory. */ - if( !(data = vips_source_map( gif->source, &size )) ) + if( !(data = vips_source_map( gif->source, &size )) ) return( -1 ); - vips_source_minimise( gif->source ); /* Treat errors from _scan() as warnings. If libnsgif really can't do * something it'll fail gracefully later when we try to read out @@ -542,7 +538,7 @@ vips_foreign_load_nsgif_load( VipsForeignLoad *load ) { VipsForeignLoadNsgif *gif = (VipsForeignLoadNsgif *) load; VipsImage **t = (VipsImage **) - vips_object_local_array( VIPS_OBJECT( load ), 4 ); + vips_object_local_array( VIPS_OBJECT( load ), 2 ); VIPS_DEBUG_MSG( "vips_foreign_load_nsgif_load:\n" ); diff --git a/libvips/foreign/webp2vips.c b/libvips/foreign/webp2vips.c index c4ab1601..73d69aa4 100644 --- a/libvips/foreign/webp2vips.c +++ b/libvips/foreign/webp2vips.c @@ -370,7 +370,7 @@ read_new( VipsImage *out, VipsSource *source, int page, int n, double scale ) read->frame_no = 0; /* Everything has to stay open until read has finished, unfortunately, - * since webp relies on us mapping the whole file. + * since webp relies on us mapping the whole source. */ g_signal_connect( out, "close", G_CALLBACK( read_close_cb ), read ); @@ -379,6 +379,8 @@ read_new( VipsImage *out, VipsSource *source, int page, int n, double scale ) read->config.options.use_threads = 1; read->config.output.is_external_memory = 1; + /* Map the whole source into memory. + */ if( !(read->data.bytes = vips_source_map( source, &read->data.size )) ) return( NULL ); @@ -783,8 +785,10 @@ static int read_image( Read *read, VipsImage *out ) { VipsImage **t = (VipsImage **) - vips_object_local_array( VIPS_OBJECT( out ), 3 ); + vips_object_local_array( VIPS_OBJECT( out ), 2 ); + /* Make the output pipeline. + */ t[0] = vips_image_new(); if( read_header( read, t[0] ) ) return( -1 ); diff --git a/libvips/iofuncs/mapfile.c b/libvips/iofuncs/mapfile.c index eedc138a..81c01832 100644 --- a/libvips/iofuncs/mapfile.c +++ b/libvips/iofuncs/mapfile.c @@ -90,6 +90,7 @@ #endif /*G_OS_WIN32*/ /* Does this fd support mmap. Pipes won't, for example. + * FIXME unused internal function */ gboolean vips__mmap_supported( int fd ) diff --git a/libvips/iofuncs/source.c b/libvips/iofuncs/source.c index d9573f4d..8a2b4dd5 100644 --- a/libvips/iofuncs/source.c +++ b/libvips/iofuncs/source.c @@ -43,11 +43,6 @@ */ -/* TODO - * - * - can we map and then close the fd? how about on Windows? - */ - /* #define TEST_SANITY #define VIPS_DEBUG diff --git a/libvips/meson.build b/libvips/meson.build index 695a5841..219c1365 100644 --- a/libvips/meson.build +++ b/libvips/meson.build @@ -75,8 +75,8 @@ endif # Keep the autotools convention for shared module suffix because GModule # depends on it: https://gitlab.gnome.org/GNOME/glib/issues/1413 module_suffix = [] -if ['darwin', 'ios'].contains(host_os) - module_suffix = 'so' +if host_os in ['darwin', 'ios'] + module_suffix = 'so' endif if magick_module diff --git a/meson.build b/meson.build index af9a0edb..a0709149 100644 --- a/meson.build +++ b/meson.build @@ -90,7 +90,7 @@ if modules_enabled endif # Detect and set symbol visibility -if get_option('default_library') == 'shared' and (host_os == 'windows' or host_os == 'cygwin') +if get_option('default_library') == 'shared' and host_os in ['windows', 'cygwin'] cfg_var.set('DLL_EXPORT', true) if cc.get_id() == 'msvc' or cc.get_id() == 'clang-cl' cfg_var.set('_VIPS_PUBLIC', '__declspec(dllexport)') diff --git a/test/meson.build b/test/meson.build index d507b709..89a5e9cc 100644 --- a/test/meson.build +++ b/test/meson.build @@ -44,19 +44,22 @@ test('connections', workdir: meson.current_build_dir(), ) -test_descriptors = executable('test_descriptors', - 'test_descriptors.c', - dependencies: libvips_dep, -) +# Uses /proc/self/fd, which is only available on *nix +if host_os == 'linux' + test_descriptors = executable('test_descriptors', + 'test_descriptors.c', + dependencies: libvips_dep, + ) -test_descriptors_sh = configure_file( - input: 'test_descriptors.sh', - output: 'test_descriptors.sh', - copy: true, -) + test_descriptors_sh = configure_file( + input: 'test_descriptors.sh', + output: 'test_descriptors.sh', + copy: true, + ) -test('descriptors', - test_descriptors_sh, - depends: test_descriptors, - workdir: meson.current_build_dir(), -) + test('descriptors', + test_descriptors_sh, + depends: test_descriptors, + workdir: meson.current_build_dir(), + ) +endif diff --git a/test/test_descriptors.c b/test/test_descriptors.c index cc989b62..46053b50 100644 --- a/test/test_descriptors.c +++ b/test/test_descriptors.c @@ -1,31 +1,86 @@ /* Read an image and check that file handles are being closed on minimise. * - * This will only work on linux: we signal success and do nothing if /dev/proc - * does not exist. + * This will only work on linux: we signal success and do nothing if + * /proc/self/fd does not exist. */ -#include +#include #include +#define _GNU_SOURCE +#include + #include -/* Count the number of files in a directory, -1 for directory not found etc. +/** + * get_open_files: + * + * Get a list of open files for this process. + * + * Returns (transfer full) (nullable): a new #GSList, or %NULL */ -static int -count_files( const char *dirname ) +static GSList * +get_open_files() { + GSList *list = NULL; GDir *dir; - int n; + const char *name; - if( !(dir = g_dir_open( dirname, 0, NULL )) ) - return( -1 ); + if( !(dir = g_dir_open( "/proc/self/fd", 0, NULL )) ) + return( NULL ); - for( n = 0; g_dir_read_name( dir ); n++ ) - ; + while( (name = g_dir_read_name( dir )) ) { + char *fullname = g_build_filename( "/proc/self/fd", name, NULL ); + + list = g_slist_prepend( list, realpath( fullname, NULL ) ); + + g_free( fullname ); + } g_dir_close( dir ); - return( n ); + return( list ); +} + +/** + * fd_check: + * @stage: the originating stage for the error message + * @fds: a #GSList of file descriptors to check against + * + * Check for a leak by comparing the currently open files for this + * process with the file descriptors in @fds. If there's a leak, + * print an error message and return %FALSE. + * + * See also: get_open_files(). + * + * Returns: %TRUE if there are no leaks; %FALSE otherwise + */ +static gboolean +fd_check( const char *stage, GSList *fds ) +{ + GSList *unique_list = NULL, *list, *iter; + + list = get_open_files(); + + for( iter = list; iter; iter = iter->next ) + if( !g_slist_find_custom( fds, iter->data, + (GCompareFunc) g_strcmp0 ) ) + unique_list = g_slist_prepend( unique_list, iter->data ); + + if( unique_list == NULL ) { + g_slist_free_full( list, g_free ); + return( TRUE ); + } + + fprintf( stderr, "%s: file descriptors not closed after %s:\n", + vips_get_prgname(), stage ); + for( iter = unique_list; iter; iter = iter->next ) + fprintf( stderr, "%s\n", (char *) iter->data ); + + g_slist_free( unique_list ); + g_slist_free_full( list, g_free ); + + return( FALSE ); } int @@ -33,39 +88,36 @@ main( int argc, char **argv ) { VipsSource *source; VipsImage *image, *x; - char fd_dir[256]; - int n_files; + GSList *list; double average; - if( VIPS_INIT( argv[0] ) ) - vips_error_exit( "unable to start" ); + if( VIPS_INIT( argv[0] ) ) + vips_error_exit( "unable to start" ); if( argc != 2 ) vips_error_exit( "usage: %s test-image", argv[0] ); - vips_snprintf( fd_dir, 256, "/proc/%d/fd", getpid() ); - n_files = count_files( fd_dir ); - if( n_files == -1 ) + list = get_open_files(); + if( list == NULL ) /* Probably not linux, silent success. */ return( 0 ); - /* This is usually 4. stdout / stdin / stderr plus one more made for - * us by glib, I think, doing what I don't know. + /* This is usually a list of 4 files. stdout / stdin / stderr plus one + * more made for us by glib, I think, doing what I don't know. */ /* Opening an image should read the header, then close the fd. */ printf( "** rand open ..\n" ); if( !(source = vips_source_new_from_file( argv[1] )) ) - vips_error_exit( NULL ); + goto error; if( !(image = vips_image_new_from_source( source, "", "access", VIPS_ACCESS_RANDOM, NULL )) ) - vips_error_exit( NULL ); - if( count_files( fd_dir ) != n_files ) - vips_error_exit( "%s: fd not closed after header read", - argv[1] ); + goto error; + if( !fd_check( "header read", list ) ) + goto error; /* We should be able to read a chunk near the top, then have the fd * closed again. @@ -73,11 +125,11 @@ main( int argc, char **argv ) printf( "** crop1, avg ..\n" ); if( vips_crop( image, &x, 0, 0, image->Xsize, 10, NULL ) || vips_avg( x, &average, NULL ) ) - vips_error_exit( NULL ); + goto error; + g_object_unref( x ); - if( count_files( fd_dir ) != n_files ) - vips_error_exit( "%s: fd not closed after first read", - argv[1] ); + if( !fd_check( "first read", list ) ) + goto error; /* We should be able to read again, a little further down, and have * the input restarted and closed again. @@ -85,11 +137,11 @@ main( int argc, char **argv ) printf( "** crop2, avg ..\n" ); if( vips_crop( image, &x, 0, 20, image->Xsize, 10, NULL ) || vips_avg( x, &average, NULL ) ) - vips_error_exit( NULL ); + goto error; + g_object_unref( x ); - if( count_files( fd_dir ) != n_files ) - vips_error_exit( "%s: fd not closed after second read", - argv[1] ); + if( !fd_check( "second read", list ) ) + goto error; /* Clean up, and we should still just have three open. */ @@ -99,9 +151,13 @@ main( int argc, char **argv ) printf( "** shutdown ..\n" ); vips_shutdown(); - if( count_files( fd_dir ) != n_files ) - vips_error_exit( "%s: fd not closed after shutdown", - argv[1] ); + if( !fd_check( "shutdown", list ) ) + goto error; + g_slist_free_full( list, g_free ); return( 0 ); + +error: + g_slist_free_full( list, g_free ); + return( 1 ); } diff --git a/test/test_descriptors.sh b/test/test_descriptors.sh index 8e0df7cc..f53fd06b 100755 --- a/test/test_descriptors.sh +++ b/test/test_descriptors.sh @@ -2,7 +2,7 @@ # test the various restartable loaders -# webp and ppm use streams, but they mmap the input, so you can't close() the +# gif, webp and ppm use streams, but they mmap the input, so you can't close() the # fd on minimise # set -x @@ -29,4 +29,3 @@ fi if test_supported svgload_source; then ./test_descriptors $test_images/logo.svg fi -