Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix sctp crash #333

Closed
wants to merge 2 commits into from
Closed

Fix sctp crash #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21cffc9
Revert "handle sctp shutdown issue mentioned here: https://github.com…
chehefen Apr 8, 2020
69f66a6
fix sctp related crash. https://github.com/sctplab/usrsctp/issues/147
chehefen Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,10 @@ matrix:
- 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
- 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

Expand Down
62 changes: 44 additions & 18 deletions src/source/Sctp/Sctp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#define LOG_CLASS "SCTP"
#include "../Include_i.h"

static volatile PSctpSessionControl pSctpSessionControl = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use statics here. This can be part of the object graph. Please change the deinitSctpSession to take this object of a parent object that it can use instead of the global

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing out lives peerConnection, so we dont have a right parent object for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand now your point. Let's make this a high-level object that's lifespan will be tracked at the application level.

Have createSctpSession and freeSctpSession. The object created then will be passed to the perr connections?


STATUS initSctpAddrConn(PSctpSession pSctpSession, struct sockaddr_conn *sconn)
{
ENTERS();
Expand Down Expand Up @@ -63,15 +65,33 @@ STATUS initSctpSession()
// Disable Explicit Congestion Notification
usrsctp_sysctl_set_sctp_ecn_enable(0);

pSctpSessionControl = MEMALLOC(SIZEOF(SctpSessionControl));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The init and deinit can take an object/return an object to avoid the global. This object then can be part of the overall WebRTC object graph

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will still be a global though right? It is shared among all PeerConnections

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having mutable globals is a very bad idea at a high-level code. I think we might need to have a high-level object that lives in parallel with peer connection (like the signaling client) that will be passed into peer connection and it's lifecycle managed by the app

pSctpSessionControl->lock = MUTEX_CREATE(FALSE);
CHK_STATUS(hashTableCreate(&pSctpSessionControl->activeSctpSessions));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the parameterized version of the hash table. The default will create 2 * 10K buckets


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)
Expand All @@ -92,7 +112,12 @@ 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);
// 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));
Expand All @@ -107,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(&params.spp_address, &remoteConn, SIZEOF(remoteConn));
MEMCPY(&params.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, &params, SIZEOF(params)) == 0, STATUS_SCTP_SESSION_SETUP_FAILED);


CleanUp:
if (STATUS_FAILED(retStatus)) {
freeSctpSession(&pSctpSession);
Expand All @@ -129,7 +153,6 @@ STATUS freeSctpSession(PSctpSession* ppSctpSession)
ENTERS();
STATUS retStatus = STATUS_SUCCESS;
PSctpSession pSctpSession;
UINT64 shutdownTimeout;

CHK(ppSctpSession != NULL, STATUS_NULL_ARG);

Expand All @@ -138,20 +161,17 @@ 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);
pSctpSession->socket = NULL;
}

shutdownTimeout = GETTIME() + DEFAULT_SCTP_SHUTDOWN_TIMEOUT;
while(ATOMIC_LOAD(&pSctpSession->shutdownStatus) != SCTP_SESSION_SHUTDOWN_COMPLETED && GETTIME() < shutdownTimeout) {
THREAD_SLEEP(DEFAULT_USRSCTP_TEARDOWN_POLLING_INTERVAL);
}
MUTEX_LOCK(pSctpSessionControl->lock);
hashTableRemove(pSctpSessionControl->activeSctpSessions, pSctpSession->key);
MUTEX_UNLOCK(pSctpSessionControl->lock);

SAFE_MEMFREE(*ppSctpSession);

Expand Down Expand Up @@ -219,19 +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 || ATOMIC_LOAD(&pSctpSession->shutdownStatus) == SCTP_SESSION_SHUTDOWN_INITIATED ||
pSctpSession->sctpSessionCallbacks.outboundPacketFunc == NULL) {
if (pSctpSession != NULL) {
ATOMIC_STORE(&pSctpSession->shutdownStatus, SCTP_SESSION_SHUTDOWN_COMPLETED);
}
if (pSctpSession == NULL || pSctpSession->sctpSessionCallbacks.outboundPacketFunc == NULL) {
return -1;
}

pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length);
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) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when do we return -1?

if (!containKey) {
return 0;
}

pSctpSession->sctpSessionCallbacks.outboundPacketFunc(pSctpSession->sctpSessionCallbacks.customData, data, length);
return 0;
}

Expand Down
18 changes: 11 additions & 7 deletions src/source/Sctp/Sctp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -59,11 +53,21 @@ typedef struct {
} SctpSessionCallbacks, *PSctpSessionCallbacks;

typedef struct {
volatile SIZE_T shutdownStatus;
struct socket *socket;
SctpSessionCallbacks sctpSessionCallbacks;
UINT64 key;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you use store the current time here and this acts as the key in the hash table. Perhaps add a comment to state what you use as a key. Also, we really need to create a hashset object in PIC

} 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*);
Expand Down