From fda5e5c402b1cb57dda010d21c656e6f9d8d1544 Mon Sep 17 00:00:00 2001
From: John Cupitt <jcupitt@gmail.com>
Date: Tue, 21 Jan 2020 08:29:59 +0000
Subject: [PATCH] lock for metadata changes

Another attempt at fixing crashes on metadata chenage in highly threaded
applications.

Global lock around set, remove and copy metadata. This is crude, but
simple, the performance impact should be small, and ought to resolve the
problem.

We'll do something better for the next version.

see https://github.com/lovell/sharp/issues/1986
---
 ChangeLog                |  1 +
 libvips/iofuncs/header.c | 42 ++++++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 3ebe6539..9c168622 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -5,6 +5,7 @@
 - fix a warning with magicksave with no delay array [chregu]
 - fix a race in tiled tiff load [kleisauke]
 - better imagemagick init [LebronCurry]
+- lock for metadata changes [jcupitt]
 
 20/6/19 started 8.9.0
 - add vips_image_get/set_array_int()
diff --git a/libvips/iofuncs/header.c b/libvips/iofuncs/header.c
index 6c77497a..6a6347a3 100644
--- a/libvips/iofuncs/header.c
+++ b/libvips/iofuncs/header.c
@@ -930,8 +930,14 @@ meta_cp( VipsImage *dst, const VipsImage *src )
 		/* Loop, copying fields.
 		 */
 		meta_init( dst );
+
+		/* We lock with vips_image_set() to stop races in highly-
+		 * threaded applications.
+		 */
+		g_mutex_lock( vips__global_lock );
 		vips_slist_map2( src->meta_traverse,
 			(VipsSListMap2Fn) meta_cp_field, dst, NULL );
+		g_mutex_unlock( vips__global_lock );
 	}
 
 	return( 0 );
@@ -1019,7 +1025,7 @@ vips_image_set( VipsImage *image, const char *name, GValue *value )
 	g_assert( name );
 	g_assert( value );
 
-        /* If this image is shared, block metadata changes. 
+        /* If this image is shared, block metadata changes.
          */
         if( G_OBJECT( image )->ref_count > 1 ) {
                 g_warning( "can't set metadata \"%s\" on shared image", name );
@@ -1027,7 +1033,17 @@ vips_image_set( VipsImage *image, const char *name, GValue *value )
         }
 
 	meta_init( image );
+
+	/* We lock between modifying metadata and copying metadata between
+	 * images, see meta_cp().
+	 *
+	 * This prevents modification of metadata by one thread racing with
+	 * metadata copy on another -- this can lead to crashes in
+	 * highly-threaded applications.
+	 */
+	g_mutex_lock( vips__global_lock );
 	(void) meta_new( image, name, value );
+	g_mutex_unlock( vips__global_lock );
 
 	/* If we're setting an EXIF data block, we need to automatically expand 
 	 * out all the tags. This will set things like xres/yres too.
@@ -1042,6 +1058,7 @@ vips_image_set( VipsImage *image, const char *name, GValue *value )
 #ifdef DEBUG
 	meta_sanity( image );
 #endif /*DEBUG*/
+
 }
 
 /* Unforunately gvalue seems to have no way of doing this. Just handle the vips
@@ -1219,19 +1236,32 @@ vips_image_get_typeof( const VipsImage *image, const char *name )
 gboolean
 vips_image_remove( VipsImage *image, const char *name )
 {
+	gboolean result;
+
+	result = FALSE;
+
         /* If this image is shared, block metadata changes. 
          */
         if( G_OBJECT( image )->ref_count > 1 ) {
                 g_warning( "can't remove metadata \"%s\" on shared image", 
 			name );
-                return( FALSE );
+                return( result );
         }
 
-	if( image->meta && 
-		g_hash_table_remove( image->meta, name ) )
-		return( TRUE );
+	if( image->meta ) {
+		/* We lock between modifying metadata and copying metadata 
+		 * between images, see meta_cp().
+		 *
+		 * This prevents modification of metadata by one thread 
+		 * racing with metadata copy on another -- this can lead to 
+		 * crashes in highly-threaded applications.
+		 */
+		g_mutex_lock( vips__global_lock );
+		result = g_hash_table_remove( image->meta, name );
+		g_mutex_unlock( vips__global_lock );
+	}
 
-	return( FALSE );
+	return( result );
 }
 
 /* Deprecated header fields we hide from _map.