vips_cache_remove() is called by signal without mutex lock

[MaxKellermann]

Every access to vips_cache_table must be protected by having the mutex vips_cache_lock locked. However, here a signal gets connected:

libvips/libvips/iofuncs/cache.c

Lines 631 to 632 in 1a2a4a4

	entry->invalidate_id = g_signal_connect( operation, "invalidate",
	G_CALLBACK( vips_cache_remove ), NULL );

This signal will be emitted without locking vips_cache_lock here:

libvips/libvips/iofuncs/operation.c

Line 629 in 1a2a4a4

g_signal_emit( operation, vips_operation_signals[SIG_INVALIDATE], 0 );

Thus vips_cache_remove() gets called without the lock, however it also doesn't acquire the lock, but accesses vips_cache_table:

libvips/libvips/iofuncs/cache.c

Lines 546 to 550 in 1a2a4a4

	static void
	vips_cache_remove( VipsOperation *operation )
	{
	VipsOperationCacheEntry *entry = (VipsOperationCacheEntry *)
	g_hash_table_lookup( vips_cache_table, operation );

This will eventually crash.
Note that adding the lock to vips_cache_remove() will still crash, because that will just wait until another thread releases the lock, and meanwhile the other thread may have destroyed the object.
(From GLib API documentation, it is not clear whether the whole g_signal library is thread-safe at all. And even if so, what happens if you disconnect a signal that is currently already running? Will it wait for completion? If yes, then you have just produced a deadlock, because g_signal_handler_disconnect() is called while holding the lock.)

[jcupitt]

Hi Max,

Yes, you're right, this should be improved.

How about making invalidate advisory rather than immediate? Operations could set a mark on cache entries meaning "this entry is no longer valid", then the entry would be removed next time that operation is looked up.

This breaks the loop (now the cache can remove operations, but operations can't remove cache entries), so it should be safer (I think). Everything is inside a mutex, at least.

I put your name on the change, I hope that's OK.

[jcupitt]

More generally, you're also right about signals and threading, it's a murky area.

Emit holds a ref to the object that's emitting during the process, and ref/unref is threadsafe, but that often won't be enough. Doing more than the minimum in a signal handler is asking for trouble.

[jcupitt]

I think this is at least partly addressed now in gut master. Please open a new issue if there is still a problem.