From 21cffc96342fde7de3799dfcfc8e3fbf224e0b4c Mon Sep 17 00:00:00 2001 From: chehefen Date: Tue, 7 Apr 2020 19:27:43 -0700 Subject: [PATCH 1/2] Revert "handle sctp shutdown issue mentioned here: https://github.com/sctplab/usrsctp/issues/147" This reverts commit 4e13ff89c81db96fa460be59e90f9d30aa938cc6. --- .travis.yml | 12 +++++++----- src/source/Sctp/Sctp.c | 27 +++++---------------------- src/source/Sctp/Sctp.h | 7 ------- 3 files changed, 12 insertions(+), 34 deletions(-) diff --git a/.travis.yml b/.travis.yml index decb40310b..1a058b06f9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -100,11 +100,13 @@ matrix: # Old Version GCC 4.4 - name: "Linux GCC 4.4 Build" os: linux - before_install: - - sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test - - sudo apt-get -q update - - sudo apt-get -y install gcc-4.4 - - sudo apt-get -y install gdb + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - gcc-4.4 + - gdb compiler: gcc before_script: export CC=gcc-4.4 && mkdir build && cd build && cmake .. -DBUILD_TEST=TRUE diff --git a/src/source/Sctp/Sctp.c b/src/source/Sctp/Sctp.c index 74893e69c7..4ff445f65d 100644 --- a/src/source/Sctp/Sctp.c +++ b/src/source/Sctp/Sctp.c @@ -92,7 +92,6 @@ STATUS createSctpSession(PSctpSessionCallbacks pSctpSessionCallbacks, PSctpSessi MEMSET(&localConn, 0x00, SIZEOF(struct sockaddr_conn)); MEMSET(&remoteConn, 0x00, SIZEOF(struct sockaddr_conn)); - ATOMIC_STORE(&pSctpSession->shutdownStatus, SCTP_SESSION_ACTIVE); pSctpSession->sctpSessionCallbacks = *pSctpSessionCallbacks; CHK_STATUS(initSctpAddrConn(pSctpSession, &localConn)); @@ -129,7 +128,6 @@ STATUS freeSctpSession(PSctpSession* ppSctpSession) ENTERS(); STATUS retStatus = STATUS_SUCCESS; PSctpSession pSctpSession; - UINT64 shutdownTimeout; CHK(ppSctpSession != NULL, STATUS_NULL_ARG); @@ -137,20 +135,9 @@ STATUS freeSctpSession(PSctpSession* ppSctpSession) CHK(pSctpSession != NULL, retStatus); - usrsctp_deregister_address(pSctpSession); - /* handle issue mentioned here: https://github.com/sctplab/usrsctp/issues/147 - * the change in shutdownStatus will trigger onSctpOutboundPacket to return -1 */ - ATOMIC_STORE(&pSctpSession->shutdownStatus, SCTP_SESSION_SHUTDOWN_INITIATED); - if (pSctpSession->socket != NULL) { - usrsctp_set_ulpinfo(pSctpSession->socket, NULL); - usrsctp_shutdown (pSctpSession->socket, SHUT_RDWR); usrsctp_close(pSctpSession->socket); - } - - shutdownTimeout = GETTIME() + DEFAULT_SCTP_SHUTDOWN_TIMEOUT; - while(ATOMIC_LOAD(&pSctpSession->shutdownStatus) != SCTP_SESSION_SHUTDOWN_COMPLETED && GETTIME() < shutdownTimeout) { - THREAD_SLEEP(DEFAULT_USRSCTP_TEARDOWN_POLLING_INTERVAL); + usrsctp_deregister_address(pSctpSession); } SAFE_MEMFREE(*ppSctpSession); @@ -222,16 +209,12 @@ INT32 onSctpOutboundPacket(PVOID addr, PVOID data, ULONG length, UINT8 tos, UINT PSctpSession pSctpSession = (PSctpSession) addr; - if (pSctpSession == NULL || ATOMIC_LOAD(&pSctpSession->shutdownStatus) == SCTP_SESSION_SHUTDOWN_INITIATED || - pSctpSession->sctpSessionCallbacks.outboundPacketFunc == NULL) { - if (pSctpSession != NULL) { - ATOMIC_STORE(&pSctpSession->shutdownStatus, SCTP_SESSION_SHUTDOWN_COMPLETED); - } - return -1; + if (pSctpSession != NULL && pSctpSession->sctpSessionCallbacks.outboundPacketFunc != NULL) { + pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length); + } else { + DLOGE("SCTP attempted to send packet but outboundPacketFunc is not defined"); } - pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length); - return 0; } diff --git a/src/source/Sctp/Sctp.h b/src/source/Sctp/Sctp.h index 04babb1473..012bf39ac5 100644 --- a/src/source/Sctp/Sctp.h +++ b/src/source/Sctp/Sctp.h @@ -16,12 +16,6 @@ extern "C" { #define SCTP_ASSOCIATION_DEFAULT_PORT 5000 #define SCTP_DCEP_HEADER_LENGTH 12 -#define SCTP_SESSION_ACTIVE 0 -#define SCTP_SESSION_SHUTDOWN_INITIATED 1 -#define SCTP_SESSION_SHUTDOWN_COMPLETED 2 - -#define DEFAULT_SCTP_SHUTDOWN_TIMEOUT 2 * HUNDREDS_OF_NANOS_IN_A_SECOND - #define DEFAULT_USRSCTP_TEARDOWN_POLLING_INTERVAL (10 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND) enum { @@ -59,7 +53,6 @@ typedef struct { } SctpSessionCallbacks, *PSctpSessionCallbacks; typedef struct { - volatile SIZE_T shutdownStatus; struct socket *socket; SctpSessionCallbacks sctpSessionCallbacks; } SctpSession, *PSctpSession; From 69f66a6fa4d69b32b784f6930386e6934e88395c Mon Sep 17 00:00:00 2001 From: chehefen Date: Tue, 7 Apr 2020 19:32:58 -0700 Subject: [PATCH 2/2] fix sctp related crash. https://github.com/sctplab/usrsctp/issues/147 --- .travis.yml | 12 ++++----- src/source/Sctp/Sctp.c | 59 ++++++++++++++++++++++++++++++++++++------ src/source/Sctp/Sctp.h | 11 ++++++++ 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/.travis.yml b/.travis.yml index 1a058b06f9..041a34ba25 100644 --- a/.travis.yml +++ b/.travis.yml @@ -100,13 +100,11 @@ matrix: # Old Version GCC 4.4 - name: "Linux GCC 4.4 Build" os: linux - addons: - apt: - sources: - - ubuntu-toolchain-r-test - packages: - - gcc-4.4 - - gdb + before_install: + - sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test + - sudo apt-get -q update + - sudo apt-get -y install gcc-4.4 + - sudo apt-get -y install gdb compiler: gcc before_script: export CC=gcc-4.4 && mkdir build && cd build && cmake .. -DBUILD_TEST=TRUE diff --git a/src/source/Sctp/Sctp.c b/src/source/Sctp/Sctp.c index 4ff445f65d..c79bd329b2 100644 --- a/src/source/Sctp/Sctp.c +++ b/src/source/Sctp/Sctp.c @@ -1,6 +1,8 @@ #define LOG_CLASS "SCTP" #include "../Include_i.h" +static volatile PSctpSessionControl pSctpSessionControl = NULL; + STATUS initSctpAddrConn(PSctpSession pSctpSession, struct sockaddr_conn *sconn) { ENTERS(); @@ -63,15 +65,33 @@ STATUS initSctpSession() // Disable Explicit Congestion Notification usrsctp_sysctl_set_sctp_ecn_enable(0); + pSctpSessionControl = MEMALLOC(SIZEOF(SctpSessionControl)); + pSctpSessionControl->lock = MUTEX_CREATE(FALSE); + CHK_STATUS(hashTableCreate(&pSctpSessionControl->activeSctpSessions)); + +CleanUp: + return retStatus; } VOID deinitSctpSession() { + if (pSctpSessionControl != NULL) { + MUTEX_LOCK(pSctpSessionControl->lock); + hashTableClear(pSctpSessionControl->activeSctpSessions); + MUTEX_UNLOCK(pSctpSessionControl->lock); + } + // need to block until usrsctp_finish or sctp thread could be calling free objects and cause segfault while (usrsctp_finish() != 0) { THREAD_SLEEP(DEFAULT_USRSCTP_TEARDOWN_POLLING_INTERVAL); } + + if (pSctpSessionControl != NULL) { + MUTEX_FREE(pSctpSessionControl->lock); + hashTableFree(pSctpSessionControl->activeSctpSessions); + SAFE_MEMFREE(pSctpSessionControl); + } } STATUS createSctpSession(PSctpSessionCallbacks pSctpSessionCallbacks, PSctpSession* ppSctpSession) @@ -92,6 +112,12 @@ STATUS createSctpSession(PSctpSessionCallbacks pSctpSessionCallbacks, PSctpSessi MEMSET(&localConn, 0x00, SIZEOF(struct sockaddr_conn)); MEMSET(&remoteConn, 0x00, SIZEOF(struct sockaddr_conn)); + // use timestamp as key + pSctpSession->key = GETTIME(); + MUTEX_LOCK(pSctpSessionControl->lock); + CHK_STATUS(hashTableUpsert(pSctpSessionControl->activeSctpSessions, pSctpSession->key, (UINT64) pSctpSession)); + MUTEX_UNLOCK(pSctpSessionControl->lock); + pSctpSession->sctpSessionCallbacks = *pSctpSessionCallbacks; CHK_STATUS(initSctpAddrConn(pSctpSession, &localConn)); @@ -106,12 +132,11 @@ STATUS createSctpSession(PSctpSessionCallbacks pSctpSessionCallbacks, PSctpSessi connectStatus = usrsctp_connect(pSctpSession->socket, (struct sockaddr*) &remoteConn, SIZEOF(remoteConn)); CHK(connectStatus >= 0 || errno == EINPROGRESS, STATUS_SCTP_SESSION_SETUP_FAILED); - memcpy(¶ms.spp_address, &remoteConn, SIZEOF(remoteConn)); + MEMCPY(¶ms.spp_address, &remoteConn, SIZEOF(remoteConn)); params.spp_flags = SPP_PMTUD_DISABLE; params.spp_pathmtu = SCTP_MTU; CHK(usrsctp_setsockopt(pSctpSession->socket, IPPROTO_SCTP, SCTP_PEER_ADDR_PARAMS, ¶ms, SIZEOF(params)) == 0, STATUS_SCTP_SESSION_SETUP_FAILED); - CleanUp: if (STATUS_FAILED(retStatus)) { freeSctpSession(&pSctpSession); @@ -135,11 +160,19 @@ STATUS freeSctpSession(PSctpSession* ppSctpSession) CHK(pSctpSession != NULL, retStatus); + usrsctp_deregister_address(pSctpSession); + if (pSctpSession->socket != NULL) { + usrsctp_set_ulpinfo(pSctpSession->socket, NULL); + usrsctp_shutdown (pSctpSession->socket, SHUT_RDWR); usrsctp_close(pSctpSession->socket); - usrsctp_deregister_address(pSctpSession); + pSctpSession->socket = NULL; } + MUTEX_LOCK(pSctpSessionControl->lock); + hashTableRemove(pSctpSessionControl->activeSctpSessions, pSctpSession->key); + MUTEX_UNLOCK(pSctpSessionControl->lock); + SAFE_MEMFREE(*ppSctpSession); *ppSctpSession = NULL; @@ -206,15 +239,25 @@ INT32 onSctpOutboundPacket(PVOID addr, PVOID data, ULONG length, UINT8 tos, UINT { UNUSED_PARAM(tos); UNUSED_PARAM(set_df); - + BOOL containKey; PSctpSession pSctpSession = (PSctpSession) addr; - if (pSctpSession != NULL && pSctpSession->sctpSessionCallbacks.outboundPacketFunc != NULL) { - pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length); - } else { - DLOGE("SCTP attempted to send packet but outboundPacketFunc is not defined"); + if (pSctpSession == NULL || pSctpSession->sctpSessionCallbacks.outboundPacketFunc == NULL) { + return -1; + } + + MUTEX_LOCK(pSctpSessionControl->lock); + hashTableContains(pSctpSessionControl->activeSctpSessions, pSctpSession->key, &containKey); + MUTEX_UNLOCK(pSctpSessionControl->lock); + + /* If the key is not in activeSctpSessions, then the session must have been freed. Thus do not call callback. + * Fixes issue stated here: https://github.com/sctplab/usrsctp/issues/147. + * (return -1 otherwise usrsctp_finish() won't finish) */ + if (!containKey) { + return 0; } + pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length); return 0; } diff --git a/src/source/Sctp/Sctp.h b/src/source/Sctp/Sctp.h index 012bf39ac5..55d22ebc4b 100644 --- a/src/source/Sctp/Sctp.h +++ b/src/source/Sctp/Sctp.h @@ -55,8 +55,19 @@ typedef struct { typedef struct { struct socket *socket; SctpSessionCallbacks sctpSessionCallbacks; + UINT64 key; } SctpSession, *PSctpSession; +typedef struct { + /* Protect activeSctpSessions access */ + MUTEX lock; + + /* This hash table is used as a set. When sctpSession get created it will be assigned with a key + * and the key is inserted into the table. When sctpSession is freed, its key is removed from + * the table. */ + PHashTable activeSctpSessions; +} SctpSessionControl, *PSctpSessionControl; + STATUS initSctpSession(); VOID deinitSctpSession(); STATUS createSctpSession(PSctpSessionCallbacks, PSctpSession*);