From: Mike Brady <4265913+mikebrady@users.noreply.github.com>
Date: Thu, 21 Sep 2023 15:04:23 +0000 (+0100)
Subject: Change principal_conn_lock from a regular mutex to a read-write mutex, so it can... 
X-Git-Tag: 4.3.2^2~46
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=152ddbb87df2bbccd0885454542f30a89b5b8f93;p=thirdparty%2Fshairport-sync.git

Change principal_conn_lock from a regular mutex to a read-write mutex, so it can be used to check and hold the current principal_conn unless it's being altered in get_play_lock or a release_play_lock.
Only allow access to the config.airplay_statusflags, build_bonjour_strings(NULL), mdns_update(NULL, secondary_txt_records) when read_lock is acquired on the principal_conn_lock.
Only allow changes to config.airplay_statusflags and only allow access to mdns_update if you are the principal conn.

The bonjour status flags and the info response plist calculations are still a dirty rotten hack.
---

diff --git a/rtsp.c b/rtsp.c
index 56e06c93..5c750a2a 100644
--- a/rtsp.c
+++ b/rtsp.c
@@ -141,12 +141,11 @@ rtsp_conn_info **conns;
 
 int metadata_running = 0;
 
-// always lock this when trying to make a conn the principal conn,
-// e.g. during an ANNOUNCE (Classic AirPlay) or SETUP (AirPlay 2)
-pthread_mutex_t principal_conn_acquisition_lock = PTHREAD_MUTEX_INITIALIZER;
 
 // always lock this when accessing the principal conn value
