Changeset 4862 for pjproject/trunk

Timestamp:

Jun 19, 2014 9:42:02 AM (10 years ago)

Author:

nanang

Message:

Fix #1773: Added group lock to SIP transport to avoid race condition between transport callback and destroy.

Location:

pjproject/trunk

Files:

• pjlib/include/pj/ssl_sock.h (modified) (2 diffs)
• pjlib/src/pj/ssl_sock_ossl.c (modified) (5 diffs)
• pjsip/src/pjsip/sip_transport_tcp.c (modified) (12 diffs)
• pjsip/src/pjsip/sip_transport_tls.c (modified) (12 diffs)
• pjsip/src/pjsip/sip_transport_udp.c (modified) (5 diffs)

pjproject/trunk/pjlib/include/pj/ssl_sock.h

@@ -553 +553 @@
     unsigned long       last_native_err;
 
+    /**
+     * Group lock assigned to the ioqueue key.
+     */
+    pj_grp_lock_t *grp_lock;
+
 } pj_ssl_sock_info;
 
@@ -561 +566 @@
 typedef struct pj_ssl_sock_param
 {
+    /**
+     * Optional group lock to be assigned to the ioqueue key.
+     *
+     * Note that when a secure socket listener is configured with a group
+     * lock, any new secure socket of an accepted incoming connection
+     * will have its own group lock created automatically by the library,
+     * this group lock can be queried via pj_ssl_sock_get_info() in the info
+     * field pj_ssl_sock_info::grp_lock.
+     */
+    pj_grp_lock_t *grp_lock;
+
     /**
      * Specifies socket address family, either pj_AF_INET() and pj_AF_INET6().

pjproject/trunk/pjlib/src/pj/ssl_sock_ossl.c

@@ -1694 +1694 @@
     asock_cfg.concurrency = ssock->param.concurrency;
     asock_cfg.whole_data = PJ_TRUE;
+    
+    /* If listener socket has group lock, automatically create group lock
+     * for the new socket.
+     */
+    if (ssock_parent->param.grp_lock) {
+        pj_grp_lock_t *glock;
+
+        status = pj_grp_lock_create(ssock->pool, NULL, &glock);
+        if (status != PJ_SUCCESS)
+            goto on_return;
+
+        /* Temporarily add ref the group lock until active socket creation,
+         * to make sure that group lock is destroyed if the active socket
+         * creation fails.
+         */
+        pj_grp_lock_add_ref(glock);
+        asock_cfg.grp_lock = ssock->param.grp_lock = glock;
+    }
 
     pj_bzero(&asock_cb, sizeof(asock_cb));
@@ -1707 +1725 @@
                                   ssock,
                                   &ssock->asock);
+
+    /* This will destroy the group lock if active socket creation fails */
+    if (asock_cfg.grp_lock) {
+        pj_grp_lock_dec_ref(asock_cfg.grp_lock);
+    }
 
     if (status != PJ_SUCCESS)
@@ -2122 +2145 @@
     /* Last known OpenSSL error code */
     info->last_native_err = ssock->last_err;
+
+    /* Group lock */
+    info->grp_lock = ssock->param.grp_lock;
 
     return PJ_SUCCESS;
@@ -2489 +2515 @@
     asock_cfg.concurrency = ssock->param.concurrency;
     asock_cfg.whole_data = PJ_TRUE;
+    asock_cfg.grp_lock = ssock->param.grp_lock;
 
     pj_bzero(&asock_cb, sizeof(asock_cb));
@@ -2575 +2602 @@
     asock_cfg.concurrency = ssock->param.concurrency;
     asock_cfg.whole_data = PJ_TRUE;
+    asock_cfg.grp_lock = ssock->param.grp_lock;
 
     pj_bzero(&asock_cb, sizeof(asock_cb));

pjproject/trunk/pjsip/src/pjsip/sip_transport_tcp.c

@@ -62 +62 @@
     pj_qos_params            qos_params;
     pj_sockopt_params        sockopt_params;
+
+    /* Group lock to be used by TCP listener and ioqueue key */
+    pj_grp_lock_t           *grp_lock;
 };
 
@@ -116 +119 @@
     /* Pending transmission list. */
     struct delayed_tdata     delayed_list;
+
+    /* Group lock to be used by TCP transport and ioqueue key */
+    pj_grp_lock_t           *grp_lock;
 };
 
