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

@@ -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 ); 

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" );
 

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 );

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 )

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

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

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)')

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

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 <sys/types.h>
+#include <stdio.h>
 #include <unistd.h>
 
+#define _GNU_SOURCE
+#include <stdlib.h>
+
 #include <vips/vips.h>
 
-/* 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 );
 }

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
-