Merge pull request from GHSA-f76w-fh7c-pc66

* Add group lock to media transport

* Also add group lock to SRTP-DTLS

* Put lock protection to avoid race condition between destroy() & dtls_on_recv()

pjmedia/include/pjmedia/transport.h

@@ -513,6 +513,9 @@ struct pjmedia_transport
 
     /** Application/user data */
     void                    *user_data;
+
+    /** Group lock, for synchronization between destroy() & callbacks. */
+    pj_grp_lock_t           *grp_lock;
 };
 
 /**

pjmedia/src/pjmedia/transport_adapter_sample.c

@@ -106,6 +106,9 @@ struct tp_adapter
 };
 
 
+static void adapter_on_destroy(void *arg);
+
+
 /*
  * Create the adapter.
  */
@@ -135,6 +138,15 @@ PJ_DEF(pj_status_t) pjmedia_tp_adapter_create( pjmedia_endpt *endpt,
     adapter->slave_tp = transport;
     adapter->del_base = del_base;
 
+    /* Setup group lock handler for destroy and callback synchronization */
+    if (transport && transport->grp_lock) {
+        pj_grp_lock_t *grp_lock = transport->grp_lock;
+
+        adapter->base.grp_lock = grp_lock;
+        pj_grp_lock_add_ref(grp_lock);
+        pj_grp_lock_add_handler(grp_lock, pool, adapter, &adapter_on_destroy);
+    }
+
     /* Done */
     *p_tp = &adapter->base;
     return PJ_SUCCESS;
@@ -421,6 +433,14 @@ static pj_status_t transport_simulate_lost(pjmedia_transport *tp,
     return pjmedia_transport_simulate_lost(adapter->slave_tp, dir, pct_lost);
 }
 
+
+static void adapter_on_destroy(void *arg)
+{
+    struct tp_adapter *adapter = (struct tp_adapter*)arg;
+
+    pj_pool_release(adapter->pool);
+}
+
 /*
  * destroy() is called when the transport is no longer needed.
  */
@@ -433,8 +453,11 @@ static pj_status_t transport_destroy  (pjmedia_transport *tp)
         pjmedia_transport_close(adapter->slave_tp);
     }
 
-    /* Self destruct.. */
-    pj_pool_release(adapter->pool);
+    if (adapter->base.grp_lock) {
+        pj_grp_lock_dec_ref(adapter->base.grp_lock);
+    } else {
+        adapter_on_destroy(tp);
+    }
 
     return PJ_SUCCESS;
 }

pjmedia/src/pjmedia/transport_ice.c

@@ -338,6 +338,7 @@ PJ_DEF(pj_status_t) pjmedia_ice_create3(pjmedia_endpt *endpt,
         pj_grp_lock_t *grp_lock = pj_ice_strans_get_grp_lock(tp_ice->ice_st);
         pj_grp_lock_add_ref(grp_lock);
         pj_grp_lock_add_handler(grp_lock, pool, tp_ice, &tp_ice_on_destroy);
+        tp_ice->base.grp_lock = grp_lock;
     }
 
     /* Done */
