From 4839106a996851d71f4170f06b1a0e869f3c5c88 Mon Sep 17 00:00:00 2001 From: wm4 Date: Sun, 23 Feb 2020 17:52:21 +0100 Subject: client API: fix race condition on client exit The relatively recently added property update code has a race condition when clients exit. It still tried to access mpv_handle during and after it was destroyed. The reason is that it unlocks the lock for the mpv_handle list (while mpv_handle is locked), but nothing in mp_destroy_client() cares about this case. The latter function locks mpv_handle only before/while it makes sure all of its activity is coming to an end, such as asynchronous requests, and property updates that are in progress. It did not include the case when mp_client_send_property_changes() was still calling send_client_property_changes() with mpv_handle locked. Fix this by checking the mpv_handle.destroying field. This field can be set only when mpv_handle is locked. While we're checking the lock, the mpv_handle list is still locked, so at worst we might be at the point before mp_destroy_client() locks the list again and finally destroys the mpv_handle. This is a hard to reproduce race condition which I spotted only once in valgrind by chance, so the fix is unconfirmed. --- player/client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/player/client.c b/player/client.c index 14ee8fd7ee..67087eac42 100644 --- a/player/client.c +++ b/player/client.c @@ -1662,7 +1662,7 @@ void mp_client_send_property_changes(struct MPContext *mpctx) struct mpv_handle *ctx = clients->clients[n]; pthread_mutex_lock(&ctx->lock); - if (!ctx->has_pending_properties) { + if (!ctx->has_pending_properties || ctx->destroying) { pthread_mutex_unlock(&ctx->lock); continue; } -- cgit v1.2.3