From f6281284a18c6318cab96f20531e4e14643ec5b4 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 8 Oct 2021 12:20:24 +0100 Subject: [PATCH] fix VipsSource and named pipes 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 https://github.com/libvips/libvips/issues/2467 --- ChangeLog | 1 + libvips/foreign/fitsload.c | 19 +++-- libvips/foreign/niftiload.c | 21 +++-- libvips/foreign/openslideload.c | 19 +++-- libvips/foreign/radiance.c | 2 +- libvips/foreign/vipsload.c | 27 +++--- libvips/include/vips/connection.h | 1 + libvips/iofuncs/source.c | 136 +++++++++++++++++++----------- 8 files changed, 143 insertions(+), 83 deletions(-) diff --git a/ChangeLog b/ChangeLog index e51b0aea..b865ed75 100644 --- 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 index 83fe0765..c85b767a 100644 --- 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 index a68725ce..61a9c07e 100644 --- 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 index 373e368d..0965bf7d 100644 --- 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 index 0bff036d..406e194e 100644 --- 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 index 73d5a290..20064cfc 100644 --- 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 index dc493ca0..5f2a22f3 100644 --- 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 index 41746d05..59d774c3 100644 --- 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,6 +131,52 @@ vips_pipe_read_limit_set( gint64 limit ) G_DEFINE_TYPE( VipsSource, vips_source, VIPS_TYPE_CONNECTION ); +/* Does this source support seek. You must unminimise before calling this. + */ +static int +vips_source_test_seek( VipsSource *source ) +{ + if( !source->have_tested_seek ) { + VipsSourceClass *class = VIPS_SOURCE_GET_CLASS( source ); + + source->have_tested_seek = TRUE; + + 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; + + 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 ); + + 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; + } + } + + 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. @@ -136,49 +184,10 @@ G_DEFINE_TYPE( VipsSource, vips_source, VIPS_TYPE_CONNECTION ); static int vips_source_test_features( VipsSource *source ) { - VipsSourceClass *class = VIPS_SOURCE_GET_CLASS( source ); - - if( source->have_tested_seek ) - return( 0 ); - source->have_tested_seek = TRUE; - - VIPS_DEBUG_MSG( "vips_source_test_features: testing seek ..\n" ); - - /* We'll need a descriptor to test. - */ - if( vips_source_unminimise( source ) ) + if( vips_source_unminimise( source ) || + vips_source_test_seek( source ) ) return( -1 ); - /* 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" ); - - /* We should be able to get the length of seekable - * objects. - */ - if( (length = vips_source_length( source )) == -1 ) - return( -1 ); - - 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; - } - return( 0 ); } @@ -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