@@ -2736,6 +2737,8 @@ static pj_status_t transport_simulate_lost(pjmedia_transport *tp,
 static void tp_ice_on_destroy(void *arg)
 {
     struct transport_ice *tp_ice = (struct transport_ice*)arg;
+
+    PJ_LOG(4, (tp_ice->base.name, "ICE transport destroyed"));
     pj_pool_safe_release(&tp_ice->pool);
 }
 
@@ -2746,6 +2749,8 @@ static pj_status_t transport_destroy(pjmedia_transport *tp)
 {
     struct transport_ice *tp_ice = (struct transport_ice*)tp;
 
+    PJ_LOG(4, (tp_ice->base.name, "Destroying ICE transport"));
+
     /* Reset callback and user data */
     pj_bzero(&tp_ice->cb, sizeof(tp_ice->cb));
     tp_ice->base.user_data = NULL;

pjmedia/src/pjmedia/transport_loop.c

@@ -130,6 +130,7 @@ static pjmedia_transport_op transport_udp_op =
     &transport_attach2
 };
 
+static void tp_loop_on_destroy(void *arg);
 
 /**
  * Initialize loopback media transport setting with its default values.
@@ -164,6 +165,8 @@ pjmedia_transport_loop_create2(pjmedia_endpt *endpt,
 {
     struct transport_loop *tp;
     pj_pool_t *pool;
+    pj_grp_lock_t *grp_lock;
+    pj_status_t status;
 
     /* Sanity check */
     PJ_ASSERT_RETURN(endpt && p_tp, PJ_EINVAL);
@@ -179,6 +182,14 @@ pjmedia_transport_loop_create2(pjmedia_endpt *endpt,
     tp->base.op = &transport_udp_op;
     tp->base.type = PJMEDIA_TRANSPORT_TYPE_UDP;
 
+    /* Create group lock */
+    status = pj_grp_lock_create(pool, NULL, &grp_lock);
+    if (status != PJ_SUCCESS)
+        return status;
+
+    pj_grp_lock_add_ref(grp_lock);
+    pj_grp_lock_add_handler(grp_lock, pool, tp, &tp_loop_on_destroy);
+
     if (opt) {
         tp->setting = *opt;
     } else {
@@ -222,17 +233,25 @@ PJ_DEF(pj_status_t) pjmedia_transport_loop_disable_rx( pjmedia_transport *tp,
     return PJ_ENOTFOUND;
 }
 
+
+static void tp_loop_on_destroy(void *arg)
+{
+    struct transport_loop *loop = (struct transport_loop*) arg;
+
+    PJ_LOG(4, (loop->base.name, "Loop transport destroyed"));
+    pj_pool_release(loop->pool);
+}
+
+
 /**
  * Close loopback transport.
  */
 static pj_status_t transport_destroy(pjmedia_transport *tp)
 {
-    struct transport_loop *loop = (struct transport_loop*) tp;
-
     /* Sanity check */
     PJ_ASSERT_RETURN(tp, PJ_EINVAL);
 
-    pj_pool_release(loop->pool);
+    pj_grp_lock_dec_ref(tp->grp_lock);
 
     return PJ_SUCCESS;
 }
@@ -378,6 +397,8 @@ static pj_status_t transport_send_rtp( pjmedia_transport *tp,
         }
     }
 
+    pj_grp_lock_add_ref(tp->grp_lock);
+
     /* Distribute to users */
     for (i=0; i<loop->user_cnt; ++i) {
         if (loop->users[i].rx_disabled) continue;
@@ -395,6 +416,8 @@ static pj_status_t transport_send_rtp( pjmedia_transport *tp,
         }
     }
 
+    pj_grp_lock_dec_ref(tp->grp_lock);
+
     return PJ_SUCCESS;
 }
 
@@ -420,13 +443,17 @@ static pj_status_t transport_send_rtcp2(pjmedia_transport *tp,
     PJ_UNUSED_ARG(addr_len);
     PJ_UNUSED_ARG(addr);
 
+    pj_grp_lock_add_ref(tp->grp_lock);
+
     /* Distribute to users */
     for (i=0; i<loop->user_cnt; ++i) {
         if (!loop->users[i].rx_disabled && loop->users[i].rtcp_cb)
             (*loop->users[i].rtcp_cb)(loop->users[i].user_data, (void*)pkt,
                                       size);
     }
 
+    pj_grp_lock_dec_ref(tp->grp_lock);
+
     return PJ_SUCCESS;
 }
 

pjmedia/src/pjmedia/transport_srtp.c

@@ -434,6 +434,8 @@ static pj_status_t create_srtp_ctx(transport_srtp *srtp,
 /* Destroy SRTP context */
 static void destroy_srtp_ctx(transport_srtp *p_srtp, srtp_context *ctx);
 
+/* SRTP destroy handler */
+static void srtp_on_destroy(void *arg);
 
 /* This function may also be used by other module, e.g: pjmedia/errno.c,
  * it should have C compatible declaration.
@@ -805,6 +807,13 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_create(
     /* Set underlying transport */
     srtp->member_tp = tp;
 
+    /* Setup group lock handler for destroy and callback synchronization */
+    if (tp && tp->grp_lock) {
+        srtp->base.grp_lock = tp->grp_lock;
+        pj_grp_lock_add_ref(tp->grp_lock);
+        pj_grp_lock_add_handler(tp->grp_lock, pool, srtp, &srtp_on_destroy);
+    }
+
     /* Initialize peer's SRTP usage mode. */
     srtp->peer_use = srtp->setting.use;
 
@@ -839,6 +848,8 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_create(
     /* Done */
     *p_tp = &srtp->base;
 
+    PJ_LOG(4, (srtp->pool->obj_name, "SRTP transport created"));
+
     return PJ_SUCCESS;
 }
 
@@ -1459,6 +1470,19 @@ static pj_status_t transport_simulate_lost(pjmedia_transport *tp,
     return pjmedia_transport_simulate_lost(srtp->member_tp, dir, pct_lost);
 }
 
+
+/* SRTP real destroy */
+static void srtp_on_destroy(void *arg)
+{
+    transport_srtp *srtp = (transport_srtp*)arg;
+
+    PJ_LOG(4, (srtp->pool->obj_name, "SRTP transport destroyed"));
+
+    pj_lock_destroy(srtp->mutex);
+    pj_pool_safe_release(&srtp->pool);
+}
+
+
 static pj_status_t transport_destroy  (pjmedia_transport *tp)
 {
     transport_srtp *srtp = (transport_srtp *) tp;
@@ -1467,6 +1491,8 @@ static pj_status_t transport_destroy  (pjmedia_transport *tp)
 
     PJ_ASSERT_RETURN(tp, PJ_EINVAL);
 
+    PJ_LOG(4, (srtp->pool->obj_name, "Destroying SRTP transport"));
+
     /* Close all keying. Note that any keying should not be destroyed before
      * SRTP transport is destroyed as re-INVITE may initiate new keying method
      * without destroying SRTP transport.
@@ -1481,12 +1507,25 @@ static pj_status_t transport_destroy  (pjmedia_transport *tp)
 
     status = pjmedia_transport_srtp_stop(tp);
 
-    /* In case mutex is being acquired by other thread */
-    pj_lock_acquire(srtp->mutex);
-    pj_lock_release(srtp->mutex);
+    if (srtp->base.grp_lock) {
+        pj_grp_lock_dec_ref(srtp->base.grp_lock);
+    } else {
+        /* Only get here when the underlying transport does not have
+         * a group lock, race condition with callbacks may occur.
+         * Currently UDP, ICE, and loop have a group lock already.
+         */
+        PJ_LOG(4,(srtp->pool->obj_name,
+                  "Warning: underlying transport does not have group lock"));
 
-    pj_lock_destroy(srtp->mutex);
-    pj_pool_release(srtp->pool);
+        /* In case mutex is being acquired by other thread.
+         * An effort to synchronize destroy() & callbacks when the underlying
+         * transport does not provide a group lock.
+         */
+        pj_lock_acquire(srtp->mutex);
+        pj_lock_release(srtp->mutex);
+
+        srtp_on_destroy(srtp);
+    }
 
     return status;
 }