[jitsi-dev] [jitsi-videobridge] Fix: race condition between addChannel() and close() when assigning ctpConnection AND channelForDtlsLock (#82)


#1

This request fixes a race condition caused by close and addChannel mutating the reference to SctpConnection and channelForDtls. This can result in a stack trace causing a cast exception. The result is the participant in the conference receives no media.

The fix is a simple synchronize block, since there is a tight connection between the value of the sctpConnection and channelForDtls and two areas where these values can be mutated - addChannel and close. I looked for any chances of dead-locks by carefully analyzing any locks obtained within the new synchronize block and didn't find any; I also ensured all areas that read and write these variables are done while the lock is held, which turned out to be the same areas, allowing for this simpler approach.

Load testing this fix reveals no race conditions are occurring now. The previous pull request made the situation much better, but didn't cover all cases.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/jitsi-videobridge/pull/82

-- Commit Summary --

  * Fix: race condition between addChannel() and close() when assigning sctpConnection AND channelForDtlsLock

-- File Changes --

    M src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java (164)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/82.patch
https://github.com/jitsi/jitsi-videobridge/pull/82.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82


#2

             {
- /*
- * channelForDtls is necessarily an RtpChannel, because we don't
- * add more than one SctpConnection. The SctpConnection socket
- * will automatically accept DTLS.
- */
- RtpChannel rtpChannelForDtls = (RtpChannel) channelForDtls;

This can still cause a cast exception. I can submit another pull request with the fix. The comment is misleading as this can change types during a close.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82/files#r38603016


#3

@@ -391,65 +396,67 @@ public boolean addChannel(Channel channel)
         if (closed)
             return false;

- if (channel instanceof SctpConnection
- && sctpConnection != null
- && sctpConnection != channel)
- {
- logd("Not adding a second SctpConnection to TransportManager.");
- return false;
- }
-
- if (!super.addChannel(channel))
- return false;
-
- if (channel instanceof SctpConnection)
+ synchronized (channelForDtlsLock)

The diff here is actually pretty small - the only thing that changes inside the synchronize block in this commit is whitespace.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82/files#r38603087


#4

@@ -682,42 +689,45 @@ public boolean close(Channel channel)

         if (removed)
         {
- if (channel == sctpConnection)
- {
- sctpConnection = null;
- }
-
- if (channel == channelForDtls)
+ synchronized (channelForDtlsLock)

Again - the diff here looks bigger on GitHub's tool than it may appear - this is mainly whitespace due to being inside a synchronized block.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82/files#r38603136


#5

Thank you for the contribution! This look reasonable to me, but I haven't looked at it in detail. I don't know when someone will be able to review it, but before we can merge it we would need you to sign our contributor agreement (for an [individual](https://jitsi.org/icla) or a [corporation](https://jitsi.org/ccla))

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82#issuecomment-137326346


#6

Corporate CLA signed; I added a commit to check the data type as well.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82#issuecomment-141182852


#7

Can one of the admins verify this patch?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/82#issuecomment-192016163