@@ -131 +137 @@
 /* This callback is called by transport manager to destroy listener */
 static pj_status_t lis_destroy(pjsip_tpfactory *factory);
+
+/* Clean up listener resources (group lock handler) */
+static void lis_on_destroy(void *arg);
 
 /* This callback is called by transport manager to create transport */
@@ -400 +409 @@
         asock_cfg.async_cnt = cfg->async_cnt;
 
+    /* Create group lock */
+    status = pj_grp_lock_create(pool, NULL, &listener->grp_lock);
+    if (status != PJ_SUCCESS)
+        return status;
+
+    pj_grp_lock_add_ref(listener->grp_lock);
+    pj_grp_lock_add_handler(listener->grp_lock, pool, listener,
+                            &lis_on_destroy);
+
+    asock_cfg.grp_lock = listener->grp_lock;
+
     pj_bzero(&listener_cb, sizeof(listener_cb));
     listener_cb.on_accept_complete = &on_accept_complete;
@@ -486 +506 @@
 
 
+/* Clean up listener resources */
+static void lis_on_destroy(void *arg)
+{
+    struct tcp_listener *listener = (struct tcp_listener *)arg;
+
+    if (listener->factory.lock) {
+        pj_lock_destroy(listener->factory.lock);
+        listener->factory.lock = NULL;
+    }
+
+    if (listener->factory.pool) {
+        pj_pool_t *pool = listener->factory.pool;
+
+        PJ_LOG(4,(listener->factory.obj_name,  "SIP TCP listener destroyed"));
+
+        listener->factory.pool = NULL;
+        pj_pool_release(pool);
+    }
+}
+
+
 /* This callback is called by transport manager to destroy listener */
 static pj_status_t lis_destroy(pjsip_tpfactory *factory)
@@ -501 +542 @@
     }
 
-    if (listener->factory.lock) {
-        pj_lock_destroy(listener->factory.lock);
-        listener->factory.lock = NULL;
-    }
-
-    if (listener->factory.pool) {
-        pj_pool_t *pool = listener->factory.pool;
-
-        PJ_LOG(4,(listener->factory.obj_name,  "SIP TCP listener destroyed"));
-
-        listener->factory.pool = NULL;
-        pj_pool_release(pool);
+    if (listener->grp_lock) {
+        pj_grp_lock_t *grp_lock = listener->grp_lock;
+        listener->grp_lock = NULL;
+        pj_grp_lock_dec_ref(grp_lock);
+        /* Listener may have been deleted at this point */
+    } else {
+        lis_on_destroy(listener);
     }
 
@@ -563 +599 @@
 /* TCP keep-alive timer callback */
 static void tcp_keep_alive_timer(pj_timer_heap_t *th, pj_timer_entry *e);
+
+/* Clean up TCP resources */
+static void tcp_on_destroy(void *arg);
 
 /*
@@ -641 +680 @@
     tcp->base.destroy = &tcp_destroy_transport;
 
+    /* Create group lock */
+    status = pj_grp_lock_create(pool, NULL, &tcp->grp_lock);
+    if (status != PJ_SUCCESS)
+        goto on_error;
+
+    pj_grp_lock_add_ref(tcp->grp_lock);
+    pj_grp_lock_add_handler(tcp->grp_lock, pool, tcp, &tcp_on_destroy);
+
     /* Create active socket */
     pj_activesock_cfg_default(&asock_cfg);
     asock_cfg.async_cnt = 1;
+    asock_cfg.grp_lock = tcp->grp_lock;
 
     pj_bzero(&tcp_callback, sizeof(tcp_callback));
@@ -781 +829 @@
     }
 
-    if (tcp->rdata.tp_info.pool) {
-        pj_pool_release(tcp->rdata.tp_info.pool);
-        tcp->rdata.tp_info.pool = NULL;
-    }
-
     if (tcp->asock) {
         pj_activesock_close(tcp->asock);
@@ -795 +838 @@
     }
 
