From 8fa3b0aa1cbbf1501a27c4cc869b8d55455a46ac Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 22 Nov 2018 09:15:57 +0000 Subject: [PATCH] start trying to fix up blob/string mixups we often set strings as blobs (eg. libtiff uses pointer/length for XML and does not null-terminate) ... when do we validate stringiness, and add the missing null? --- TODO | 9 +++ libvips/foreign/tiff2vips.c | 58 +++++------------- libvips/foreign/vipspng.c | 6 +- libvips/include/vips/header.h | 6 +- libvips/iofuncs/header.c | 109 +++++++++++++++++++++++++++++++--- 5 files changed, 131 insertions(+), 57 deletions(-) diff --git a/TODO b/TODO index 967dcc6b..f919385b 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,12 @@ +- use set_blob_copy everywhere we can + +- argh we now have two type sof BLOB, with and without invisible null + termination, with no way to tell them apart + + + + + - try $ vips gaussmat x.mat 0.1 0.1 diff --git a/libvips/foreign/tiff2vips.c b/libvips/foreign/tiff2vips.c index 5267df63..f7b47589 100644 --- a/libvips/foreign/tiff2vips.c +++ b/libvips/foreign/tiff2vips.c @@ -1352,74 +1352,44 @@ rtiff_set_header( Rtiff *rtiff, VipsImage *out ) /* Read any ICC profile. */ if( TIFFGetField( rtiff->tiff, - TIFFTAG_ICCPROFILE, &data_length, &data ) && - data && - data_length ) { - void *data_copy; - - if( !(data_copy = vips_malloc( NULL, data_length )) ) + TIFFTAG_ICCPROFILE, &data_length, &data ) ) { + if( vips_image_set_blob_copy( rtiff, out, + VIPS_META_ICC_NAME, data_length, data ) ) return( -1 ); - memcpy( data_copy, data, data_length ); - vips_image_set_blob( out, VIPS_META_ICC_NAME, - (VipsCallbackFn) vips_free, data_copy, data_length ); } /* Read any XMP metadata. */ if( TIFFGetField( rtiff->tiff, - TIFFTAG_XMLPACKET, &data_length, &data ) && - data && - data_length ) { - void *data_copy; - - if( !(data_copy = vips_malloc( NULL, data_length )) ) + TIFFTAG_XMLPACKET, &data_length, &data ) ) { + if( vips_image_set_blob_copy( rtiff, out, + VIPS_META_XMP_NAME, data_length, data ) ) return( -1 ); - memcpy( data_copy, data, data_length ); - vips_image_set_blob( out, VIPS_META_XMP_NAME, - (VipsCallbackFn) vips_free, data_copy, data_length ); } /* Read any IPTC metadata. */ if( TIFFGetField( rtiff->tiff, - TIFFTAG_RICHTIFFIPTC, &data_length, &data ) && - data && - data_length ) { - void *data_copy; - - /* libtiff stores IPTC as an array of long, not byte. - */ - data_length *= 4; - - if( !(data_copy = vips_malloc( NULL, data_length )) ) + TIFFTAG_RICHTIFFIPTC, &data_length, &data ) ) { + if( vips_image_set_blob_copy( rtiff, out, + VIPS_META_IPTC_NAME, data_length, data ) ) return( -1 ); - memcpy( data_copy, data, data_length ); - vips_image_set_blob( out, VIPS_META_IPTC_NAME, - (VipsCallbackFn) vips_free, data_copy, data_length ); /* Older versions of libvips used this misspelt name :-( attach * under this name too for compatibility. */ - if( !(data_copy = vips_malloc( NULL, data_length )) ) + if( vips_image_set_blob_copy( rtiff, out, + "ipct-data", data_length, data ) ) return( -1 ); - memcpy( data_copy, data, data_length ); - vips_image_set_blob( out, "ipct-data", - (VipsCallbackFn) vips_free, data_copy, data_length ); } /* Read any photoshop metadata. */ if( TIFFGetField( rtiff->tiff, - TIFFTAG_PHOTOSHOP, &data_length, &data ) && - data && - data_length ) { - void *data_copy; - - if( !(data_copy = vips_malloc( NULL, data_length )) ) + TIFFTAG_PHOTOSHOP, &data_length, &data ) ) { + if( vips_image_set_blob_copy( rtiff, out, + VIPS_META_PHOTOSHOP_NAME, data_length, data ) ) return( -1 ); - memcpy( data_copy, data, data_length ); - vips_image_set_blob( out, VIPS_META_PHOTOSHOP_NAME, - (VipsCallbackFn) vips_free, data_copy, data_length ); } /* IMAGEDESCRIPTION often has useful metadata. diff --git a/libvips/foreign/vipspng.c b/libvips/foreign/vipspng.c index 2e8569fb..058b1aa9 100644 --- a/libvips/foreign/vipspng.c +++ b/libvips/foreign/vipspng.c @@ -71,6 +71,8 @@ * - support png8 palette write with palette, colours, Q, dither * 25/8/18 * - support xmp read/write + * 21/11/18 + * - save xmp as string, not blob */ /* @@ -267,7 +269,8 @@ read_new_filename( VipsImage *out, const char *name, gboolean fail ) return( read ); } -/* Set the png text data as metadata on the vips image. +/* Set the png text data as metadata on the vips image. These are always + * null-terminated strings. */ static int vips__set_text( VipsImage *out, int i, const char *key, const char *text ) @@ -1104,7 +1107,6 @@ write_vips( Write *write, if( vips_image_get_string( in, VIPS_META_XMP_NAME, &str ) ) return( -1 ); - vips__png_set_text( write->pPng, write->pInfo, "XML:com.adobe.xmp", str ); } diff --git a/libvips/include/vips/header.h b/libvips/include/vips/header.h index 7fdf84f7..8a1bda37 100644 --- a/libvips/include/vips/header.h +++ b/libvips/include/vips/header.h @@ -193,8 +193,10 @@ void vips_image_set_area( VipsImage *image, const char *name, VipsCallbackFn free_fn, void *data ); int vips_image_get_area( const VipsImage *image, const char *name, void **data ); -void vips_image_set_blob( VipsImage *image, const char *name, - VipsCallbackFn free_fn, void *data, size_t length ); +void vips_image_set_blob( VipsImage *image, + const char *name, VipsCallbackFn free_fn, void *data, size_t length ); +void vips_image_set_blob_copy( VipsImage *image, + const char *name, void *data, size_t length ); int vips_image_get_blob( const VipsImage *image, const char *name, void **data, size_t *length ); diff --git a/libvips/iofuncs/header.c b/libvips/iofuncs/header.c index ae74a3b2..b12ccb9b 100644 --- a/libvips/iofuncs/header.c +++ b/libvips/iofuncs/header.c @@ -26,6 +26,8 @@ * - return header enums as enums, not ints * - vips_image_get_*() all convert everything to target type if they can * - rename "field" as "name" in docs + * 21/11/18 + * - get_string will allow BLOB, G_STRING and REF_STRING */ /* @@ -1347,8 +1349,8 @@ vips_image_get_area( const VipsImage *image, const char *name, void **data ) * See also: vips_image_get_blob(), vips_image_set(). */ void -vips_image_set_blob( VipsImage *image, const char *name, - VipsCallbackFn free_fn, void *data, size_t length ) +vips_image_set_blob( VipsImage *image, + const char *name, VipsCallbackFn free_fn, void *data, size_t length ) { GValue value = { 0 }; @@ -1358,6 +1360,70 @@ vips_image_set_blob( VipsImage *image, const char *name, g_value_unset( &value ); } +/** + * vips_image_set_blob_copy: (method) + * @image: image to attach the metadata to + * @name: metadata name + * @data: pointer to area of memory + * @length: length of memory area + * + * Attaches @blob as a metadata item on @image under the name @name, taking + * a copy of the memory area. A convenience function over vips_image_set_blob(). + * + * One more byte is allocated and a secret null added to the end, although + * this extra length is not recorded. This makes reading the value out later + * as a C string safer and more convenient. + * + * See also: vips_image_get_blob(), vips_image_set(). + */ +void +vips_image_set_blob_copy( VipsImage *image, + const char *name, void *data, size_t length ) +{ + void *data_copy; + + if( !data || + length == 0 ) + return( 0 ); + + if( !(data_copy = vips_malloc( NULL, length + 1 )) ) + return( -1 ); + memcpy( data_copy, data, length ); + ((unsigned char *) data_copy)[length] = '\0'; + + vips_image_set_blob( out, field, + (VipsCallbackFn) vips_free, data_copy, length ); + + return( 0 ); +} + +/* Set a blob on an image from a libtiff pointer/length. We don't null + * terminate, even though most of these things are strings, because we want to + * be able to write them back unaltered. + * + * Null-termination, checking for utf-8, checking for embedded null characters + * etc. must all be done on readout from the blob. + */ +static int +rtiff_set_blob( Rtiff *rtiff, VipsImage *out, const char *field, + uint32 data_length, unsigned char *data ) +{ + unsigned char *data_copy; + + if( !data || + data_length == 0 ) + return( 0 ); + + if( !(data_copy = vips_malloc( NULL, data_length )) ) + return( -1 ); + memcpy( data_copy, data, data_length ); + + vips_image_set_blob( out, field, + (VipsCallbackFn) vips_free, data_copy, data_length ); + + return( 0 ); +} + /** * vips_image_get_blob: (method) * @image: image to get the metadata from @@ -1498,7 +1564,8 @@ vips_image_set_double( VipsImage *image, const char *name, double d ) * * Gets @out from @im under the name @name. * The field must be of type - * VIPS_TYPE_REFSTRING. + * G_STRING, VIPS_TYPE_REFSTRING or VIPS_TYPE_BLOB. If it's a BLOB, it must be + * null-terminated. * * Do not free @out. * @@ -1513,21 +1580,45 @@ vips_image_get_string( const VipsImage *image, const char *name, const char **out ) { GValue value = { 0 }; - VipsArea *area; if( vips_image_get( image, name, &value ) ) return( -1 ); - if( G_VALUE_TYPE( &value ) != VIPS_TYPE_REF_STRING ) { + if( G_VALUE_TYPE( &value ) == VIPS_TYPE_BLOB ) { + char *str; + size_t length; + + str = vips_value_get_blob( &value, &length ); + if( length <= 0 || + str[length] != '\0' ) { + g_value_unset( &value ); + vips_error( "VipsImage", + _( "field \"%s\" is not null-terminated" ), + name ); + + return( -1 ); + } + + *out = str; + } + else if( G_VALUE_TYPE( &value ) == VIPS_TYPE_REF_STRING ) { + VipsArea *area; + + area = g_value_get_boxed( &value ); + *out = area->data; + } + else if( G_VALUE_TYPE( &value ) == G_TYPE_STRING ) { + *out = g_value_get_string( &value ); + } + else { vips_error( "VipsImage", _( "field \"%s\" is of type %s, not VipsRefString" ), name, g_type_name( G_VALUE_TYPE( &value ) ) ); g_value_unset( &value ); + return( -1 ); } - area = g_value_get_boxed( &value ); - *out = area->data; g_value_unset( &value ); return( 0 ); @@ -1541,9 +1632,9 @@ vips_image_get_string( const VipsImage *image, const char *name, * * Attaches @str as a metadata item on @image as @name. * A convenience - * function over vips_image_set() using an vips_ref_string. + * function over vips_image_set() using #VIPS_TYPE_REF_STRING. * - * See also: vips_image_get_double(), vips_image_set(), vips_ref_string + * See also: vips_image_get_double(), vips_image_set(). */ void vips_image_set_string( VipsImage *image, const char *name, const char *str )