From 7d0a77eef431748f26cacee140bef3b36cc9bc22 Mon Sep 17 00:00:00 2001 From: jbauch Date: Fri, 7 Jul 2017 13:44:07 -0700 Subject: [PATCH] Handle case where UDP packet contains multiple DTLS records. Our DTLS implementation doesn't do this, but other implementations may; see https://tools.ietf.org/html/rfc6347#section-4.1.1. BUG=chromium:537189 Review-Url: https://codereview.webrtc.org/2970883005 Cr-Commit-Position: refs/heads/master@{#18934} --- webrtc/p2p/base/dtlstransportchannel.cc | 33 +++++++++++-------- .../p2p/base/dtlstransportchannel_unittest.cc | 16 +++++++++ webrtc/p2p/base/fakeicetransport.h | 27 ++++++++++----- 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index 4249ae7e93..08dfb6b386 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -537,20 +537,25 @@ void DtlsTransport::OnDtlsEvent(rtc::StreamInterface* dtls, int sig, int err) { char buf[kMaxDtlsPacketLen]; size_t read; int read_error; - rtc::StreamResult ret = dtls_->Read(buf, sizeof(buf), &read, &read_error); - if (ret == rtc::SR_SUCCESS) { - SignalReadPacket(this, buf, read, rtc::CreatePacketTime(0), 0); - } else if (ret == rtc::SR_EOS) { - // Remote peer shut down the association with no error. - LOG_J(LS_INFO, this) << "DTLS transport closed"; - set_writable(false); - set_dtls_state(DTLS_TRANSPORT_CLOSED); - } else if (ret == rtc::SR_ERROR) { - // Remote peer shut down the association with an error. - LOG_J(LS_INFO, this) << "DTLS transport error, code=" << read_error; - set_writable(false); - set_dtls_state(DTLS_TRANSPORT_FAILED); - } + rtc::StreamResult ret; + // The underlying DTLS stream may have received multiple DTLS records in + // one packet, so read all of them. + do { + ret = dtls_->Read(buf, sizeof(buf), &read, &read_error); + if (ret == rtc::SR_SUCCESS) { + SignalReadPacket(this, buf, read, rtc::CreatePacketTime(0), 0); + } else if (ret == rtc::SR_EOS) { + // Remote peer shut down the association with no error. + LOG_J(LS_INFO, this) << "DTLS transport closed"; + set_writable(false); + set_dtls_state(DTLS_TRANSPORT_CLOSED); + } else if (ret == rtc::SR_ERROR) { + // Remote peer shut down the association with an error. + LOG_J(LS_INFO, this) << "DTLS transport error, code=" << read_error; + set_writable(false); + set_dtls_state(DTLS_TRANSPORT_FAILED); + } + } while (ret == rtc::SR_SUCCESS); } if (sig & rtc::SE_CLOSE) { RTC_DCHECK(sig == rtc::SE_CLOSE); // SE_CLOSE should be by itself. diff --git a/webrtc/p2p/base/dtlstransportchannel_unittest.cc b/webrtc/p2p/base/dtlstransportchannel_unittest.cc index 225d6f7d24..e5e7a6d620 100644 --- a/webrtc/p2p/base/dtlstransportchannel_unittest.cc +++ b/webrtc/p2p/base/dtlstransportchannel_unittest.cc @@ -670,6 +670,22 @@ TEST_F(DtlsTransportChannelTest, TestTransferDtlsTwoChannels) { TestTransfer(1, 1000, 100, false); } +// Connect with DTLS, combine multiple DTLS records into one packet. +// Our DTLS implementation doesn't do this, but other implementations may; +// see https://tools.ietf.org/html/rfc6347#section-4.1.1. +// This has caused interoperability problems with ORTCLib in the past. +TEST_F(DtlsTransportChannelTest, TestTransferDtlsCombineRecords) { + PrepareDtls(true, true, rtc::KT_DEFAULT); + ASSERT_TRUE(Connect()); + // Our DTLS implementation always sends one record per packet, so to simulate + // an endpoint that sends multiple records per packet, we configure the fake + // ICE transport to combine every two consecutive packets into a single + // packet. + cricket::FakeIceTransport* transport = client1_.GetFakeIceTransort(0); + transport->combine_outgoing_packets(true); + TestTransfer(0, 500, 100, false); +} + // Connect with A doing DTLS and B not, and transfer some data. TEST_F(DtlsTransportChannelTest, TestTransferDtlsRejected) { PrepareDtls(true, false, rtc::KT_DEFAULT); diff --git a/webrtc/p2p/base/fakeicetransport.h b/webrtc/p2p/base/fakeicetransport.h index d9c8485063..eff246e0cf 100644 --- a/webrtc/p2p/base/fakeicetransport.h +++ b/webrtc/p2p/base/fakeicetransport.h @@ -161,6 +161,11 @@ class FakeIceTransport : public IceTransportInternal { // Fake PacketTransportInternal implementation. bool writable() const override { return writable_; } bool receiving() const override { return receiving_; } + // If combine is enabled, every two consecutive packets to be sent with + // "SendPacket" will be combined into one outgoing packet. + void combine_outgoing_packets(bool combine) { + combine_outgoing_packets_ = combine; + } int SendPacket(const char* data, size_t len, const rtc::PacketOptions& options, @@ -168,14 +173,18 @@ class FakeIceTransport : public IceTransportInternal { if (!dest_) { return -1; } - rtc::CopyOnWriteBuffer packet(data, len); - if (async_) { - invoker_.AsyncInvokeDelayed( - RTC_FROM_HERE, rtc::Thread::Current(), - rtc::Bind(&FakeIceTransport::SendPacketInternal, this, packet), - async_delay_ms_); - } else { - SendPacketInternal(packet); + + send_packet_.AppendData(data, len); + if (!combine_outgoing_packets_ || send_packet_.size() > len) { + rtc::CopyOnWriteBuffer packet(std::move(send_packet_)); + if (async_) { + invoker_.AsyncInvokeDelayed( + RTC_FROM_HERE, rtc::Thread::Current(), + rtc::Bind(&FakeIceTransport::SendPacketInternal, this, packet), + async_delay_ms_); + } else { + SendPacketInternal(packet); + } } rtc::SentPacket sent_packet(options.packet_id, rtc::TimeMillis()); SignalSentPacket(this, sent_packet); @@ -233,6 +242,8 @@ class FakeIceTransport : public IceTransportInternal { bool had_connection_ = false; bool writable_ = false; bool receiving_ = false; + bool combine_outgoing_packets_ = false; + rtc::CopyOnWriteBuffer send_packet_; }; } // namespace cricket