fix VipsSource and named pipes

jcupitt · jcupitt · commit f6281284a18c · 2021-10-08T12:20:24.000+01:00
We used to assume (in several places) that any source with a filename was seekable. This patch adds a is_file test, and makes all the loaders use it. see libvips#2467
diff --git a/ChangeLog b/ChangeLog
@@ -10,6 +10,7 @@
 - flatten handles out of range alpha and max_alpha correctly
 - don't use atexit for cleanup, it's too unreliable
 - tiff writer loops for the whole image rather than per page [LionelArn2]
+- fix VipsSource with named pipes [vibbix]
 
 16/8/21 started 8.11.4
 - fix off-by-one error in new rank fast path
diff --git a/libvips/foreign/fitsload.c b/libvips/foreign/fitsload.c
@@ -88,17 +88,22 @@ vips_foreign_load_fits_build( VipsObject *object )
 	VipsForeignLoadFits *fits = 
 		(VipsForeignLoadFits *) object;
 
-	/* We can only open source which have an associated filename, since
+	/* We can only open sources which have an associated filename, since
 	 * the fits library works in terms of filenames.
 	 */
 	if( fits->source ) {
-		fits->filename = vips_connection_filename( VIPS_CONNECTION( 
-			fits->source ) );
-		if( !fits->filename ) {
+		VipsConnection *connection = VIPS_CONNECTION( fits->source );
+
+		const char *filename;
+
+		if( !vips_source_is_file( fits->source ) ||
+			!(filename = vips_connection_filename( connection )) ) {
 			vips_error( class->nickname, "%s", 
 				_( "no filename available" ) );
 			return( -1 );
 		}
+
+		fits->filename = filename;
 	}
 
 	if( VIPS_OBJECT_CLASS( vips_foreign_load_fits_parent_class )->
@@ -296,10 +301,12 @@ vips_foreign_load_fits_source_build( VipsObject *object )
 static gboolean
 vips_foreign_load_fits_source_is_a_source( VipsSource *source )
 {
+	VipsConnection *connection = VIPS_CONNECTION( source );
+
 	const char *filename;
 
-	return( (filename = 
-		vips_connection_filename( VIPS_CONNECTION( source ) )) &&
+	return( vips_source_is_file( source ) &&
+		(filename = vips_connection_filename( connection )) &&
 		vips__fits_isfits( filename ) );
 }
 
diff --git a/libvips/foreign/niftiload.c b/libvips/foreign/niftiload.c
@@ -121,13 +121,18 @@ vips_foreign_load_nifti_build( VipsObject *object )
 	 * the nifti library works in terms of filenames.
 	 */
 	if( nifti->source ) {
-		nifti->filename = vips_connection_filename( VIPS_CONNECTION( 
-			nifti->source ) );
-		if( !nifti->filename ) {
-			vips_error( class->nickname, "%s", 
-				_( "no filename available" ) );
+		VipsConnection *connection = VIPS_CONNECTION( nifti->source );
+
+		const char *filename;
+
+		if( !vips_source_is_file( nifti->source ) ||
+			!(filename = vips_connection_filename( connection )) ) {
+			vips_error( class->nickname, 
+				"%s", _( "no filename available" ) );
 			return( -1 );
 		}
+
+		nifti->filename = filename; 
 	}
 
 	if( VIPS_OBJECT_CLASS( vips_foreign_load_nifti_parent_class )->
@@ -747,10 +752,12 @@ vips_foreign_load_nifti_source_build( VipsObject *object )
 static gboolean
 vips_foreign_load_nifti_source_is_a_source( VipsSource *source )
 {
+	VipsConnection *connection = VIPS_CONNECTION( source );
+
 	const char *filename;
 
-	return( (filename = 
-		vips_connection_filename( VIPS_CONNECTION( source ) )) &&
+	return( vips_source_is_file( source ) &&
+		(filename = vips_connection_filename( connection )) &&
 		vips_foreign_load_nifti_is_a( filename ) );
 }
 
diff --git a/libvips/foreign/openslideload.c b/libvips/foreign/openslideload.c
@@ -761,14 +761,19 @@ vips_foreign_load_openslide_build( VipsObject *object )
 	 * the openslide library works in terms of filenames.
 	 */
 	if( openslide->source ) {
-		openslide->filename = 
-			vips_connection_filename( VIPS_CONNECTION( 
-				openslide->source ) );
-		if( !openslide->filename ) {
+		VipsConnection *connection = 
+			VIPS_CONNECTION( openslide->source );
+
+		const char *filename;
+
+		if( !vips_source_is_file( openslide->source ) ||
+			!(filename = vips_connection_filename( connection )) ) {
 			vips_error( class->nickname, "%s", 
 				_( "no filename available" ) );
 			return( -1 );
 		}
+
+		openslide->filename = filename;
 	}
 
 	if( VIPS_OBJECT_CLASS( vips_foreign_load_openslide_parent_class )->
@@ -1033,10 +1038,12 @@ vips_foreign_load_openslide_source_build( VipsObject *object )
 static gboolean
 vips_foreign_load_openslide_source_is_a_source( VipsSource *source )
 {
+	VipsConnection *connection = VIPS_CONNECTION( source );
+
 	const char *filename;
 
-	return( (filename = 
-		vips_connection_filename( VIPS_CONNECTION( source ) )) &&
+	return( vips_source_is_file( source ) &&
+		(filename = vips_connection_filename( connection )) &&
 		vips__openslide_isslide( filename ) );
 }
 
diff --git a/libvips/foreign/radiance.c b/libvips/foreign/radiance.c
@@ -873,7 +873,7 @@ rad2vips_get_header( Read *read, VipsImage *out )
 		interpretation,
 		1, read->aspect );
 
-	VIPS_SETSTR( out->filename, 
+	VIPS_SETSTR( out->filename,
 		vips_connection_filename( 
 			VIPS_CONNECTION( read->sbuf->source ) ) );
 
diff --git a/libvips/foreign/vipsload.c b/libvips/foreign/vipsload.c
@@ -110,31 +110,24 @@ vips_foreign_load_vips_get_flags_filename( const char *filename )
 static int
 vips_foreign_load_vips_header( VipsForeignLoad *load )
 {
+	VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( load );
 	VipsForeignLoadVips *vips = (VipsForeignLoadVips *) load;
+	VipsConnection *connection = VIPS_CONNECTION( vips->source );
 
 	const char *filename;
 	VipsImage *image;
 	VipsImage *x;
 
-	if( (filename = 
-		vips_connection_filename( VIPS_CONNECTION( vips->source ) )) ) {
-		if( !(image = vips_image_new_mode( filename, "r" )) )
-			return( -1 );
-	}
-	else {
-		VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( load );
-
-		/* We could add load vips from memory, fd, via mmap etc. here.
-		 * We should perhaps move iofuncs/vips.c into this file.
-		 *
-		 * For now, just fail unless there's a filename associated
-		 * with this source.
-		 */
+	if( !vips_source_is_file( vips->source ) ||
+		!(filename = vips_connection_filename( connection )) ) {
 		vips_error( class->nickname, 
 			"%s", _( "no filename associated with source" ) );
 		return( -1 );
 	}
 
+	if( !(image = vips_image_new_mode( filename, "r" )) )
+		return( -1 );
+
 	/* What a hack. Remove the @out that's there now and replace it with
 	 * our image. 
 	 */
@@ -280,10 +273,12 @@ vips_foreign_load_vips_source_build( VipsObject *object )
 static gboolean
 vips_foreign_load_vips_source_is_a_source( VipsSource *source )
 {
+	VipsConnection *connection = VIPS_CONNECTION( source );
+
 	const char *filename;
 
-	return( (filename = 
-		vips_connection_filename( VIPS_CONNECTION( source ) )) &&
+	return( vips_source_is_file( source ) &&
+		(filename = vips_connection_filename( connection )) &&
 		vips__file_magic( filename ) );
 }
 
diff --git a/libvips/include/vips/connection.h b/libvips/include/vips/connection.h
@@ -218,6 +218,7 @@ int vips_source_unminimise( VipsSource *source );
 int vips_source_decode( VipsSource *source );
 gint64 vips_source_read( VipsSource *source, void *data, size_t length );
 gboolean vips_source_is_mappable( VipsSource *source );
+gboolean vips_source_is_file( VipsSource *source );
 const void *vips_source_map( VipsSource *source, size_t *length );
 VipsBlob *vips_source_map_blob( VipsSource *source );
 gint64 vips_source_seek( VipsSource *source, gint64 offset, int whence );
diff --git a/libvips/iofuncs/source.c b/libvips/iofuncs/source.c
@@ -10,6 +10,8 @@
  * 26/11/20
  * 	- use _setmode() on win to force binary read for previously opened
  * 	  descriptors
+ * 8/10/21
+ * 	- fix named pipes
  */
 
 /*
@@ -129,59 +131,66 @@ vips_pipe_read_limit_set( gint64 limit )
 
 G_DEFINE_TYPE( VipsSource, vips_source, VIPS_TYPE_CONNECTION );
 
-/* We can't test for seekability or length during _build, since the read and 
- * seek signal handlers might not have been connected yet. Instead, we test 
- * when we first need to know.
+/* Does this source support seek. You must unminimise before calling this.
  */
 static int
-vips_source_test_features( VipsSource *source )
+vips_source_test_seek( VipsSource *source )
 {
-	VipsSourceClass *class = VIPS_SOURCE_GET_CLASS( source );
-
-	if( source->have_tested_seek ) 
-		return( 0 );
-	source->have_tested_seek = TRUE;
+	if( !source->have_tested_seek ) {
+		VipsSourceClass *class = VIPS_SOURCE_GET_CLASS( source );
 
-	VIPS_DEBUG_MSG( "vips_source_test_features: testing seek ..\n" );
+		source->have_tested_seek = TRUE;
 
-	/* We'll need a descriptor to test.
-	 */
-	if( vips_source_unminimise( source ) ) 
-		return( -1 );
+		VIPS_DEBUG_MSG( "vips_source_can_seek: testing seek ..\n" );
 
-	/* Can we seek this input?
-	 *
-	 * We need to call the method directly rather than via
-	 * vips_source_seek() etc. or we might trigger seek emulation.
-	 */
-	if( source->data ||
-		class->seek( source, 0, SEEK_CUR ) != -1 ) { 
-		gint64 length;
+		/* Can we seek this input?
+		 *
+		 * We need to call the method directly rather than via
+		 * vips_source_seek() etc. or we might trigger seek emulation.
+		 */
+		if( source->data ||
+			class->seek( source, 0, SEEK_CUR ) != -1 ) { 
+			gint64 length;
 
-		VIPS_DEBUG_MSG( "    seekable source\n" );
+			VIPS_DEBUG_MSG( "    seekable source\n" );
 
-		/* We should be able to get the length of seekable 
-		 * objects.
-		 */
-		if( (length = vips_source_length( source )) == -1 ) 
-			return( -1 );
+			/* We should be able to get the length of seekable 
+			 * objects.
+			 */
+			if( (length = vips_source_length( source )) == -1 ) 
+				return( -1 );
 
-		source->length = length;
+			source->length = length;
 
-		/* If we can seek, we won't need to save header bytes.
-		 */
-		VIPS_FREEF( g_byte_array_unref, source->header_bytes ); 
-	}
-	else {
-		/* Not seekable. This must be some kind of pipe.
-		 */
-		VIPS_DEBUG_MSG( "    not seekable\n" );
-		source->is_pipe = TRUE;
+			/* If we can seek, we won't need to save header bytes.
+			 */
+			VIPS_FREEF( g_byte_array_unref, source->header_bytes ); 
+		}
+		else {
+			/* Not seekable. This must be some kind of pipe.
+			 */
+			VIPS_DEBUG_MSG( "    not seekable\n" );
+			source->is_pipe = TRUE;
+		}
 	}
 
 	return( 0 );
 }
 
+/* We can't test for seekability or length during _build, since the read and 
+ * seek signal handlers might not have been connected yet. Instead, we test 
+ * when we first need to know.
+ */
+static int
+vips_source_test_features( VipsSource *source )
+{
+	if( vips_source_unminimise( source ) || 
+		vips_source_test_seek( source ) )
+		return( -1 );
+
+	return( 0 );
+}
+
 #ifdef TEST_SANITY
 static void
 vips_source_sanity( VipsSource *source )
@@ -634,12 +643,19 @@ vips_source_unminimise( VipsSource *source )
 		connection->tracked_descriptor = fd;
 		connection->descriptor = fd;
 
-		VIPS_DEBUG_MSG( "vips_source_unminimise: "
-			"restoring read position %" G_GINT64_FORMAT "\n", 
-			source->read_position );
-		if( vips__seek( connection->descriptor, 
-			source->read_position, SEEK_SET ) == -1 )
-			return( -1 );
+		if( vips_source_test_seek( source ) ) 
+			return( -1 ); 
+
+		/* It might be a named pipe.
+		 */
+		if( !source->is_pipe ) {
+			VIPS_DEBUG_MSG( "vips_source_unminimise: restoring "
+				"read position %" G_GINT64_FORMAT "\n", 
+				source->read_position );
+			if( vips__seek( connection->descriptor, 
+				source->read_position, SEEK_SET ) == -1 )
+				return( -1 );
+		}
 	}
 
 	return( 0 );
@@ -976,6 +992,32 @@ vips_source_is_mappable( VipsSource *source )
 		 VIPS_CONNECTION( source )->descriptor != -1) );
 }
 
+/**
+ * vips_source_is_file:
+ * @source: source to operate on
+ *
+ * Test if this source is a simple file with support for seek. Named pipes,
+ * for example, will fail this test. If TRUE, you can use
+ * vips_connection_filename() to find the filename.
+ *
+ * Use this to add basic source support for older loaders which can only work
+ * on files.
+ *
+ * Returns: %TRUE if the source is a simple file.
+ */
+gboolean 
+vips_source_is_file( VipsSource *source )
+{
+	if( vips_source_unminimise( source ) ||
+		vips_source_test_features( source ) )
+		return( -1 );
+
+	/* There's a filename, and it supports seek.
+	 */
+	return( VIPS_CONNECTION( source )->filename &&
+		!source->is_pipe );
+}
+
 /**
  * vips_source_map:
  * @source: source to operate on
