From 5a068083ad676862f5a7f83212728331719e90d0 Mon Sep 17 00:00:00 2001 From: Seth Miller Date: Fri, 21 Aug 2015 08:41:58 -0700 Subject: [PATCH 1/2] Fix: race condition between addChannel() and close() when assigning sctpConnection AND channelForDtlsLock --- .../videobridge/IceUdpTransportManager.java | 164 ++++++++++-------- 1 file changed, 87 insertions(+), 77 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java b/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java index effa785619..4950fd4fa0 100644 --- a/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java +++ b/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java @@ -188,6 +188,11 @@ private static void logd(String s) */ private Channel channelForDtls = null; + /** + * Used to synchronize access to {@link #channelForDtls}. + */ + private final Object channelForDtlsLock = new Object(); + /** * Whether this TransportManager has been closed. */ @@ -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) { - // When an SctpConnection is added, it automatically replaces - // channelForDtls, because it needs DTLS packets for the application - // data inside them. - sctpConnection = (SctpConnection) channel; - if (channelForDtls != null) + if (channel instanceof SctpConnection + && sctpConnection != null + && sctpConnection != channel) { - /* - * 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; - - rtpChannelForDtls.getDatagramFilter(false).setAcceptNonRtp( - false); - rtpChannelForDtls.getDatagramFilter(true).setAcceptNonRtp( - false); + logd("Not adding a second SctpConnection to TransportManager."); + return false; } - channelForDtls = sctpConnection; - } - else if (channelForDtls == null) - { - channelForDtls = channel; - RtpChannel rtpChannel = (RtpChannel) channel; + if (!super.addChannel(channel)) + return false; - // The new channelForDtls will always accept DTLS packets on its - // RTP socket. - rtpChannel.getDatagramFilter(false).setAcceptNonRtp(true); - // If we use rtcpmux, we don't want to accept DTLS packets on the - // RTCP socket, because they will be duplicated from the RTP socket, - // because both sockets are actually filters on the same underlying - // socket. - rtpChannel.getDatagramFilter(true).setAcceptNonRtp(!rtcpmux); - } + if (channel instanceof SctpConnection) + { + // When an SctpConnection is added, it automatically replaces + // channelForDtls, because it needs DTLS packets for the application + // data inside them. + sctpConnection = (SctpConnection) channel; + if (channelForDtls != null) + { + /* + * 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; + + rtpChannelForDtls.getDatagramFilter(false).setAcceptNonRtp( + false); + rtpChannelForDtls.getDatagramFilter(true).setAcceptNonRtp( + false); + } + channelForDtls = sctpConnection; + } + else if (channelForDtls == null) + { + channelForDtls = channel; + + RtpChannel rtpChannel = (RtpChannel) channel; + + // The new channelForDtls will always accept DTLS packets on its + // RTP socket. + rtpChannel.getDatagramFilter(false).setAcceptNonRtp(true); + // If we use rtcpmux, we don't want to accept DTLS packets on the + // RTCP socket, because they will be duplicated from the RTP socket, + // because both sockets are actually filters on the same underlying + // socket. + rtpChannel.getDatagramFilter(true).setAcceptNonRtp(!rtcpmux); + } - if (iceConnected) - channel.transportConnected(); + if (iceConnected) + channel.transportConnected(); - EventAdmin eventAdmin - = conference.getVideobridge().getEventAdmin(); - if (eventAdmin != null) - { - eventAdmin.sendEvent(EventFactory.transportChannelAdded(channel)); + EventAdmin eventAdmin + = conference.getVideobridge().getEventAdmin(); + if (eventAdmin != null) + { + eventAdmin.sendEvent(EventFactory.transportChannelAdded(channel)); + } } - return true; } @@ -682,42 +689,45 @@ public boolean close(Channel channel) if (removed) { - if (channel == sctpConnection) - { - sctpConnection = null; - } - - if (channel == channelForDtls) + synchronized (channelForDtlsLock) { - if (sctpConnection != null) + if (channel == sctpConnection) { - channelForDtls = sctpConnection; + sctpConnection = null; } - else if (channel instanceof RtpChannel) - { - RtpChannel newChannelForDtls = null; - for (Channel c : getChannels()) + if (channel == channelForDtls) + { + if (sctpConnection != null) { - if (c instanceof RtpChannel) - newChannelForDtls = (RtpChannel) c; + channelForDtls = sctpConnection; } - if (newChannelForDtls != null) + else if (channel instanceof RtpChannel) { - newChannelForDtls.getDatagramFilter(false) - .setAcceptNonRtp(true); - newChannelForDtls.getDatagramFilter(true) - .setAcceptNonRtp(!rtcpmux); + RtpChannel newChannelForDtls = null; + + for (Channel c : getChannels()) + { + if (c instanceof RtpChannel) + newChannelForDtls = (RtpChannel) c; + } + if (newChannelForDtls != null) + { + newChannelForDtls.getDatagramFilter(false) + .setAcceptNonRtp(true); + newChannelForDtls.getDatagramFilter(true) + .setAcceptNonRtp(!rtcpmux); + } + channelForDtls = newChannelForDtls; } - channelForDtls = newChannelForDtls; - } - if (channel instanceof RtpChannel) - { - RtpChannel rtpChannel = (RtpChannel) channel; + if (channel instanceof RtpChannel) + { + RtpChannel rtpChannel = (RtpChannel) channel; - rtpChannel.getDatagramFilter(false).setAcceptNonRtp(false); - rtpChannel.getDatagramFilter(true).setAcceptNonRtp(false); + rtpChannel.getDatagramFilter(false).setAcceptNonRtp(false); + rtpChannel.getDatagramFilter(true).setAcceptNonRtp(false); + } } } From ff227b36b633876191edd6ec366802bf69e1d991 Mon Sep 17 00:00:00 2001 From: Seth Miller Date: Thu, 27 Aug 2015 15:53:36 -0700 Subject: [PATCH 2/2] fix: Verify the datatype when switching dtls channels. The close method can change the datatype of the dtls channel even with thread safety - RtpChannel could be closed before the SctpConnection, causing a cast exception. --- .../java/org/jitsi/videobridge/IceUdpTransportManager.java | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java b/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java index 4950fd4fa0..fde5fc99d3 100644 --- a/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java +++ b/src/main/java/org/jitsi/videobridge/IceUdpTransportManager.java @@ -415,13 +415,8 @@ public boolean addChannel(Channel channel) // channelForDtls, because it needs DTLS packets for the application // data inside them. sctpConnection = (SctpConnection) channel; - if (channelForDtls != null) + if (channelForDtls != null && channelForDtls instanceof RtpChannel) { - /* - * 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; rtpChannelForDtls.getDatagramFilter(false).setAcceptNonRtp(