-pthread_mutex_t principal_conn_lock = PTHREAD_MUTEX_INITIALIZER;
+// use a read lock when consulting and holding it
+// use a write lock if you want to change it
+pthread_rwlock_t principal_conn_lock = PTHREAD_RWLOCK_INITIALIZER;
 
 // always lock this when accessing the list of connection threads
 pthread_mutex_t conns_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -565,23 +564,24 @@ void cancel_all_RTSP_threads(airplay_stream_c stream_category, int except_this_o
 // other devices.
 
 void release_play_lock(rtsp_conn_info *conn) {
-  pthread_cleanup_debug_mutex_lock(&principal_conn_lock, 100000,
-                                   1); // don't let the principal_conn be changed
+  // no need thread cancellation points in here
+  pthread_rwlock_wrlock(&principal_conn_lock);
   if (principal_conn == conn) {        // if we have the player
     if (conn != NULL)
       debug(2, "Connection %d: principal_conn released.", conn->connection_number);
     principal_conn = NULL; // let it go
   }
-  pthread_cleanup_pop(1); // release the principal_conn lock
+  pthread_rwlock_unlock(&principal_conn_lock);
 }
 
 // stop the current principal_conn from playing if necessary and make conn the principal_conn.
 
 int get_play_lock(rtsp_conn_info *conn, int allow_session_interruption) {
   int response = 0;
-  pthread_cleanup_debug_mutex_lock(&principal_conn_lock, 100000, 1);
+  pthread_rwlock_wrlock(&principal_conn_lock);
+  pthread_cleanup_push(rwlock_unlock,(void *)&principal_conn_lock);
   if (principal_conn != NULL)
-    debug(2, "Connection %d: is requested to relinquish principal_conn.",
+    debug(1, "Connection %d: is requested to relinquish principal_conn.",
           principal_conn->connection_number);
   if (conn != NULL)
     debug(2, "Connection %d: request to acquire principal_conn.", conn->connection_number);
@@ -594,10 +594,10 @@ int get_play_lock(rtsp_conn_info *conn, int allow_session_interruption) {
       warn("Connection %d: request to re-acquire principal_conn!",
            principal_conn->connection_number);
   } else if (allow_session_interruption != 0) {
-    player_stop(principal_conn);
-    debug(2, "Connection %d: termination requested.", principal_conn->connection_number);
-    pthread_cancel(principal_conn->thread);
-    usleep(2000000);       // don't know why this delay is needed.
+    rtsp_conn_info *previous_principal_conn = principal_conn;
+    principal_conn = NULL; // no longer the principal conn
+    pthread_cancel(previous_principal_conn->thread);
+    // usleep(1000000);       // don't know why this delay is needed.
     principal_conn = conn; // make the conn the new principal_conn
     response = 1;          // interrupted an existing session
   } else {
@@ -766,6 +766,11 @@ void msg_retain(rtsp_message *msg) {
 }
 
 rtsp_message *msg_init(void) {
+  // no thread cancellation points here
+  int rc = pthread_mutex_lock(&reference_counter_lock);
+  if (rc)
+    debug(1, "Error %d locking reference counter lock");
+
   rtsp_message *msg = malloc(sizeof(rtsp_message));
   if (msg) {
     memset(msg, 0, sizeof(rtsp_message));
@@ -776,6 +781,11 @@ rtsp_message *msg_init(void) {
     die("msg_init -- can not allocate memory for rtsp_message %d.", msg_indexes);
   }
   // debug(1,"msg_init -- create item %d.", msg->index_number);
+  
+  rc = pthread_mutex_unlock(&reference_counter_lock);
+  if (rc)
+    debug(1, "Error %d unlocking reference counter lock");
+
   return msg;
 }
 
@@ -1707,6 +1717,14 @@ void handle_get_info(__attribute((unused)) rtsp_conn_info *conn, rtsp_message *r
     } else {
       void *qualifier_response_data = NULL;
       size_t qualifier_response_data_length = 0;
+      
+      
+      pthread_rwlock_rdlock(&principal_conn_lock); // don't let the principal_conn be changed
+      pthread_cleanup_push(rwlock_unlock,(void *)&principal_conn_lock);
+
+      
+      
+      
       if (add_pstring_to_malloc("acl=0", &qualifier_response_data,
                                 &qualifier_response_data_length) == 0)
         debug(1, "Problem");
@@ -1769,6 +1787,7 @@ void handle_get_info(__attribute((unused)) rtsp_conn_info *conn, rtsp_message *r
       free(vs);
       // pkString_make(pkString, sizeof(pkString), config.airplay_device_id);
       // plist_dict_set_item(response_plist, "pk", plist_new_string(pkString));
+      pthread_cleanup_pop(1); // release the principal_conn lock
       plist_to_bin(response_plist, &resp->content, &resp->contentlength);
       if (resp->contentlength == 0)
         debug(1, "GET /info Stage 1: response bplist not created!");
@@ -2685,11 +2704,6 @@ void teardown_phase_two(rtsp_conn_info *conn) {
 #endif
     }
     conn->groupContainsGroupLeader = 0;
-    config.airplay_statusflags &= (0xffffffff - (1 << 11)); // DeviceSupportsRelay
-    build_bonjour_strings(NULL);
-    debug(2, "Connection %d: TEARDOWN mdns_update on %s.", conn->connection_number,
-          get_category_string(conn->airplay_stream_category));
-    mdns_update(NULL, secondary_txt_records);
     if (conn->dacp_active_remote != NULL) {
       free(conn->dacp_active_remote);
       conn->dacp_active_remote = NULL;
@@ -2701,13 +2715,11 @@ void teardown_phase_two(rtsp_conn_info *conn) {
 void handle_teardown_2(rtsp_conn_info *conn, __attribute__((unused)) rtsp_message *req,
                        rtsp_message *resp) {
 
-  debug(2, "Connection %d: TEARDOWN %s.", conn->connection_number,
+  debug(1, "Connection %d: TEARDOWN %s.", conn->connection_number,
         get_category_string(conn->airplay_stream_category));
   debug_log_rtsp_message(2, "TEARDOWN: ", req);
-  // if (have_player(conn)) {
   resp->respcode = 200;
   msg_add_header(resp, "Connection", "close");
-
   plist_t messagePlist = plist_from_rtsp_content(req);
   if (messagePlist != NULL) {
     // now see if the incoming plist contains a "streams" array
@@ -2726,6 +2738,18 @@ void handle_teardown_2(rtsp_conn_info *conn, __attribute__((unused)) rtsp_messag
             get_category_string(conn->airplay_stream_category));
       teardown_phase_one(conn); // try to do phase one anyway
       teardown_phase_two(conn);
+      
+      // only update these things if you're (still) the principal conn
+      pthread_rwlock_rdlock(&principal_conn_lock); // don't let the principal_conn be changed
+      pthread_cleanup_push(rwlock_unlock,(void *)&principal_conn_lock);
+      if ((principal_conn == conn) && (conn->airplay_stream_category == ptp_stream)) {
+        config.airplay_statusflags &= (0xffffffff - (1 << 11)); // DeviceSupportsRelay
+        build_bonjour_strings(conn);
+        debug(2, "Connection %d: TEARDOWN mdns_update on %s.", conn->connection_number,
+              get_category_string(conn->airplay_stream_category));
+        mdns_update(NULL, secondary_txt_records);
+      }
+      pthread_cleanup_pop(1); // release the principal_conn lock
       debug(2, "Connection %d: TEARDOWN %s -- close the connection complete",
             conn->connection_number, get_category_string(conn->airplay_stream_category));
       release_play_lock(conn);
@@ -2737,6 +2761,7 @@ void handle_teardown_2(rtsp_conn_info *conn, __attribute__((unused)) rtsp_messag
     debug(1, "Connection %d: missing plist!", conn->connection_number);
     resp->respcode = 451; // don't know what to do here
   }
+
   // debug(1,"Bogus exit for valgrind -- remember to comment it out!.");
   // exit(EXIT_SUCCESS); //
 }
@@ -3081,12 +3106,18 @@ void handle_setup_2(rtsp_conn_info *conn, rtsp_message *req, rtsp_message *resp)
                                                   conn->connection_number); // kill all the other
                  listeners
               */
+              // only update these things if you're (still) the principal conn
+              pthread_rwlock_wrlock(&principal_conn_lock); // don't let the principal_conn be changed
+              pthread_cleanup_push(rwlock_unlock,(void *)&principal_conn_lock);
+              if (principal_conn == conn) {
+                    config.airplay_statusflags |= 1 << 11; // DeviceSupportsRelay
+                    build_bonjour_strings(conn);
+                    debug(2, "Connection %d: SETUP mdns_update on %s.", conn->connection_number,
+                          get_category_string(conn->airplay_stream_category));
+                    mdns_update(NULL, secondary_txt_records);
+              }
+              pthread_cleanup_pop(1); // release the principal_conn lock
 
-              config.airplay_statusflags |= 1 << 11; // DeviceSupportsRelay
-              build_bonjour_strings(conn);
-              debug(2, "Connection %d: SETUP mdns_update on %s.", conn->connection_number,
-                    get_category_string(conn->airplay_stream_category));
-              mdns_update(NULL, secondary_txt_records);
               resp->respcode = 200;
             } else {
               debug(1, "SETUP on Connection %d: PTP setup -- no timingPeerInfo plist.",
@@ -3580,8 +3611,8 @@ void handle_set_parameter_parameter(rtsp_conn_info *conn, rtsp_message *req,
         shairport_sync_set_volume(shairportSyncSkeleton, volume);
       } else {
 #endif
-        pthread_cleanup_debug_mutex_lock(&principal_conn_lock, 100000,
-                                         1); // don't let the principal_conn be changed
+        pthread_rwlock_rdlock(&principal_conn_lock); // don't let the principal_conn be changed
+        pthread_cleanup_push(rwlock_unlock,(void *)&principal_conn_lock);
         if (principal_conn == conn) {
           player_volume(volume, conn);
         }