+    if (tcp->grp_lock) {
+        pj_grp_lock_t *grp_lock = tcp->grp_lock;
+        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;
+}
+
+/* Clean up TCP resources */
+static void tcp_on_destroy(void *arg)
+{
+    struct tcp_transport *tcp = (struct tcp_transport*)arg;
+
     if (tcp->base.lock) {
         pj_lock_destroy(tcp->base.lock);
@@ -805 +865 @@
     }
 
+    if (tcp->rdata.tp_info.pool) {
+        pj_pool_release(tcp->rdata.tp_info.pool);
+        tcp->rdata.tp_info.pool = NULL;
+    }
+
     if (tcp->base.pool) {
         pj_pool_t *pool;
 
-        if (reason != PJ_SUCCESS) {
+        if (tcp->close_reason != PJ_SUCCESS) {
             char errmsg[PJ_ERR_MSG_SIZE];
 
-            pj_strerror(reason, errmsg, sizeof(errmsg));
+            pj_strerror(tcp->close_reason, errmsg, sizeof(errmsg));
             PJ_LOG(4,(tcp->base.obj_name, 
                       "TCP transport destroyed with reason %d: %s", 
-                      reason, errmsg));
+                      tcp->close_reason, errmsg));
 
         } else {
@@ -827 +892 @@
         pj_pool_release(pool);
     }
-
-    return PJ_SUCCESS;
-}
-
+}
 
 /*

pjproject/trunk/pjsip/src/pjsip/sip_transport_tls.c

@@ -59 +59 @@
     pj_ssl_cert_t           *cert;
     pjsip_tls_setting        tls_setting;
+
+    /* Group lock to be used by TLS transport and ioqueue key */
+    pj_grp_lock_t           *grp_lock;
 };
 
@@ -107 +110 @@
     /* Pending transmission list. */
     struct delayed_tdata     delayed_list;
+
+    /* Group lock to be used by TLS transport and ioqueue key */
+    pj_grp_lock_t           *grp_lock;
 };
 
@@ -134 +140 @@
 /* This callback is called by transport manager to destroy listener */
 static pj_status_t lis_destroy(pjsip_tpfactory *factory);
+
+/* Clean up listener resources (group lock handler) */
+static void lis_on_destroy(void *arg);
 
 /* This callback is called by transport manager to create transport */
@@ -377 +386 @@
     }
 
+    /* Create group lock */
+    status = pj_grp_lock_create(pool, NULL, &listener->grp_lock);
+    if (status != PJ_SUCCESS)
+        return status;
+
+    /* Setup group lock handler */
+    pj_grp_lock_add_ref(listener->grp_lock);
+    pj_grp_lock_add_handler(listener->grp_lock, pool, listener,
+                            &lis_on_destroy);
+
+    ssock_param.grp_lock = listener->grp_lock;
+
     /* Create SSL socket */
     status = pj_ssl_sock_create(pool, &ssock_param, &listener->ssock);
@@ -510 +531 @@
 
 
+/* Clean up listener resources */
+static void lis_on_destroy(void *arg)
+{
+    struct tls_listener *listener = (struct tls_listener*)arg;
+
+    if (listener->factory.lock) {
+        pj_lock_destroy(listener->factory.lock);
+        listener->factory.lock = NULL;
+    }
+
+    if (listener->factory.pool) {
+        pj_pool_t *pool = listener->factory.pool;
+
+        PJ_LOG(4,(listener->factory.obj_name,  "SIP TLS listener destroyed"));
+
+        listener->factory.pool = NULL;
+        pj_pool_release(pool);
+    }
+}
+
+
 /* This callback is called by transport manager to destroy listener */
 static pj_status_t lis_destroy(pjsip_tpfactory *factory)
@@ -525 +567 @@
     }
 
