Changeset 5778 for pjproject/trunk/pjlib/src/pj/ioqueue_select.c

Timestamp:

Apr 6, 2018 6:11:36 AM (7 years ago)

Author:

ming

Message:

Re #2091:

• Fix possible multiple socket closes and querying already-closed sockets.
• Also prevent possible exception if replace_udp_sock() fails.

If replace_udp_sock() fails, then key->fd will have already been closed. So when calling pj_ioqueue_unregister(key), it will attempt to close the socket again. This may (but not always) result in an exception, which seems to happen when the socket descriptor has been reused by another app.

Explanation: EXC_GUARD exception happens when you try to close a file descriptor that you don't own.

Stack trace:
Exception Type: EXC_GUARD
Exception Subtype: GUARD_TYPE_FD
Exception Message: CLOSE on file descriptor 11 (guarded with 0x08fd4dbfade2dead)
Exception Note: SIMULATED (this is NOT a crash) requested by (null)
Triggered by Thread: 7

Thread 7 Crashed:
0 libsystem_kernel.dylib 0x0000000183b09224 close + 8
1 0x00000001031b5b58 pj_sock_close + 12
2 0x00000001031b1d58 pj_ioqueue_unregister + 120
3 0x000000010313b018 udp_destroy + 44
4 0x0000000103138bf8 destroy_transport + 220
5 0x000000010313942c pjsip_tpmgr_destroy + 132
6 0x0000000103133a40 pjsip_endpt_destroy + 244
7 0x000000010315a1b8 pjsua_destroy2 + 2640

File:

• pjproject/trunk/pjlib/src/pj/ioqueue_select.c (modified) (5 diffs)

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

@@ -510 +510 @@
 
     /* Close socket. */
-    pj_sock_close(key->fd);
+    if (key->fd != PJ_INVALID_SOCKET) {
+        pj_sock_close(key->fd);
+        key->fd = PJ_INVALID_SOCKET;
+    }
 
     /* Clear callback */
@@ -723 +726 @@
     old_sock = h->fd;
 
+    fds_cnt = 0;
+    fds[fds_cnt++] = &h->ioqueue->rfdset;
+    fds[fds_cnt++] = &h->ioqueue->wfdset;
+#if PJ_HAS_TCP
+    fds[fds_cnt++] = &h->ioqueue->xfdset;
+#endif
+
     /* Can only replace UDP socket */
     pj_assert(h->fd_type == pj_SOCK_DGRAM());
@@ -736 +746 @@
         }
         
-        /* Investigate the old socket */
-        addr_len = sizeof(local_addr);
-        status = pj_sock_getsockname(old_sock, &local_addr, &addr_len);
-        if (status != PJ_SUCCESS) {
-            PJ_LOG(5,(THIS_FILE, "Error get socket name %d", status));
-            continue;
-        }
+        if (old_sock != PJ_INVALID_SOCKET) {
+            /* Investigate the old socket */
+            addr_len = sizeof(local_addr);
+            status = pj_sock_getsockname(old_sock, &local_addr, &addr_len);
+            if (status != PJ_SUCCESS) {
+                PJ_LOG(5,(THIS_FILE, "Error get socket name %d", status));
+                continue;
+            }
         
-        addr_len = sizeof(rem_addr);
-        status = pj_sock_getpeername(old_sock, &rem_addr, &addr_len);
-        if (status != PJ_SUCCESS) {
-            PJ_LOG(5,(THIS_FILE, "Error get peer name %d", status));
-        } else {
-            flags |= HAS_PEER_ADDR;
-        }
-
-        status = pj_sock_get_qos_params(old_sock, &qos_params);
-        if (status == PJ_STATUS_FROM_OS(EBADF) ||
-            status == PJ_STATUS_FROM_OS(EINVAL))
-        {
-            PJ_LOG(5,(THIS_FILE, "Error get qos param %d", status));
-            continue;
-        }
+            addr_len = sizeof(rem_addr);
+            status = pj_sock_getpeername(old_sock, &rem_addr, &addr_len);
+            if (status != PJ_SUCCESS) {
+                PJ_LOG(5,(THIS_FILE, "Error get peer name %d", status));
+            } else {
+                flags |= HAS_PEER_ADDR;
+            }
+
+            status = pj_sock_get_qos_params(old_sock, &qos_params);
+            if (status == PJ_STATUS_FROM_OS(EBADF) ||
+                status == PJ_STATUS_FROM_OS(EINVAL))
+            {
+                PJ_LOG(5,(THIS_FILE, "Error get qos param %d", status));
+                continue;
+            }
         
-        if (status != PJ_SUCCESS) {
-            PJ_LOG(5,(THIS_FILE, "Error get qos param %d", status));
-        } else {
-            flags |= HAS_QOS;
-        }
-
-        /* We're done with the old socket, close it otherwise we'll get
-         * error in bind()
-         */
-        status = pj_sock_close(old_sock);
-        if (status != PJ_SUCCESS) {
-            PJ_LOG(5,(THIS_FILE, "Error closing socket %d", status));
+            if (status != PJ_SUCCESS) {
+                PJ_LOG(5,(THIS_FILE, "Error get qos param %d", status));
+            } else {
+                flags |= HAS_QOS;
+            }
+
+            /* We're done with the old socket, close it otherwise we'll get
+             * error in bind()
+             */
+            status = pj_sock_close(old_sock);
+            if (status != PJ_SUCCESS) {
+                PJ_LOG(5,(THIS_FILE, "Error closing socket %d", status));
+            }
+            
+            old_sock = PJ_INVALID_SOCKET;
         }
 
@@ -845 +859 @@
      * fd sets.
      */
-    fds_cnt = 0;
-    fds[fds_cnt++] = &h->ioqueue->rfdset;
-    fds[fds_cnt++] = &h->ioqueue->wfdset;
-#if PJ_HAS_TCP
-    fds[fds_cnt++] = &h->ioqueue->xfdset;
-#endif
-
     for (i=0; i<fds_cnt; ++i) {
-        if (PJ_FD_ISSET(old_sock, fds[i])) {
-            PJ_FD_CLR(old_sock, fds[i]);
+        if (PJ_FD_ISSET(h->fd, fds[i])) {
+            PJ_FD_CLR(h->fd, fds[i]);
             PJ_FD_SET(new_sock, fds[i]);
         }
@@ -871 +878 @@
     if (new_sock != PJ_INVALID_SOCKET)
         pj_sock_close(new_sock);
+    if (old_sock != PJ_INVALID_SOCKET)
+        pj_sock_close(old_sock);
+
+    /* Clear the occurrence of old socket in the fd sets. */
+    for (i=0; i<fds_cnt; ++i) {
+        if (PJ_FD_ISSET(h->fd, fds[i])) {
+            PJ_FD_CLR(h->fd, fds[i]);
+        }
+    }
+
+    h->fd = PJ_INVALID_SOCKET;
     PJ_PERROR(1,(THIS_FILE, status, "Error replacing socket [%d]", status));
     pj_lock_release(h->ioqueue->lock);