0031-r2191-timer-fixes.patch

From 27a076f2f6c6007c0ba41d2868a803c4d841e815 Mon Sep 17 00:00:00 2001
From: nanang <nanang@localhost>
Date: Tue, 23 Apr 2019 08:42:45 +0000
Subject: [PATCH] Fixed #2191:  - Stricter double timer entry scheduling
 prevention.  - Integrate group lock in SIP transport, e.g: for add/dec ref,
 for timer scheduling.

---
 pjlib/include/pj/timer.h            |  2 +-
 pjlib/src/pj/timer.c                | 11 +++++++-
 pjsip/include/pjsip/sip_endpoint.h  | 39 +++++++++++++++++++++++++++++
 pjsip/include/pjsip/sip_transport.h |  2 ++
 pjsip/src/pjsip/sip_endpoint.c      | 36 ++++++++++++++++++++++++++
 pjsip/src/pjsip/sip_transport.c     | 36 +++++++++++++++++++++-----
 pjsip/src/pjsip/sip_transport_tcp.c | 10 +++++---
 pjsip/src/pjsip/sip_transport_tls.c | 14 ++++++++---
 pjsip/src/pjsip/sip_transport_udp.c |  2 ++
 9 files changed, 137 insertions(+), 15 deletions(-)

diff --git a/pjlib/include/pj/timer.h b/pjlib/include/pj/timer.h
index df6155a81..14857b872 100644
--- a/pjlib/include/pj/timer.h
+++ b/pjlib/include/pj/timer.h
@@ -252,9 +252,9 @@ PJ_DECL(pj_status_t) pj_timer_heap_schedule( pj_timer_heap_t *ht,
  *
  * @param ht        The timer heap.
  * @param entry     The entry to be registered.
+ * @param delay     The interval to expire.
  * @param id_val    The value to be set to the "id" field of the timer entry
  * 		    once the timer is scheduled.
- * @param delay     The interval to expire.
  * @param grp_lock  The group lock.
  *
  * @return          PJ_SUCCESS, or the appropriate error code.
diff --git a/pjlib/src/pj/timer.c b/pjlib/src/pj/timer.c
index f0a2cbbc9..cbdd9791f 100644
--- a/pjlib/src/pj/timer.c
+++ b/pjlib/src/pj/timer.c
@@ -502,7 +502,7 @@ static pj_status_t schedule_w_grp_lock(pj_timer_heap_t *ht,
     PJ_ASSERT_RETURN(entry->cb != NULL, PJ_EINVAL);
 
     /* Prevent same entry from being scheduled more than once */
-    PJ_ASSERT_RETURN(entry->_timer_id < 1, PJ_EINVALIDOP);
+    //PJ_ASSERT_RETURN(entry->_timer_id < 1, PJ_EINVALIDOP);
 
 #if PJ_TIMER_DEBUG
     entry->src_file = src_file;
@@ -512,6 +512,15 @@ static pj_status_t schedule_w_grp_lock(pj_timer_heap_t *ht,
     PJ_TIME_VAL_ADD(expires, *delay);
     
     lock_timer_heap(ht);
+
+    /* Prevent same entry from being scheduled more than once */
+    if (pj_timer_entry_running(entry)) {
+	unlock_timer_heap(ht);
+	PJ_LOG(3,(THIS_FILE, "Bug! Rescheduling outstanding entry (%p)",
+		  entry));
+	return PJ_EINVALIDOP;
+    }
+
     status = schedule_entry(ht, entry, &expires);
     if (status == PJ_SUCCESS) {
 	if (set_id)
diff --git a/pjsip/include/pjsip/sip_endpoint.h b/pjsip/include/pjsip/sip_endpoint.h
index 99683fbe1..ee967f8d9 100644
--- a/pjsip/include/pjsip/sip_endpoint.h
+++ b/pjsip/include/pjsip/sip_endpoint.h
@@ -138,6 +138,7 @@ PJ_DECL(pj_status_t) pjsip_endpt_handle_events( pjsip_endpoint *endpt,
 PJ_DECL(pj_status_t) pjsip_endpt_handle_events2(pjsip_endpoint *endpt,
 					        const pj_time_val *max_timeout,
 					        unsigned *count);
+
 /**
  * Schedule timer to endpoint's timer heap. Application must poll the endpoint
  * periodically (by calling #pjsip_endpt_handle_events) to ensure that the
@@ -166,6 +167,44 @@ PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer( pjsip_endpoint *endpt,
 						 const pj_time_val *delay );
 #endif
 
+/**
+ * Schedule timer to endpoint's timer heap with group lock. Application must
+ * poll the endpoint periodically (by calling #pjsip_endpt_handle_events) to
+ * ensure that the timer events are handled in timely manner. When the
+ * timeout for the timer has elapsed, the callback specified in the entry
+ * argument will be called. This function, like all other endpoint functions,
+ * is thread safe.
+ *
+ * @param endpt	    The endpoint.
+ * @param entry	    The timer entry.
+ * @param delay	    The relative delay of the timer.
+ * @param id_val    The value to be set to the "id" field of the timer entry
+ * 		    once the timer is scheduled.
+ * @param grp_lock  The group lock.
+ * @return	    PJ_OK (zero) if successfull.
+ */
+#if PJ_TIMER_DEBUG
+#define pjsip_endpt_schedule_timer_w_grp_lock(ept,ent,d,id,gl) \
+		pjsip_endpt_schedule_timer_w_grp_lock_dbg(ept,ent,d,id,gl,\
+							  __FILE__, __LINE__)
+
+PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock_dbg(
+						    pjsip_endpoint *endpt,
+						    pj_timer_entry *entry,
+						    const pj_time_val *delay,
+						    int id_val,
+						    pj_grp_lock_t *grp_lock,
+						    const char *src_file,
+						    int src_line);
+#else
+PJ_DECL(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock(
+						 pjsip_endpoint *endpt,
+						 pj_timer_entry *entry,
+						 const pj_time_val *delay,
+						 int id_val,
+						 pj_grp_lock_t *grp_lock );
+#endif
+
 /**
  * Cancel the previously registered timer.
  * This function, like all other endpoint functions, is thread safe.
diff --git a/pjsip/include/pjsip/sip_transport.h b/pjsip/include/pjsip/sip_transport.h
index addc8d521..d1ff3618b 100644
--- a/pjsip/include/pjsip/sip_transport.h
+++ b/pjsip/include/pjsip/sip_transport.h
@@ -810,6 +810,8 @@ struct pjsip_transport
     pj_pool_t		   *pool;	    /**< Pool used by transport.    */
     pj_atomic_t		   *ref_cnt;	    /**< Reference counter.	    */
     pj_lock_t		   *lock;	    /**< Lock object.		    */
+    pj_grp_lock_t	   *grp_lock;	    /**< Group lock for sync with
+					         ioqueue and timer.	    */
     pj_bool_t		    tracing;	    /**< Tracing enabled?	    */
     pj_bool_t		    is_shutdown;    /**< Being shutdown?	    */
     pj_bool_t		    is_destroying;  /**< Destroy in progress?	    */
diff --git a/pjsip/src/pjsip/sip_endpoint.c b/pjsip/src/pjsip/sip_endpoint.c
index d810781d5..71bc761c2 100644
--- a/pjsip/src/pjsip/sip_endpoint.c
+++ b/pjsip/src/pjsip/sip_endpoint.c
@@ -802,6 +802,42 @@ PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer( pjsip_endpoint *endpt,
 }
 #endif
 
+/*
+ * Schedule timer with group lock.
+ */
+#if PJ_TIMER_DEBUG
+PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock_dbg(
+						    pjsip_endpoint *endpt,
+						    pj_timer_entry *entry,
+						    const pj_time_val *delay,
+						    int id_val,
+						    pj_grp_lock_t *grp_lock,
+						    const char *src_file,
+						    int src_line)
+{
+    PJ_LOG(6, (THIS_FILE, "pjsip_endpt_schedule_timer_w_grp_lock"
+			  "(entry=%p, delay=%u.%u, grp_lock=%p)",
+			  entry, delay->sec, delay->msec, grp_lock));
+    return pj_timer_heap_schedule_w_grp_lock_dbg(endpt->timer_heap, entry,
+						 delay, id_val, grp_lock,
+						 src_file, src_line);
+}
+#else
+PJ_DEF(pj_status_t) pjsip_endpt_schedule_timer_w_grp_lock(
+						 pjsip_endpoint *endpt,
+						 pj_timer_entry *entry,
+						 const pj_time_val *delay,
+						 int id_val,
+						 pj_grp_lock_t *grp_lock )
+{
+    PJ_LOG(6, (THIS_FILE, "pjsip_endpt_schedule_timer_w_grp_lock"
+			  "(entry=%p, delay=%u.%u, grp_lock=%p)",
+			  entry, delay->sec, delay->msec, grp_lock));
+    return pj_timer_heap_schedule_w_grp_lock( endpt->timer_heap, entry,
+					      delay, id_val, grp_lock );
+}
+#endif
+
 /*
  * Cancel the previously registered timer.
  */
diff --git a/pjsip/src/pjsip/sip_transport.c b/pjsip/src/pjsip/sip_transport.c
index 67e235a39..529604399 100644
--- a/pjsip/src/pjsip/sip_transport.c
+++ b/pjsip/src/pjsip/sip_transport.c
@@ -1012,6 +1012,9 @@ static void transport_idle_callback(pj_timer_heap_t *timer_heap,
 
     PJ_UNUSED_ARG(timer_heap);
 
+    if (entry->id == PJ_FALSE)
+	return;
+
     entry->id = PJ_FALSE;
     pjsip_transport_destroy(tp);
 }
@@ -1049,6 +1052,10 @@ PJ_DEF(pj_status_t) pjsip_transport_add_ref( pjsip_transport *tp )
 
     PJ_ASSERT_RETURN(tp != NULL, PJ_EINVAL);
 
+    /* Add ref transport group lock, if any */
+    if (tp->grp_lock)
+	pj_grp_lock_add_ref(tp->grp_lock);
+
     /* Cache some vars for checking transport validity later */
     tpmgr = tp->tpmgr;
     key_len = sizeof(tp->key.type) + tp->addr_len;
@@ -1063,8 +1070,8 @@ PJ_DEF(pj_status_t) pjsip_transport_add_ref( pjsip_transport *tp )
 	    pj_atomic_get(tp->ref_cnt) == 1)
 	{
 	    if (tp->idle_timer.id != PJ_FALSE) {
-		pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
 		tp->idle_timer.id = PJ_FALSE;
+		pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
 	    }
 	}
 	pj_lock_release(tpmgr->lock);
@@ -1114,14 +1121,23 @@ PJ_DEF(pj_status_t) pjsip_transport_dec_ref( pjsip_transport *tp )
 		delay.msec = 0;
 	    }
 
-	    pj_assert(tp->idle_timer.id == 0);
-	    tp->idle_timer.id = PJ_TRUE;
-	    pjsip_endpt_schedule_timer(tp->tpmgr->endpt, &tp->idle_timer, 
-				       &delay);
+	    /* Avoid double timer entry scheduling */
+	    if (pj_timer_entry_running(&tp->idle_timer))
+		pjsip_endpt_cancel_timer(tp->tpmgr->endpt, &tp->idle_timer);
+
+	    pjsip_endpt_schedule_timer_w_grp_lock(tp->tpmgr->endpt,
+						  &tp->idle_timer,
+						  &delay,
+						  PJ_TRUE,
+						  tp->grp_lock);
 	}
 	pj_lock_release(tpmgr->lock);
     }
 
+    /* Dec ref transport group lock, if any */
+    if (tp->grp_lock)
+	pj_grp_lock_dec_ref(tp->grp_lock);
+
     return PJ_SUCCESS;
 }
 
@@ -1168,6 +1184,10 @@ PJ_DEF(pj_status_t) pjsip_transport_register( pjsip_tpmgr *mgr,
     /* Register new entry */
     pj_hash_set(tp->pool, mgr->table, &tp->key, key_len, hval, tp);
 
+    /* Add ref transport group lock, if any */
+    if (tp->grp_lock)
+	pj_grp_lock_add_ref(tp->grp_lock);
+
     pj_lock_release(mgr->lock);
 
     TRACE_((THIS_FILE,"Transport %s registered: type=%s, remote=%s:%d",
@@ -1199,8 +1219,8 @@ static pj_status_t destroy_transport( pjsip_tpmgr *mgr,
      */
     //pj_assert(tp->idle_timer.id == PJ_FALSE);
     if (tp->idle_timer.id != PJ_FALSE) {
-	pjsip_endpt_cancel_timer(mgr->endpt, &tp->idle_timer);
 	tp->idle_timer.id = PJ_FALSE;
+	pjsip_endpt_cancel_timer(mgr->endpt, &tp->idle_timer);
     }
 
     /*
@@ -1226,6 +1246,10 @@ static pj_status_t destroy_transport( pjsip_tpmgr *mgr,
     pj_lock_release(mgr->lock);
     pj_lock_release(tp->lock);
 
+    /* Dec ref transport group lock, if any */
+    if (tp->grp_lock)
+	pj_grp_lock_dec_ref(tp->grp_lock);
+
     /* Destroy. */
     return tp->destroy(tp);
 }
diff --git a/pjsip/src/pjsip/sip_transport_tcp.c b/pjsip/src/pjsip/sip_transport_tcp.c
index fe327459e..374bf461b 100644
--- a/pjsip/src/pjsip/sip_transport_tcp.c
+++ b/pjsip/src/pjsip/sip_transport_tcp.c
@@ -692,6 +692,8 @@ static pj_status_t tcp_create( struct tcp_listener *listener,
     pj_grp_lock_add_ref(tcp->grp_lock);
     pj_grp_lock_add_handler(tcp->grp_lock, pool, tcp, &tcp_on_destroy);
 
+    tcp->base.grp_lock = tcp->grp_lock;
+
     /* Create active socket */
     pj_activesock_cfg_default(&asock_cfg);
     asock_cfg.async_cnt = 1;
@@ -746,7 +748,11 @@ static pj_status_t tcp_create( struct tcp_listener *listener,
     return PJ_SUCCESS;
 
 on_error:
-    tcp_destroy(&tcp->base, status);
+    if (tcp->grp_lock && pj_grp_lock_get_ref(tcp->grp_lock))
+	tcp_destroy(&tcp->base, status);
+    else
+    	tcp_on_destroy(tcp);
+
     return status;
 }
 
@@ -867,8 +873,6 @@ static pj_status_t tcp_destroy(pjsip_transport *transport,
 	tcp->grp_lock = NULL;
 	pj_grp_lock_dec_ref(grp_lock);
 	/* Transport may have been deleted at this point */
-    } else {
-	tcp_on_destroy(tcp);
     }
 
     return PJ_SUCCESS;
diff --git a/pjsip/src/pjsip/sip_transport_tls.c b/pjsip/src/pjsip/sip_transport_tls.c
index d3afae5e9..dd3a4d639 100644
--- a/pjsip/src/pjsip/sip_transport_tls.c
+++ b/pjsip/src/pjsip/sip_transport_tls.c
@@ -165,6 +165,10 @@ static pj_status_t tls_create(struct tls_listener *listener,
 			      struct tls_transport **p_tls);
 
 
+/* Clean up TLS resources */
+static void tls_on_destroy(void *arg);
+
+
 static void tls_perror(const char *sender, const char *title,
 		       pj_status_t status)
 {
@@ -893,7 +897,11 @@ static pj_status_t tls_create( struct tls_listener *listener,
     return PJ_SUCCESS;
 
 on_error:
-    tls_destroy(&tls->base, status);
+    if (tls->grp_lock && pj_grp_lock_get_ref(tls->grp_lock))
+	tls_destroy(&tls->base, status);
+    else
+    	tls_on_destroy(tls);
+
     return status;
 }
 
@@ -1048,8 +1056,6 @@ static pj_status_t tls_destroy(pjsip_transport *transport,
 	tls->grp_lock = NULL;
 	pj_grp_lock_dec_ref(grp_lock);
 	/* Transport may have been deleted at this point */
-    } else {
-	tls_on_destroy(tls);
     }
 
     return PJ_SUCCESS;
@@ -1235,7 +1241,7 @@ static pj_status_t lis_create_transport(pjsip_tpfactory *factory,
     pj_ssl_sock_set_user_data(tls->ssock, tls);
 
     /* Set up the group lock */
-    tls->grp_lock = glock;
+    tls->grp_lock = tls->base.grp_lock = glock;
     pj_grp_lock_add_ref(tls->grp_lock);
     pj_grp_lock_add_handler(tls->grp_lock, pool, tls, &tls_on_destroy);
 
diff --git a/pjsip/src/pjsip/sip_transport_udp.c b/pjsip/src/pjsip/sip_transport_udp.c
index dbda474cf..b82d519c9 100644
--- a/pjsip/src/pjsip/sip_transport_udp.c
+++ b/pjsip/src/pjsip/sip_transport_udp.c
@@ -691,6 +691,8 @@ static pj_status_t register_to_ioqueue(struct udp_transport *tp)
 	pj_grp_lock_add_ref(tp->grp_lock);
 	pj_grp_lock_add_handler(tp->grp_lock, tp->base.pool, tp,
 				&udp_on_destroy);
+
+	tp->base.grp_lock = tp->grp_lock;
     }
     
     /* Register to ioqueue. */
-- 
2.20.1