-    if (listener->factory.lock) {
-        pj_lock_destroy(listener->factory.lock);
-        listener->factory.lock = NULL;
-    }
-
-    if (listener->factory.pool) {
-        pj_pool_t *pool = listener->factory.pool;
-
-        PJ_LOG(4,(listener->factory.obj_name,  "SIP TLS listener destroyed"));
-
-        listener->factory.pool = NULL;
-        pj_pool_release(pool);
+    if (listener->grp_lock) {
+        pj_grp_lock_t *grp_lock = listener->grp_lock;
+        listener->grp_lock = NULL;
+        pj_grp_lock_dec_ref(grp_lock);
+        /* Listener may have been deleted at this point */
+    } else {
+        lis_on_destroy(listener);
     }
 
@@ -753 +790 @@
 
 
+/* Clean up TLS resources */
+static void tls_on_destroy(void *arg)
+{
+    struct tls_transport *tls = (struct tls_transport*)arg;
+
+    if (tls->rdata.tp_info.pool) {
+        pj_pool_release(tls->rdata.tp_info.pool);
+        tls->rdata.tp_info.pool = NULL;
+    }
+
+    if (tls->base.lock) {
+        pj_lock_destroy(tls->base.lock);
+        tls->base.lock = NULL;
+    }
+
+    if (tls->base.ref_cnt) {
+        pj_atomic_destroy(tls->base.ref_cnt);
+        tls->base.ref_cnt = NULL;
+    }
+
+    if (tls->base.pool) {
+        pj_pool_t *pool;
+
+        if (tls->close_reason != PJ_SUCCESS) {
+            char errmsg[PJ_ERR_MSG_SIZE];
+
+            pj_strerror(tls->close_reason, errmsg, sizeof(errmsg));
+            PJ_LOG(4,(tls->base.obj_name, 
+                      "TLS transport destroyed with reason %d: %s", 
+                      tls->close_reason, errmsg));
+
+        } else {
+
+            PJ_LOG(4,(tls->base.obj_name, 
+                      "TLS transport destroyed normally"));
+
+        }
+
+        pool = tls->base.pool;
+        tls->base.pool = NULL;
+        pj_pool_release(pool);
+    }
+}
+
 /* Destroy TLS transport */
 static pj_status_t tls_destroy(pjsip_transport *transport, 
@@ -794 +875 @@
     }
 
-    if (tls->rdata.tp_info.pool) {
-        pj_pool_release(tls->rdata.tp_info.pool);
-        tls->rdata.tp_info.pool = NULL;
-    }
-
     if (tls->ssock) {
         pj_ssl_sock_close(tls->ssock);
         tls->ssock = NULL;
     }
-    if (tls->base.lock) {
-        pj_lock_destroy(tls->base.lock);
-        tls->base.lock = NULL;
-    }
-
-    if (tls->base.ref_cnt) {
-        pj_atomic_destroy(tls->base.ref_cnt);
-        tls->base.ref_cnt = NULL;
-    }
-
-    if (tls->base.pool) {
-        pj_pool_t *pool;
-
-        if (reason != PJ_SUCCESS) {
-            char errmsg[PJ_ERR_MSG_SIZE];
-
-            pj_strerror(reason, errmsg, sizeof(errmsg));
-            PJ_LOG(4,(tls->base.obj_name, 
-                      "TLS transport destroyed with reason %d: %s", 
-                      reason, errmsg));
-
-        } else {
-
-            PJ_LOG(4,(tls->base.obj_name, 
-                      "TLS transport destroyed normally"));
-
-        }
-
-        pool = tls->base.pool;
-        tls->base.pool = NULL;
-        pj_pool_release(pool);
+
+    if (tls->grp_lock) {
+        pj_grp_lock_t *grp_lock = tls->grp_lock;
+        tls->grp_lock = NULL;
+        pj_grp_lock_dec_ref(grp_lock);
+        /* Transport may have been deleted at this point */
+    } else {
+        tls_on_destroy(tls);
     }
 
@@ -907 +960 @@
     struct tls_transport *tls;
     pj_pool_t *pool;
+    pj_grp_lock_t *glock;
     pj_ssl_sock_t *ssock;
     pj_ssl_sock_param ssock_param;
@@ -986 +1040 @@
     }
 
-    status = pj_ssl_sock_create(pool, &ssock_param, &ssock);
+    /* Create group lock */
+    status = pj_grp_lock_create(pool, NULL, &glock);
     if (status != PJ_SUCCESS)
         return status;
+
+    ssock_param.grp_lock = glock;
+    status = pj_ssl_sock_create(pool, &ssock_param, &ssock);
+    if (status != PJ_SUCCESS) {
+        pj_grp_lock_destroy(glock);
+        return status;
+    }
 
     /* Apply SSL certificate */
     if (listener->cert) {
         status = pj_ssl_sock_set_certificate(ssock, pool, listener->cert);
-        if (status != PJ_SUCCESS)
+        if (status != PJ_SUCCESS) {
+            pj_grp_lock_destroy(glock);
             return status;
+        }
     }
 
@@ -1005 +1069 @@
     status = tls_create(listener, pool, ssock, PJ_FALSE, &local_addr, 
                         rem_addr, &remote_name, &tls);
-    if (status != PJ_SUCCESS)
+    if (status != PJ_SUCCESS) {
+        pj_grp_lock_destroy(glock);
         return status;
+    }
 
     /* Set the "pending" SSL socket user data */
     pj_ssl_sock_set_user_data(tls->ssock, tls);
+
+    /* Set up the group lock */
+    tls->grp_lock = glock;
+    pj_grp_lock_add_ref(tls->grp_lock);
+    pj_grp_lock_add_handler(tls->grp_lock, pool, tls, &tls_on_destroy);
 
     /* Start asynchronous connect() operation */
@@ -1130 +1201 @@
     /* Set the "pending" SSL socket user data */
     pj_ssl_sock_set_user_data(new_ssock, tls);
+
+    /* Set up the group lock */
+    if (ssl_info.grp_lock) {
+        tls->grp_lock = ssl_info.grp_lock;
+        pj_grp_lock_add_ref(tls->grp_lock);
+        pj_grp_lock_add_handler(tls->grp_lock, tls->base.pool, tls,
+                                &tls_on_destroy);
+    }
 
     /* Prevent immediate transport destroy as application may access it 

pjproject/trunk/pjsip/src/pjsip/sip_transport_udp.c

@@ -77 +77 @@
     int                 is_closing;
     pj_bool_t           is_paused;
+
+    /* Group lock to be used by UDP transport and ioqueue key */
+    pj_grp_lock_t      *grp_lock;
 };
 
@@ -346 +349 @@
 }
 
+
+/* Clean up UDP resources */
+static void udp_on_destroy(void *arg)
+{
+    struct udp_transport *tp = (struct udp_transport*)arg;
+    int i;
+
+    /* Destroy rdata */
+    for (i=0; i<tp->rdata_cnt; ++i) {
+        pj_pool_release(tp->rdata[i]->tp_info.pool);
+    }
+
+    /* Destroy reference counter. */
+    if (tp->base.ref_cnt)
+        pj_atomic_destroy(tp->base.ref_cnt);
+
+    /* Destroy lock */
+    if (tp->base.lock)
+        pj_lock_destroy(tp->base.lock);
+
+    /* Destroy pool. */
+    pjsip_endpt_release_pool(tp->base.endpt, tp->base.pool);
+}
+
+
 /*
  * udp_destroy()
@@ -398 +426 @@
     }
 
-    /* Destroy rdata */
-    for (i=0; i<tp->rdata_cnt; ++i) {
-        pj_pool_release(tp->rdata[i]->tp_info.pool);
-    }
-
-    /* Destroy reference counter. */
-    if (tp->base.ref_cnt)
-        pj_atomic_destroy(tp->base.ref_cnt);
-
-    /* Destroy lock */
-    if (tp->base.lock)
-        pj_lock_destroy(tp->base.lock);
-
-    /* Destroy pool. */
-    pjsip_endpt_release_pool(tp->base.endpt, tp->base.pool);
+    if (tp->grp_lock) {
+        pj_grp_lock_t *grp_lock = tp->grp_lock;
+        tp->grp_lock = NULL;
+        pj_grp_lock_dec_ref(grp_lock);
+        /* Transport may have been deleted at this point */
+    } else {
+        udp_on_destroy(tp);
+    }
 
     return PJ_SUCCESS;
@@ -603 +624 @@
     pj_ioqueue_t *ioqueue;
     pj_ioqueue_callback ioqueue_cb;
+    pj_status_t status;
 
     /* Ignore if already registered */
     if (tp->key != NULL)
         return PJ_SUCCESS;
+
+    /* Create group lock */
+    status = pj_grp_lock_create(tp->base.pool, NULL, &tp->grp_lock);
+    if (status != PJ_SUCCESS)
+        return status;
+
+    pj_grp_lock_add_ref(tp->grp_lock);
+    pj_grp_lock_add_handler(tp->grp_lock, tp->base.pool, tp, &udp_on_destroy);
     
     /* Register to ioqueue. */
@@ -614 +644 @@
     ioqueue_cb.on_write_complete = &udp_on_write_complete;
 
-    return pj_ioqueue_register_sock(tp->base.pool, ioqueue, tp->sock, tp,
-                                    &ioqueue_cb, &tp->key);
+    return pj_ioqueue_register_sock2(tp->base.pool, ioqueue, tp->sock,
+                                     tp->grp_lock, tp, &ioqueue_cb, &tp->key);
 }